rzo1 commented on PR #210: URL: https://github.com/apache/openjpa/pull/210#issuecomment-6076915895
The diagnosis and the fix look correct. `Class.forName(name, true, multiLoader)` registers the loader instance with the JVM, and a fresh `MultiClassLoader` per `resolveClass` call meant JVM-internal state grew with every deserialized class name. Resolving the candidates once and calling `forName` with the real loaders avoids that. The comment recording that `MultiClassLoader` now serves only as an ordered, de-duplicating container is worth keeping. The `BlacklistClassResolver` check remains first, so security behaviour is unchanged. One effect is not mentioned in the description: the new `super.resolveClass(desc)` fallback also covers primitive descriptors (`int`, `boolean`) and the `latestUserDefinedLoader` path, which the previous single `Class.forName` did not. Three remarks, none blocking: 1. The lazy initialisation cannot move into the constructor: `PersistentObjectInputStream` assigns `_ctx` after `super(delegate)`, and its `addContextClassLoaders` override dereferences it, so eager initialisation would throw an NPE. A comment would prevent a later simplification. 2. Only the first `ClassNotFoundException` is retained. `notFound.addSuppressed(e)` for the others would make failures across several loaders easier to diagnose. 3. `java.util.Arrays` and `java.util.Objects` are fully qualified, while the rest of the file uses imports. Also, about two thirds of the diff is reformatting, placing a ~20 line functional change inside 113 lines; that hinders review, `git blame` and a 3.2.x backport. On tests: "no new classloaders are registered" is hard to assert, but the resolution paths are cheap to pin — a class visible only through the thread context loader, an array type, and a primitive descriptor, the last covering the new fallback. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
