-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Java 17 support #1536
Comments
(Related: #1529) |
Yes, this should be fixed by #1529 (I just need to add some documentation around the new defining process) |
Hey @mcculls The superclass access check is failing on the proxy class, problem doesn't exist in a lower JDK (except 12, where it popped up and vanished in JDK 13) Previous behaviour that works
Required changes for JDK 17 (because of superclass access check)
Exception
Dropping a JDK version and the issue goes away xD
|
Forgot to mention - When doing this, the unsafe access exception disappears and this one pops up |
@GedMarc can you share an example project or test that recreates the exception? |
Here we are :- profile "raw" for replicating on automatic module names, profile "modular" for executions on pure modules (for jlink/jpackage distributions) Modular results :-
Raw execution Results
|
Thanks, so the issue with the module example is that recent JDKs disallow access to For the pure module case adding For the automatic module case you can either add I'll have a think about how best to do this in the build (which will probably involve making it a pure module) |
Hmm I can't confirm that, I've added the requires clause to both the core as a pure module, and the consumer as an automatic module, the same exception - (Only in 17) - also to command line
Removing the added exports and adding in the requires still ends with the same result I'm attaching the jar(zip) with the module-info updated to include jdk.unsupported, |
The module shade/build is configured here (maybe to look at to assist not sure) - https://github.com/GedMarc/GuicedEE-Services/tree/master/guice-core |
i haven't sorry! let me do that quick |
@mcculls Purring like a kitten sir! the requires clause only necessary on the guice module, otherwise everything looks good -
Happy panda :) You're the man! |
putting the modular jar here with my module-info just incase a multi release jar becomes a thing (doesn't include error prone annotations) |
@mcculls is there any plan to make a release that includes recent JDK17 related improvements in the near future? |
@GedMarc is there a way to use just Gradles without module-info? |
@vjh0107 I'm not entirely sure why you would use a java version above 8 without a module-info, that's quite a performance degradation running a modular VM in classpath mode - you maybe want to think about staying on 8 if that is the path you are thinking of taking - Essentially though, that isn't a viable path so I'm not sure I'm actually going to try You can take the guice artifact and install it into your local repository, removing the module-info file, you can also create a shade that excludes the module-info file from the artifact, |
Hi @GedMarc, I'm curious about this performance degradation you mentioned: do you have more details as to how using a modularized jar on the classpath degrades performance? Thanks, |
@norrisjeremy AMD is now a standard across all languages and has filtered down to Java, so aside from all the obvious reasons on why that is and all those benefits - I'm going to try list everything off the top of my head and mention what it replaces in java for the same result, just native.
So good news, you're on Guice which is already a modular DI, is not externally configured, and defines injections properly - was just way ahead of its time. Just got to remember to deploy JRE's, and build them accordingly - JDK in production, especially now, still frowned upon I've found no less than a 60% boot up performance time in native runtimes, modules provide exceedingly great memory management to anything else, and not needing to bundle an entire damn framework to do microservicing - well, also it's AMD so all of those reasons why as well. Opinionated, but proven. No one yet has done an article on the performance differences between enterprise applications in native runtimes vs classpath management. |
Hi @GedMarc, I'm still not sure I fully understand your earlier statement discouraging usage of Java9+ with the classpath instead of modulepath: I don't believe I can recall having seen any direct statements from Oracle officially deprecating the usage of the classpath in-lieu of the modulepath, but perhaps I have missed something? Thanks, |
@GedMarc thank you |
Would it be possible to publish a new Guice release that includes #1529? I wasn't able to get our services to start up on Java 17 without those changes |
Can we pretty please get a new release out? 🐈 |
Can we put out a new release of Guice? |
@markmarch @ad-fu @java-team-github-bot Sorry for dropping yet another comment, but I hope the explicit mentions here bring in some progress. |
We are working on a new release but there are currently some test failures when running with Java 17. @mcculls If you have time, can you look into some of the test failures since you are most familiar with the bytecode gen stuff and those tests failures all seem to be related to that? With bazel installed, you can reproduce the issue with the following command from the root of the repo:
I'll also spend some time looking into this. The original plan is to migrate from maven to bazel then create a new release that also include some of the kotlin work we've done that hasn't been released but I probably will just cut a new release once the test failures are fixed and punt the bazel and kotlin work to next release. |
Hi @markmarch some of those failing tests rely on parsing stack traces to verify what mechanism was used to call the method. Java17 hides certain types of frames by default, particularly frames relating to anonymous nest-mates, so you need to add https://github.com/google/guice/blob/master/core/pom.xml#L104 Note this is just for test verification purposes - in everyday use you wouldn't worry about not seeing these frames in stack traces. EDIT: also the assisted-inject failure in
EDIT: lastly, the JPA failures look related to the version of Mockito used, upgrading Mockito should fix those. |
…1536 (comment) - skip reflection fallback test that only works for java version < 17 - add required jvm flags to mirror maven setup - upgrade mockito version PiperOrigin-RevId: 423168879
…1536 (comment) - skip reflection fallback test that only works for java version < 17 - add required jvm flags to mirror maven setup - upgrade mockito version PiperOrigin-RevId: 423389524
…ase. closes #1536 PiperOrigin-RevId: 423924393
The rececently released Java 17 removed Unsafe.defineAnonymousClass(): https://bugs.openjdk.java.net/browse/JDK-8267178
In effect Guice will use ChildClassDefiner where it previously used UnsafeClassDefiner, and will then fail when generating its proxy classes for non-public classes e.g. using @Inject on their constructor, with an error like this:
class XYZ$$EnhancerByGuice$$575108993 cannot access its superclass XYZ (XYZ$$EnhancerByGuice$$575108993 is in unnamed module of loader ChildClassDefiner$ChildLoader @72acb566; XYZ is in unnamed module of loader 'app')
If you trace down where this error comes from in the JDK, the inter-module boundaries are not even checked if the superclass is not public, so there is no chance of fixing this by adding module exports or opens to the codebase that is using Guice. Apparently Hidden Classes are the suggested replacement for Unsafe.defineAnonymousClass:
https://openjdk.java.net/jeps/371
The text was updated successfully, but these errors were encountered: