Proxy creation improvements for JPMS, remove the need for --add-opens in some cases #25132
Conversation
The best way to define a class is to define using an anchor class, which defines its classloader and security policies, and is the only way to define a class in Java 9+ without --add-opens on the command line. The package name of the anchor class must be the same as the new class. If the conditions are not met, we fall back to calling private methods on the ClassLoader to define classes. This needs --add-opens, otherwise a exception is raised. Moved global references to the private ClassLoader methods to a separate ClassLoaderMethods class. This means that the global references will be initialized lazily only when needed, to avoid getting an exception on accessing the private methods (if not --add-opens, e.g. in GF Embedded) if they are not needed.
Weld specifies a different package name for proxies than for the original class. We change the name of the proxy class using Javassist to keep the original package name so that we can use the Lookup method. We keep separate Javassist class pools for separate application classloaders, so that applications don't share them.
Discovered a flaw in the solution for ProxyServiceImpl
OndroMih
force-pushed
the
ondromih-jpms-defineClass
branch
from
f507d40 to
3e85563
Compare
|
Hi @dmatej, @avpinchuk, @pzygielo, could you please review this and tell me whether this is a viable approach to make the code compatible with JPMS? |
|
I did not any experiments yet how GF behaves with your changes (but will do soon ;) ). But I have a question. What happens when we redeploy application many times? Generated classes unloaded or stay loaded (as a garbage, actually) in the bootstrap or server CL? |
|
My understanding is that the ideal goal here is to eliminate the need for However, as it stands, there are still many places relying on ClassLoader.defineClass. So, I see this PR introduces a pragmatic compromise with lazy loading. This is smart, as it means that if ClassLoader.defineClass isn't actually triggered at runtime, we won't immediately hit errors even without --add-opens. It's a nice improvement in that users might not need --add-opens from the get-go, and only add it if they actually run into problems. My question is, is the eventual plan to migrate all usages of ClassLoader.defineClass to lookup.defineClass? Or are there scenarios where switching to lookup.defineClass isn't feasible? Just curious about the roadmap here. Anyway, this looks like valuable progress. |
|
Hi @mz1999 ,
I don’t know. Currently, there are some failed tests related to remote EJBs, which our team at OmniFish haven’t time to look into. This is more like a proof of concept than a plan. We try to replace usages of ClassLoader.defineClass with lookup.defineClass continually, with other improvements. This PR is an effort to replace rhem everywhere but it’s not completed yet. The problem is that ClassLoader.defineClass, as it is used in many places in GlassFish, has different semantics compared to lookup.defineClass. In most cases, it defines a class in a different package than the original class it wraps. This is not allowed with lookup.defineClass, which requires that the new class is in the same package. And this is probably what dails the remote EJB tests. This also changes packages of CDI proxy classes. We need to investigate what is the impact of the package change and how to fix all issues with it. |
|
Thanks for the update! Sounds like a big job, lots to do. I'd be happy to contribute and have some time available to assist if you think it would be helpful. About the failed remote EJB tests, are they unit tests or TCK? Knowing that would help me try to reproduce it locally and investigate. |
Well, they are integration tests in the GlassFish project. You can run the failed test in the GlassFish source root with: To build GlassFish without running tests: To build common test artifacts: To run the failed test: |
|
I ran the test case public static Object lookupRemote30BusinessObject(Object jndiObj, String businessInterface) throws NamingException {
...
Class<?> genericEJBHome = loadGeneratedGenericEJBHomeClass(loader, jndiObj.getClass());
final Object genericHomeObj = PortableRemoteObject.narrow(jndiObj, genericEJBHome);
...
}Let's briefly analyze how this
The root cause is that Based on this error, the idea for a fix is to change the In the public static Object lookupRemote30BusinessObject(Object jndiObj, String businessInterface) throws NamingException {
Class<?> businessInterfaceClass = loadGeneratedRemoteBusinessClasses(businessInterface);
...
Class<?> genericEJBHome = loadGeneratedGenericEJBHomeClass(loader, businessInterfaceClass);
final Object genericHomeObj = PortableRemoteObject.narrow(jndiObj, genericEJBHome);
...
}I've verified that this modification allows the test case to pass. However, the EJB part is quite complex, and I don't fully understand it. I'm just making guesses and modifications based on debugging information, so I'm not sure if this modification is entirely complete or correct. |
Raising as a draft, because this improves only a few places. Hopefully all tests pass and we can add more improvements gradually.
In short, use JPMS compatible MethodHandles.Lookup instead of calling ClassLoader.defineClass by reflection. This approach has some limitations. We need a class to be used as a template (the generated class will have the same classloader and protection domain derived from it). And the generated class must have the same package name as the template class.
For Weld proxies, I decided to generate a class with a different name than Weld requests, because the generated class must be in the same package as the anchor class and Weld requests a different package name for the generated classes. It looks like Weld doesn't care and all worked on my sample app with EJBs.
Ideally, all methods in
ClassGeneratorthat I marked as deprecated, will be removed and callers will be refactored to use the non-deprecated methods. The deprecated methods use reflection incompatible with JPMS, the other ones use the JPMS compatible MethodHandles. There are still a lot of places, though, where the deprecated methods are used, so it will take some time.Yet to refactor:
ProxyServicesImpl.classPoolMapshould be moved to a global service rather than be a static field. It must be global becauseProxyServicesImplis created for each app deployment, while the classloader where the classes are generated, is shared. So the cache needs to be preserved, otherwise we'd attempt to create a new class that already exists in the classloader.