Repository navigation
Fix to avoid a separate ClassLoader for proxied classes - #1722
Conversation
utafrali
left a comment
There was a problem hiding this comment.
The fix correctly identifies and resolves the root cause: the proxy class cache was keyed on the thread context ClassLoader while ByteBuddy's UsingLookup strategy (introduced in WICKET-7204) actually defines the class in the proxied type's ClassLoader, causing LinkageError on a second request thread. The change is surgical, well-documented, and accompanied by a direct regression test.
reiern70
left a comment
There was a problem hiding this comment.
The fix is right. Since WICKET-7204 the proxy is defined with UsingLookup in the proxied type's own class loader. That ignores the loader passed to load(...) and, unlike the old INJECTION.allowExistingTypes(), doesn't tolerate an existing class. The cache, though, is still keyed on the class resolver / context class loader. So a thread with a different loader misses the cache and defines WicketProxy_X a second time in the same loader, which throws LinkageError: attempted duplicate class definition. Keying the cache on the loader the class is defined in fixes it. Without the fix the new test fails with exactly that error, and with the fix it passes.
Master: master has the same 7204 commit (c0e4956) and an identical ByteBuddyProxyFactory. The test fails there in the same way. Could this go to master first and be cherry-picked to 10.x, either by retargeting this PR or adding a master PR? No release has 7204 yet (10.11.0 predates it, and there is no 11.x), so both lines need this before their next release.
Suggestions (none of them blocking):
- The
startsWith("java.")check now appears in bothresolveLoadingStrategyandresolveClassLoader(Class). If the two ever disagree, the bug comes back. One helper used by both would keep them in step. - The
typeClassLoader != nullfallback never gives a working proxy: a non-java.*type on the bootstrap loader can't get a private lookup anyway. It's harmless, but it could go. - The Javadoc on the private
resolveClassLoader(Class), and on the test, is a paragraph of rationale. One line would do, e.g. "The class loader the proxy for the type is defined in, and cached for." The reasoning fits better in the commit body, which is empty right now. - The commit subject says "avoid a separate ClassLoader", but the change is to the cache key, not to where the class is defined. Something like "Cache ByteBuddy proxy classes for the class loader they are defined in", with a
WICKET-7204reference in the body. The PR description has the cause backwards: the proxies are defined in the type's own loader, but cached under a different one. It also has a typo ("Ddue").
…proxies A non-java.* type on the bootstrap class loader cannot get a private lookup, so its proxy fails on that regardless of the class loader it is cached for; the fallback to the application's class loader for it is gone. isInternalJavaType becomes isInJavaPackage, because java.sql and java.net.http are not internal. The rationale on resolveClassLoader(Class) moves to the commit message, and a test proxies java.sql.Timestamp, whose platform class loader is the reason the java.* check in resolveClassLoader(Class) has to stay. Generated-by: Claude Code (Claude Opus 5.5)
|
@reiern70 thanks for the review. All four suggestions are in, together with the master question:
|
Since WICKET-7204 a ByteBuddy proxy class is defined with
UsingLookupin the class loader of the proxied type, but the proxy class cache was still keyed on the application's class resolver, i.e. the context class loader of the current thread. A thread with another context class loader missed the cache and defined the proxy class a second time in the same loader, which fails withLinkageError: attempted duplicate class definition.The cache is now keyed on the class loader the proxy is defined in. Types in a
java.*package keep their own path: they are renamed, defined in a wrapper loader and cached for the application's loader, becausejava.sqland other platform types have a loader that cannot see Wicket's interfaces.ByteBuddyProxyFactoryTestcovers both: the same proxy class on a thread with another context class loader (fails with theLinkageErrorwithout the fix), and a proxy forjava.sql.Timestamp.The same change goes to master in #1726; this PR is its 10.x counterpart. Squash on merge.
Generated-by: Claude Code (Claude Opus 5.5)