MethodHandles.Lookup.defineClass for CGLIB class definition purposes [SPR-15859]

[spring-projects-issues]

Juergen Hoeller opened SPR-15859 and commented

As discussed in cglib/cglib@d6fe1d8 and in #20245 comments, there is currently a defineClass warning triggered by CGLIB when running on JDK 9 in classpath mode. While there are workarounds for it ("illegal-access=deny" or "add-opens java.base/java.lang=ALL-UNNAMED"), suppressing that warning at runtime, it'd be nice to avoid the warning completely when running on JDK 9, possibly through a specific check for JDK 9 which skips the ClassLoader.defineClass access attempt completely, always going with the Unsafe.defineClass fallback right away in such a scenario. We have yet to see whether this can be patched in CGLIB itself or just in Spring's CGLIB fork.

UPDATE: Since JDK 11 won't have Unsafe.defineClass at all anymore, we need to use MethodHandles.Lookup.defineClass as our primary mechanism, avoiding a ClassLoader.defineClass warning on the classpath and providing compatibility with the module path on JDK 11.

---

Affects: 5.0 RC3

Reference URL: cglib/cglib@d6fe1d8

Issue Links:

• Compatibility with JDK 11 [SPR-16391] #20937 Compatibility with JDK 11 ("is depended on by")
• An illegal reflective access operation has occurred [SPR-16777] #21317 An illegal reflective access operation has occurred ("duplicates")
• Running an app with @Configuration using Java 9 prints ugly illegal access warnings [SPR-15939] #20493 Running an app with @Configuration using Java 9 prints ugly illegal access warnings ("is duplicated by")
• ReflectUtils produces Warning in Spring Boot 2.0.0.M6 and Java 9.0.1 [SPR-16236] #20783 ReflectUtils produces Warning in Spring Boot 2.0.0.M6 and Java 9.0.1 ("is duplicated by")
• Upgrade to CGLIB 3.2.5 [SPR-15147] #19713 Upgrade to CGLIB 3.2.5
• DATACMNS-1376 Assure JDK 11 compatibility for DefaultMethodInvokingMethodInterceptor
• i'm new with spring and from the start i found this warning while executing [SPR-17380] #21913 i'm new with spring and from the start i found this warning while executing
• Spring Boot DevTools on 5.1 fails with java.lang.LinkageError: loader attempted duplicate class definition [SPR-16902] #21441 Spring Boot DevTools on 5.1 fails with java.lang.LinkageError: loader attempted duplicate class definition
• Illegal reflective access operation warning for toString() on CGLIB proxies [SPR-17500] #22032 Illegal reflective access operation warning for toString() on CGLIB proxies
• DATACMNS-1401 Warning about illegal reflective access in Spring Data Commons / MongoDB

Referenced from: commits 6a34ca2, 61c3db0

17 votes, 40 watchers

[spring-projects-issues]

Alan Bateman commented

Look up java.lang.invoke.MethodHandles.Lookup.defineClass, it's the supported way to inject classes into the same runtime package as another class. It is new in Java SE 9 so it means using a MR JAR or using reflective if you are compiling to an older release. Also note that Unsafe.defineClass is deprecated (forRemoval=true) so it will likely be removed at some point.

[spring-projects-issues]

Juergen Hoeller commented

AlanBateman, the MethodHandles.Lookup.defineClass option has been a topic with Rafael Winterhalter already... but supporting that approach in CGLIB proper seems to be out of scope, also due to the underlying MethodHandles facility being JDK 7+ to begin with.

From my perspective, the ClassLoader.defineClass technique is still the best compromise for CGLIB by default. Along those lines, the similarly structured (and JDK 6 compatible) Unsafe.defineClass is ok as a fallback for the time being, in particular for older applications and frameworks (including Spring Framework 4.3.x) to have a chance of running on JDK 9's module path.

However, for Spring Framework 5.0+, we could enforce a different default policy. In particular, we could ship a JDK 8/9 oriented CGLIB fork that uses MethodHandles.Lookup.defineClass on JDK 9 (even in classpath mode) and falls back to CGLIB's regular policy otherwise.

[spring-projects-issues]

Rafael Winterhalter commented

The problem that we face in cglib is not the requirement to use MethodHandles.Lookup::defineClass - this can be solved by using reflection conditionally on the availablility of this class - but the API implications of this change. Via a lookup, it is only possible - by design - to define a class in the package in which the lookup was created. However, most proxies are today created in the package of the instrumented class to allow proxying package-private methods or to subclass package-private types. By using MethodHandles.Lookup, we would need end-users to provide an appropriate lookup instance for each class's definition or we would need to define proxy classes in a cglib-owned package which would change the nature of those proxies.

cglib has existed since Java 1.1 and has a large use-base. At the same time, it is no longer under active development what led to people finding work-arounds for bugs. As a consequence, it is almost impossible to change anything without breaking things for many users which is why I am hesistant to apply such changes.

In Byte Buddy, users can supply a ClassLoadingStrategy where I implemented a new strategy for using a MethodHandles.Lookup. But as mentioned, this is insufficient for most use cases. For example, in Mockito, we really need to allow proxying package-private types as users often want to test internal API in their unit tests. If we wanted to use lookup instance, we would need to change the Mockito API to:

Foo foo = mock(Foo.class, MethodHandles.lookup());

for any mock class what would destroy the API's expressiveness and make field injection impossible. Therefore, we still rely on the class definition via ClassLoader/Unsafe. A similar problem occurs in Java agents that often need to define helper classes (just as javac) which should typically live in the same package / module as the instrumented class. Agents can however work around any problem by redefining the JVM's module to open certain packages even if the jdk.unsupported package disappeared.

As for now: I could add a system property to cglib that allows always using sun.misc.Unsafe which Spring could set before loading its first cglib-related class. Would this solve your problem? If you shade cglib, you automatically shade the property's name space such that this should be invisible to the outside.

[spring-projects-issues]

Alan Bateman commented

If Spring could use Lookup.defineClass then it would avoid cglib needing to know about Lookup objects.

[spring-projects-issues]

Rafael Winterhalter commented

Typically, Spring defines a proxy in the same package as a user class to access package-private methods. This would require getting hold of a lookup object provided by the user which would require an API change on Spring's side to get hold of such a lookup with the right privileges.

[spring-projects-issues]

Alan Bateman commented

If the user module opens the package to Spring then Spring should be able obtain a full power Lookup with MethodHandles.privateLookupIn.

[spring-projects-issues]

Rafael Winterhalter commented

That is true for bean classes that are defined and controlled by the user but not for classes that are provided by third-parties that do not neccessarily open their packages to Spring. This would require a command-line opening of those modules. That said, using a method handle will most likely work out for many cases but it is not a drop-in replacement.

[spring-projects-issues]

Alan Bateman commented

If the developer of a library chooses to encapsulate its internals then that should be respected. If Spring is dependent on breaking into that library then it should work with the maintainer of the library to come up with a solution.

[spring-projects-issues]

Juergen Hoeller commented

To be clear, what Rafael is referring to here is not that Spring is trying to break into any particular library but rather that Spring's generic AOP facilities can be used to create class-based proxies for arbitrary target classes... So some users may have chosen to apply AOP to third-party classes that are not fully opened, with the framework just doing its usual generic stuff about it.

That said, I don't see this as a major problem. It's the user's responsibility to make sure that everything is properly opened in a module arrangement, according to the purposes of that particular application setup. And in most cases, all that really matters in AOP setup is the public API surface of that particular class, so even a public lookup handle should be sufficient.

Beyond AOP, we're using defineClass for a few other features as well, not least of it all for @Configuration classes. It's definitely more common there to declare non-public artifacts but at least such classes are usually in the user's own codebase. I'll experiment a bit with a custom CGLIB ReflectUtils variant that uses Lookup.defineClass; let's see how it goes.

[spring-projects-issues]

Juergen Hoeller commented

Hmm, I see that Lookup.defineClass requires PACKAGE access in any case... So I suppose a public lookup won't be sufficient, even if the resulting class effectively just propagates public API calls.

[spring-projects-issues]

Mark Mielke commented

If the developer of a library chooses to encapsulate its internals then that should be respected. If Spring is dependent on breaking into that library then it should work with the maintainer of the library to come up with a solution.

From my perspective... The difficulty here is that modules didn't exist prior to Java 9, and the concept of "developer of a library chooses to encapsulate its internals" was only a recommendation as it could not be enforced. We don't actually know who intended for the modules to be usable with Spring in the ways they might be used at present, and who didn't. All we really know, is that over several years certain the class and method access qualifiers, and the Spring implementation have evolved to a current state. Then, Java 9 came along and made modules restricted by default. This effectively broke compatibility, and requires the use of the "-add-opens" and other flags to restore elements of the previous long-standing behaviour.

I don't disagree with modules themselves, but I think we should be looking at this problem as "now that compatibility has been broken, and things must be more explicit... which libraries need to change to enable their use in Java 9?" Spring is one of them. But, all these third party libraries are also a concern. And getting them all to open themselves up to Spring, or other AOP frameworks, is either going to be a chore, or a severe frustration which will result in "-add-opens" and other such flags being long-standing new best practices, or the module system relaxed in some way.

It's a good opportunity to re-evaluate. However, the practical element may mean that even if everybody agrees that something should be done, it is still very difficult or impossible to do it (for example, third party libraries where the original authors are not willing to support Java 9 yet or ever?).

But... that all said... when I read the Spring documentation around these topics, and the ability to use it against third party code, I remember having spider sensations on the back of my neck and thinking this didn't sound wise, and was likely to bite us. So, I believe in my case, we have never done this. I can't think of a single place in our code where we couldn't enable Spring to have access. I think this makes us lucky, though, and we have to consider those who read the same documentation, and thought it sounded like a great idea to take advantage of, and design their architecture around...

[spring-projects-issues]

Alan Bateman commented

I don't understand the last comment. If existing libraries are used as modules then they work as automatic modules, no encapsulation. In any case, this issue is about Spring+CGLIB using the protected ClassLoader.defineClass method from the wrong context and seeing if the the Lookup.defineClass method can be used instead.

Juergen - yes, it needs PACKAGE access because the resulting class will get package access. If the application has opened the package to spring then you can use privateLookupIn to get a full-power Lookup that includes PACKAGE access.

[spring-projects-issues]

Juergen Hoeller commented

AlanBateman, Rafael Winterhalter, this sounds almost like the CGLIB ReflectUtils implementation could try MethodHandles.privateLookupIn first (when on JDK 9), falling back to ClassLoader.defineClass (on JDK 8 or when there was a lookup exception before), then finally to Unsafe.defineClass. Not necessarily saying that CGLIB's default implementation has to do this; we could replace it with a custom variant in Spring's CGLIB fork (which only really has to work for Spring's particular purposes).

[spring-projects-issues]

Mark Mielke commented

@AlanBateman : I might have misunderstood. But, after investigating several issues in our builds with Java 9, one path lead here, and several paths lead to needing -add-open, where the same build and code runs without warnings on Java 8.

I'm trying to say that it isn't like people have spent years deciding exactly where the module boundaries should be and they are definitely where they should be. The module boundaries have been approximate up until now, because there was no way to enforce them prior. With Java 9, they are now enforced, and this causes a requirement for re-evaluation on both consumer and producer side, and it will take some time to reach a stable state again where everybody agrees on where the boundaries should be.

Back to your statement:

If the developer of a library chooses to encapsulate its internals then that should be respected. If Spring is dependent on breaking into that library then it should work with the maintainer of the library to come up with a solution.

I don't think most people have carefully and meticulous chose with full awareness of impact. Java 9 is a surprise for most people still - both producer and consumer. And even if they did spend 5 minutes thinking about it when they published their module info, I don't think we're at a final resting point here where what the producer says is what goes. We're still at evaluating impact phase. :-(

Anyways... I don't mean to distract from the CGLIB discussion. I just want to make sure we're not presuming some deeper well-developed thought on the part of the producers here. We all have a problem to solve here, and that is understanding how Java modules affect us now that they are finally here.

[spring-projects-issues]

Rafael Winterhalter commented

I experienced the same problem with Mockito and the prototype I came up with requires users to register a lookup in a global mockito registry or falls back to a public lookup if no such instance is available. This is of course rather unsatisfactory as it requires more work to do less and we decidedto retain use of Unsafe, also since we need Objenesis for instantiation.

Another issue comes with load time weaving where one often needs to define helper classes what can be done with lookups only when the classes are defined by the instrumented class as the Java agent is not provided a lookup for the instrumented class's package what I could image is also a problem for Spring when being used with AspectJ.