Affects: 5.3
There's a problem in Spring Boot's DevTools that's caused by an AOP proxy being created using the restart class loader rather than the class loader of the proxy's target. This causes problem when calling package-private methods on the proxy.
I've hacked together something that overrides the proxy creator to use the target's class loader and it fixes the problem. I'd now like to explore how we could make the solution more robust and I think some Framework changes are required. The override of AbstractAutoProxyCreator.createProxy is only changing the behaviour of a single line:
https://github.com/spring-projects/spring-framework/blob/a9240e0bac4d71792bc22642093cbda86ed82f85/spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/AbstractAutoProxyCreator.java#L461
Rather than using getProxyClassLoader(), my override uses beanClass.getClassLoader().
The other part of the problem is then configuring the context to use the customized AnnotationAwareAspectJAutoProxyCreator. I'm changing the class of its bean definition at the moment. Perhaps there's already a more elegant way to do this?
Comment From: jhoeller
This is related to the recent #26403 where we were trying to prevent an illegal access warning for defining proxy classes in Boot's RestartClassLoader when the original class lives in the parent ClassLoader. Ironically by not using that recent SmartClassLoader.publicDefineClass mechanism and preventing illegal access completely on JVM startup, we'd end up defining the proxy class in the original bean ClassLoader anyway since that's the only thing the JDK 9+ Lookup.defineClass API can do for us. That said, of course we should be aiming for consistent behavior here and not rely on JDK configuration differences.
Looking at it the other way round, when do we have to define the proxy class in the restart loader in such a scenario? Only really when the original class has also been loaded there, so that'd be covered by using beanClass.getClassLoader() already. However, the latter isn't appropriate in other scenarios: Whenever the AOP proxy refers to aspects from a more specific class loader than the original bean class came from, we should be defining the proxy in that specific class loader. Even worse, when the original bean class came from a low-level class loader which doesn't even know about Spring's AOP implementation classes, we have to use a more specific class loader as well. That's why we use getProxyClassLoader() there, typically referring to the common application class loader (usually the same as the bean class loader in the bean factory).
I tend to see the restart class loader as a special case here. For general class loaders, we cannot reasonably infer whether the proxy class loader is necessary or the original bean class loader is sufficient. However, for restart class loaders, we should only really use the restart loader if the original bean class has also been loaded there. In all other scenarios, the application's bean class loader is sufficient or even preferable (given the package-private method scenario). We could simply introduce such an indication in the SmartClassLoader interface, to be implemented by Boot's RestartClassLoader accordingly, and automatically take it into acocunt in AbstractAutoProxyCreator.
Comment From: jhoeller
Let's assume the following code in AbstractAutoProxyCreator:
ClassLoader targetClassLoader = getProxyClassLoader();
if (targetClassLoader != beanClass.getClassLoader() && targetClassLoader instanceof SmartClassLoader) {
targetClassLoader = ((SmartClassLoader) targetClassLoader).getOriginalClassLoader();
}
return proxyFactory.getProxy(targetClassLoader);
Boot would then override RestartClassLoader.getOriginalClassLoader() to return the original bean class loader (maybe a straightforward getParent()?) whereas a regular non-DevTools application ClassLoader would get used as-is (even if implements SmartClassLoader, it should simply leave the default getOriginalClassLoader() method returning this). This would cover all scenarios as far as I can see, effectively matching beanClass.getClassLoader() for regular application classes while still picking up the application class loader for classes from lower-level loaders (such as JDK library classes).
Comment From: wilkinsona
Thanks very much, Juergen. This all sounds good to me. In particular, I agree that the restart class loader is a special case here. Our intention would be to implement this fix in DevTools. Doing that by making RestartClassLoader a SmartClassLoader and a new getOriginalClassLoader method would be pleasingly elegant.
We also have https://github.com/spring-projects/spring-boot/issues/19010 which I think is another variant of the same problem. In that scenario, the restart class loader was being used to create a proxy that's sub-classing a private class (I mistakenly said package-private in the issue). The super-class is part of Spring Security and was loaded by the app class loader. This causes the proxying attempt to fail with an IllegalAccessError occurs. Applying the same hack to change the class loader fixes this problem too with the proxy now being created using the app class loader.
Comment From: jhoeller
Even more elegantly, RestartClassLoader seems to be a SmartClassLoader already :-) Based on #26403, @snicoll implemented publicDefineClass to enforce a warning-free definition in the restart class loader... which seems to be contradicting this issue a bit but is still worth having for cases where we do need to define a class in the restart class loader. Actually, such a publicDefineClass method was meant to be made available in the regular non-DevTools ClassLoader implementation in Boot as well, preventing an illegal access warning e.g. for proxying JDK library classes.
Comment From: wilkinsona
Actually, such a publicDefineClass method was meant to be made available in the regular non-DevTools ClassLoader in Boot as well, preventing an illegal access warning e.g. for proxying JDK library classes.
Unfortunately, outside of DevTools, we don't always control the ClassLoader. For example, when an application's launched in your IDE or run via Maven or Gradle, the ClassLoader will be the JVM's system class loader.
Comment From: jhoeller
Being at it, could Boot allow for setting up a custom SmartClassLoader in such IDE/Maven/Gradle scenarios even outside of DevTools? With custom user configuration, just in case anyone complains about remaining illegal access warnings in their scenario?
It probably will be less of an issue in JDK 16+, actually, since denying illegal access by default there leads to us silently defining the class in the original bean's ClassLoader via Lookup.defineClass anyway... which works for everything but the JDK library loader. In that sense, it's also a solution for people to bootstrap their JVM with --illegal-access=deny in order to get rid of those warnings, except for defining proxies for JDK library classes where we really need a publicDefineClass-enabled ClassLoader. The latter is exactly the scenario where a SmartClassLoader for non-DevTools scenarios still makes sense as an option to provide.
Comment From: wilkinsona
Being at it, could Boot allow for setting up a custom SmartClassLoader in such IDE/Maven/Gradle scenarios even outside of DevTools? With custom user configuration, just in case anyone complains about remaining illegal access warnings in their scenario?
That's something that I'd quite like us to be able to do, but I haven't had the time to figure out how we could do it in a consistent manner. It would also help in instrumentation scenarios, such as https://github.com/spring-projects/spring-boot/issues/22109, where users would rely on things like Tomcat's InstrumentableClassLoader in a more traditional deployment.
Comment From: wilkinsona
@jhoeller Would it be possible to make a similar change to AbstractAdvisingBeanPostProcessor?
Comment From: jhoeller
Indeed, that's pretty much the same case there. Done!
Comment From: wilkinsona
Thanks!