Skip to content
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 16 incompatibility due to usage of internal APIs #423

Closed
aahlenst opened this issue May 4, 2021 · 7 comments · Fixed by #427
Closed

Java 16 incompatibility due to usage of internal APIs #423

aahlenst opened this issue May 4, 2021 · 7 comments · Fixed by #427

Comments

@aahlenst
Copy link

aahlenst commented May 4, 2021

The README promises that "Playwright requires Java 8 or newer". This does not include Java 16, however. With JEP 396, further APIs were encapsulated that are being used by Playwright. As a result, running the test suite with Java 16 fails. Examples:

[ERROR] com.microsoft.playwright.TestScreencast.shouldExposeVideoPath(Path)  Time elapsed: 0.096 s  <<< ERROR!
java.lang.reflect.InaccessibleObjectException: Unable to make field private final sun.nio.fs.UnixFileSystem sun.nio.fs.UnixPath.fs accessible: module java.base does not "opens sun.nio.fs" to unnamed module @152f2773
	at com.microsoft.playwright.TestScreencast.shouldExposeVideoPath(TestScreencast.java:30)

...


[ERROR] com.microsoft.playwright.TestBrowserContextAddCookies.shouldRoundtripCookie  Time elapsed: 0.103 s  <<< ERROR!
java.lang.reflect.InaccessibleObjectException: Unable to make private java.util.Collections$EmptyList() accessible: module java.base does not "opens java.util" to unnamed module @152f2773
	at com.microsoft.playwright.TestBrowserContextAddCookies.shouldRoundtripCookie(TestBrowserContextAddCookies.java:53)

Steps to reproduce:

  1. Install any version of OpenJDK 16 (I'm using Liberica by Bell Soft)
  2. Run mvn test

I know about workarounds like --add-opens. But it would be great if Playwright worked without them. Because the OpenJDK team plans to enable further restrictions in the future, it might make sense to test against the newest JDK as part of each CI run.

@yury-s
Copy link
Member

yury-s commented May 4, 2021

Source code is compatible with any java version 8+. If you want to run it in a JDK that has modules, you may need to configure your project accordingly. We currently run our tests with JDK 11 and it works perfectly fines. There is some peculiarity with JDK16 which makes them fail with the odd (given that we don't access neither sun.nio.fs nor other private APIs directly) error which could be overcome by adding the following argument to the surefire plugin:

          <argLine>
            --add-opens java.base/java.util=ALL-UNNAMED
          </argLine>

I created #425 which adds the argument to our pom.xml which seems harmless for earlier versions too (actually tried only with 11 which has modules support).

Update: it does fail with java 8 as it does not recognize --add-opens command line flag. I've closed the PR for now as we are not planning to switch from 8 to a newer version for the tests for now but if you want to test it on your machine with a newer java you can set that flag. I'm sure this could also be solved with different profiles for different java targets but it doesn't seem to provide much value for our clients since the tests are expected to be run only by the playwright team.

I also tried running our examples on JDK 16 and I had to run them as MAVEN_OPTS='--add-opens java.base/java.util=ALL-UNNAMED' mvn exec:java -e -Dexec.mainClass=org.example.PrintTitle to suppress the error. Are you experience problems when running your Playwright client or only when running our unit tests?

@yury-s
Copy link
Member

yury-s commented May 5, 2021

Ok, good news is the problem boils down to this bug in Gson library, the error message you get is a bit misleading but it can be reproduce with the following very simple code (just insert it into any test):

new Gson().toJsonTree(Collections.emptyList());

Bad news is that there are no signs of interest from Gson maintainers in fixing this any time soon.

@aahlenst
Copy link
Author

aahlenst commented May 5, 2021

Are you experience problems when running your Playwright client or only when running our unit tests?

I had this problem when introducing Playwright into a new (non-modularized) project I'm working on. Running the tests was the easiest way to reproduce the problem for the report.

Bad news is that there are no signs of interest from Gson maintainers in fixing this any time soon.

One of the former authors of Gson developed a replacement of Gson called Moshi with less baggage. In https://www.reddit.com/r/androiddev/comments/684flw/why_use_moshi_over_gson/dgx3gpm/ he explains the differences himself.

@yury-s
Copy link
Member

yury-s commented May 5, 2021

One of the former authors of Gson developed a replacement of Gson called Moshi with less baggage. In https://www.reddit.com/r/androiddev/comments/684flw/why_use_moshi_over_gson/dgx3gpm/ he explains the differences himself.

Yeah, Moshi was my consideration after reading the issues in Gson repo. So far Gson has been serving us pretty well but if the issues pile up Moshi sounds like a good alternative, we need to evaluate whether switching to it right now is worth the effort.

@yury-s
Copy link
Member

yury-s commented May 6, 2021

Moshi looks pretty good but the migration is not as easy as I originally thought, in particular it is missing intermediate model similar to JsonElement in Gson which means that we'd have to come up with a replacement for that (bare Map and List would be to clunky in the code due to type casts). For now I'm going to land a fix that avoids access to private JDK classes. Our usage of those is very limited actually so it should be sustainable.

If more problems like this arise in the future we can reconsider switching to Moshi or something else, for now it doesn't seem justified given that we have a workaround for this problem.

@yury-s
Copy link
Member

yury-s commented May 6, 2021

@aahlenst The fix is now available in 1.11.0-SNAPSHOT, can you give it a try?

@aahlenst
Copy link
Author

aahlenst commented May 6, 2021

Works great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants