Allow a caller supplied factory to create the class realm - #152
Allow a caller supplied factory to create the class realm#152slachiewicz wants to merge 4 commits into
Conversation
Extracted from #144, where a transformer-backed realm needed ClassWorld to register an implementation it does not know about. Duplicate detection, registration and listener notification stay here so callers do not reimplement them. The factory runs before the id is known, so a realm whose id turns out to be taken is closed before the exception is thrown rather than leaked.
slachiewicz
added
enhancement
There was a problem hiding this comment.
Pull request overview
Adds a new ClassWorld API to allow callers to supply a factory for creating the ClassRealm, enabling custom realm implementations to be registered while preserving existing behavior for the id-based overloads.
Changes:
- Refactors realm creation/registration to use a shared
register(ClassRealm)helper. - Introduces
ClassWorld#newRealm(Supplier<ClassRealm>)with validation, duplicate handling, and cleanup-on-rejection semantics. - Adds JUnit tests covering the new supplier-based realm creation, duplicate handling, null handling, and cross-world rejection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/org/codehaus/plexus/classworlds/ClassWorld.java | Adds supplier-based realm creation API and centralizes registration logic. |
| src/test/java/org/codehaus/plexus/classworlds/ClassWorldTest.java | Adds tests for the new supplier-based realm creation behavior and edge cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A factory is free to return the realm already registered under that id, for instance world.newRealm(() -> world.getClassRealm(id)) or anything that memoises. The duplicate path then closed that realm and left it in the world, so a failed call silently killed a live class loader: after it, getResource returned null and loadClass threw ClassNotFoundException. Only realms that are not the registered instance are closed now. The regression test gives the realm a jar of its own, since closing a realm with no URLs of its own has no observable effect.
register() called getId() a second time, so a subclass that overrides it could be checked under one key and stored under another. The caller now passes the id it validated. Also documents that the factory runs under the world's monitor and that a realm from another world is rejected without being closed, and stops the foreign-world test leaking its second world.
Reshapes the new API from newRealm(Supplier<ClassRealm>) to createRealm(String, Function<String, ClassRealm>). The duplicate check now runs before the factory does, which is the invariant the id based overloads already have: a rejected call has no side effect, so there is no realm to close again and no way to close the one already registered. Not an overload of newRealm because a second two argument newRealm makes newRealm(id, null) ambiguous, which the library itself relies on in ClassRealm.createChildRealm and downstream callers do too. The factory result is checked against the requested id, since the registry key and the realm's own id would otherwise be free to disagree.
|
The close-on-duplicate bug was real. With a realm that owns a jar, a rejected call left a closed loader registered in the world: Guarding it with an identity check worked, but taking the id up front removes the branch instead. A distinct name rather than an overload: a second two argument Not taking the other half of the suggestion. Calling the factory outside the monitor is safe, but the id based overload already constructs a realm under the same lock, so this adds no exposure that was not there; and moving the call out would force a re-check afterwards, reintroducing exactly the branch the reshape deletes. Verified: reverting the two main-source files and rerunning fails the new test on a realm that is still registered but closed. This comment was created with AI assistance. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/main/java/org/codehaus/plexus/classworlds/ClassWorld.java:141
register()blindly overwrites an existing realm entry viarealms.put(id, realm). WithcreateRealm(...), the factory runs while the world monitor is held and can re-enterClassWorldand register the same id, causing thisputto silently replace the existing realm (and potentially leak the replaced realm). Guard against duplicates insideregister()itself so it never overwrites an existing id.
private ClassRealm register(String id, ClassRealm realm) {
realms.put(id, realm);
for (ClassWorldListener listener : listeners) {
listener.realmCreated(realm);
Extracted from #144. That draft needs
ClassWorldto register a realm implementation it does not know about, and the review there converged on the transformer realm itself living in Maven rather than here — this is the part that belongs in classworlds.createRealm(String id, Function<String, ClassRealm> factory)checks the id before running the factory, so a rejected call has no side effect: no realm to close again, and no way to close the one already registered under that id. The realm handed back is checked against the requested id, so the registry key andgetId()cannot disagree, and a realm belonging to another world is rejected without being closed since it may be live over there.It is not an overload of
newRealm. A second two argumentnewRealmmakes the literalnewRealm(id, null)ambiguous, andClassRealm.createChildRealmrelies on that call, as do downstream callers.Nothing else from #144 comes along:
ClassRealmstays concrete, the default realm fornewRealm(id, classLoader, null)is unchanged, and no new dependencies are added.@since 2.13.0presumes this ships as a minor rather than in 2.12.1; the pom moves at release time.This change was created with AI assistance.