You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by "bitstorm (via GitHub)" <gi...@apache.org> on 2026/09/07 14:22:48 UTC
[PR] WICKET-7204 Don't use INJECTION for ByteBuddy proxy creation [wicket]
bitstorm opened a new pull request, #1577:
URL: https://github.com/apache/wicket/pull/1577
The current proxy creation with byte buddy uses ClassLoadingStrategy.Default.INJECTION. ad class loading strategy, but this relies on Java Unsafe support, which is deprecated and disabled with the newer release of ByteBuddy.
I'm not a bytecode expert but looks like it's the minimal change required.
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7204 Don't use INJECTION for ByteBuddy proxy creation [wicket]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1577:
URL: https://github.com/apache/wicket/pull/1577#issuecomment-5572292489
## [Codecov](https://app.codecov.io/gh/apache/wicket/pull/1577?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `75.00000%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 61.85%. Comparing base ([`ca5252b`](https://app.codecov.io/gh/apache/wicket/commit/ca5252b75f9bd36fff956d0c99b0d49458aa6c56?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4d47a71`](https://app.codecov.io/gh/apache/wicket/commit/4d47a713f34a802a088630c7ec9ca9284e2a18ad?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1577 +/- ##
=========================================
Coverage 61.85% 61.85%
- Complexity 11181 11184 +3
=========================================
Files 1245 1245
Lines 48220 48227 +7
Branches 6759 6760 +1
=========================================
+ Hits 29825 29833 +8
- Misses 15694 15695 +1
+ Partials 2701 2699 -2
```
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7204 Don't use INJECTION for ByteBuddy proxy creation [wicket]
Posted by "bitstorm (via GitHub)" <gi...@apache.org>.
bitstorm merged PR #1577:
URL: https://github.com/apache/wicket/pull/1577
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7204 Don't use INJECTION for ByteBuddy proxy creation [wicket]
Posted by "papegaaij (via GitHub)" <gi...@apache.org>.
papegaaij commented on code in PR #1577:
URL: https://github.com/apache/wicket/pull/1577#discussion_r3961475632
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -17,8 +17,10 @@
package org.apache.wicket.proxy.bytebuddy;
import java.io.Serializable;
+import java.lang.invoke.MethodHandles;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Modifier;
Review Comment:
`Modifier` is not needed once the branch is on the package rather than on the visibility.
```suggestion
```
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -116,10 +120,26 @@ public static <T> Class<T> createOrGetProxyClass(Class<T> type)
.implement(InterceptorMutator.class).intercept(FieldAccessor.ofBeanProperty())
.implement(Serializable.class, IWriteReplace.class, ILazyInitProxy.class).intercept(MethodDelegation.toField(INTERCEPTOR_FIELD_NAME))
.make()
- .load(classLoader, ClassLoadingStrategy.Default.INJECTION.allowExistingTypes())
+ .load(classLoader, loadingStrategy)
.getLoaded());
Review Comment:
The monitor argument makes class creation for one `(loader, type)` pair mutually exclusive. Without it, `UsingLookup` has no `allowExistingTypes()` equivalent and concurrent first use of a package-private type defines the class twice — WICKET-7005 again.
```suggestion
.load(classLoader, resolveLoadingStrategy(type))
.getLoaded(),
DYNAMIC_CLASS_CACHE);
```
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -100,8 +102,10 @@ public <T> T createProxy(final Class<T> type, final IProxyTargetLocator locator)
@SuppressWarnings("unchecked")
public static <T> Class<T> createOrGetProxyClass(Class<T> type)
- {
+ {
+ ClassLoadingStrategy<ClassLoader> loadingStrategy = resolveLoadingStrategy(type);
ClassLoader classLoader = resolveClassLoader();
+
Review Comment:
Resolving the strategy here runs `privateLookupIn` on every `createProxy` call, not just on a cache miss, and makes it throw even when the proxy class is already cached. Moving it into the lambda also lets the trailing whitespace go.
```suggestion
{
ClassLoader classLoader = resolveClassLoader();
```
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -205,7 +225,9 @@ private static boolean hasNoArgConstructor(Class<?> type)
for (Constructor<?> constructor : type.getDeclaredConstructors())
{
if (constructor.getParameterTypes().length == 0)
+ {
Review Comment:
Unrelated to the fix — worth dropping to keep the commit to one thing.
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -116,10 +120,26 @@ public static <T> Class<T> createOrGetProxyClass(Class<T> type)
.implement(InterceptorMutator.class).intercept(FieldAccessor.ofBeanProperty())
.implement(Serializable.class, IWriteReplace.class, ILazyInitProxy.class).intercept(MethodDelegation.toField(INTERCEPTOR_FIELD_NAME))
.make()
- .load(classLoader, ClassLoadingStrategy.Default.INJECTION.allowExistingTypes())
+ .load(classLoader, loadingStrategy)
.getLoaded());
}
+ private static ClassLoadingStrategy<ClassLoader> resolveLoadingStrategy(Class<?> type)
+ {
+ try
+ {
+ int modifiers = type.getModifiers();
+
+ return Modifier.isPublic(modifiers)
+ ? ClassLoadingStrategy.Default.WRAPPER.allowExistingTypes()
+ : ClassLoadingStrategy.UsingLookup.of(MethodHandles.privateLookupIn(type, MethodHandles.lookup()));
+ }
+ catch (IllegalAccessException e)
+ {
+ throw new WicketRuntimeException(e);
+ }
+ }
Review Comment:
This is the core of it: branch on whether the proxy keeps the type's package, not on the type's visibility. Only a `java.*` proxy is renamed into another package by `WicketNamingStrategy`, and that is the only case that needs a class loader of its own — `privateLookupIn` cannot serve it anyway, since `java.base` does not open its packages.
The message on the exception matters here: when it fires, it is because a named module does not `open` its package to wicket-ioc, and a bare `IllegalAccessException` gives the user nothing to act on.
```suggestion
/**
* The proxy has to be defined in the same runtime package as the type it proxies, or the
* package private methods it overrides are not overridden at all. Only a proxy for a
* <em>java.**</em> type is renamed into another package, and needs a class loader of its own.
*/
private static ClassLoadingStrategy<ClassLoader> resolveLoadingStrategy(Class<?> type)
{
if (type.getName().startsWith("java."))
{
return ClassLoadingStrategy.Default.WRAPPER.allowExistingTypes();
}
try
{
return ClassLoadingStrategy.UsingLookup
.of(MethodHandles.privateLookupIn(type, MethodHandles.lookup()));
}
catch (IllegalAccessException e)
{
throw new WicketRuntimeException("Cannot create a proxy for " + type.getName()
+ ", because its package is not open to " + ByteBuddyProxyFactory.class.getModule(), e);
}
}
```
##########
pom.xml:
##########
@@ -159,7 +159,7 @@
<aspectj.version>1.9.25.1</aspectj.version>
<assertj-core.version>3.27.7</assertj-core.version>
<bouncycastle.version>1.85.2</bouncycastle.version>
- <byte-buddy.version>1.18.8</byte-buddy.version>
+ <byte-buddy.version>1.18.13</byte-buddy.version>
Review Comment:
This bump is separable from the fix: 1.18.13 does not disable `INJECTION` (I checked on JDK 25), so it is not what forces the change. No objection to bumping, but it reads as part of the fix here.
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7204 Don't use INJECTION for ByteBuddy proxy creation [wicket]
Posted by "bitstorm (via GitHub)" <gi...@apache.org>.
bitstorm commented on code in PR #1577:
URL: https://github.com/apache/wicket/pull/1577#discussion_r3961754690
##########
pom.xml:
##########
@@ -159,7 +159,7 @@
<aspectj.version>1.9.25.1</aspectj.version>
<assertj-core.version>3.27.7</assertj-core.version>
<bouncycastle.version>1.85.2</bouncycastle.version>
- <byte-buddy.version>1.18.8</byte-buddy.version>
+ <byte-buddy.version>1.18.13</byte-buddy.version>
Review Comment:
It fails on JDK > 25 (i.e. 26) starting from version 1.18.9. See PR https://github.com/apache/wicket/pull/1574 . So yes, we better keep version bumping out of this commit but I had to do it to test the result on JDK 26.
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7204 Don't use INJECTION for ByteBuddy proxy creation [wicket]
Posted by "reiern70 (via GitHub)" <gi...@apache.org>.
reiern70 commented on code in PR #1577:
URL: https://github.com/apache/wicket/pull/1577#discussion_r3960181638
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -116,10 +120,26 @@ public static <T> Class<T> createOrGetProxyClass(Class<T> type)
.implement(InterceptorMutator.class).intercept(FieldAccessor.ofBeanProperty())
.implement(Serializable.class, IWriteReplace.class, ILazyInitProxy.class).intercept(MethodDelegation.toField(INTERCEPTOR_FIELD_NAME))
.make()
- .load(classLoader, ClassLoadingStrategy.Default.INJECTION.allowExistingTypes())
+ .load(classLoader, loadingStrategy)
.getLoaded());
}
+ private static ClassLoadingStrategy<ClassLoader> resolveLoadingStrategy(Class<?> type)
Review Comment:
@bitstorm javadoc?
--
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: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org