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

Fixes #5475 Scan for updated ASM version #5479

Merged
merged 3 commits into from Oct 20, 2020

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Oct 20, 2020

Scan for the highest known ASM version so we can always run on latest JVM if the ASM jar is updated. #5475

Scan for the highest known ASM version so we can always run on latest JVM if the ASM jar is updated.
@gregw gregw requested a review from sbordet October 20, 2020 11:30
@gregw
Copy link
Contributor Author

gregw commented Oct 20, 2020

@sbordet Can you review this one... @janbartel and I have gone over and over this and think this is the best way to find the highest known ASM version.

@gregw gregw requested a review from joakime October 20, 2020 11:32
@gregw gregw added this to In progress in Jetty 9.4.33 via automation Oct 20, 2020
@gregw gregw moved this from In progress to Review in progress in Jetty 9.4.33 Oct 20, 2020
@joakime
Copy link
Contributor

joakime commented Oct 20, 2020

What happens (with this change) in the following scenarios ...

Scenario 1.

  • Jetty 9.4.33
  • ASM 5.0 (with support for Java 8 bytecode)
  • Running on AdoptOpenJDK 8u262
  • War deployed.
  • Dependency in WEB-INF/lib/foo.jar has module-info.class compiled against Java 11.

Expectation: this should work, no issue/failure.

Scenario 2.

  • Jetty 9.4.33
  • ASM 5.0 (with support for Java 8 bytecode)
  • Running on AdoptOpenJDK 8u262
  • War deployed.
  • Dependency in WEB-INF/lib/bar.jar has META-INF/versions/11/Bar.class compiled against Java 11.

Expectation: this should work, no issue/failure.

Scenario 3.

  • Jetty 9.4.33
  • ASM 9.0 (with support for Java 16 support) - but no manifest (like in a badly made uber-jar)
  • Running on AdoptOpenJDK 8u262
  • War deployed.
  • Dependency in WEB-INF/lib/bar.jar has META-INF/versions/11/Bar.class compiled against Java 11.

Expectation: this should work, no issues/failure.

Scenario 4.

  • Jetty 9.4.33
  • ASM 9.0 (with support for Java 16 support) - but no manifest (like in a badly made uber-jar)
  • Running on AdoptOpenJDK 11
  • War deployed.
  • Dependency in WEB-INF/lib/baz.jar is compiled against Java 11.

Expectation: this should work, no issues/failure.

Scenario 5.

  • Jetty 9.4.33
  • ASM 9.0 (with support for Java 16 support) - but no manifest (like in a badly made uber-jar)
  • Running on AdoptOpenJDK 16
  • War deployed.
  • Dependency in WEB-INF/lib/baz.jar is compiled against Java 16.

Expectation: this should work, no issues/failure.

I can't put my finger on it (yet), but I think this PR would cause Scenario 4 and 5 to fail.

@gregw
Copy link
Contributor Author

gregw commented Oct 20, 2020

What happens (with this change) in the following scenarios ...

Scenario 1.

  • Jetty 9.4.33
  • ASM 5.0 (with support for Java 8 bytecode)
  • Running on AdoptOpenJDK 8u262
  • War deployed.
  • Dependency in WEB-INF/lib/foo.jar has module-info.class compiled against Java 11.

Expectation: this should work, no issue/failure.

I think ASM 5 might have some problems with the java 11 module-info class? Why do you expect an old ASM to be able to handle a new language feature?

I'm not sure your scenarios are the right way to break this down.... let me post another comment with all the scenarios I think are relevant.

I'm assuming that module-info.class should not be scanned, either because it's a global exclusion on all jar scanning in our code, or because the Runtime JVM dictates that we should ignore that file.

@gregw
Copy link
Contributor Author

gregw commented Oct 20, 2020

At any given time in the future, we will be using ASM version X. We will write our ClassVisitor against version X and the known language features at that stage. We then have the following permutations can combinations:

Normal deployment is ASM X jar with ASM X ClassVisitor and all webapp code using features known to ASM X. We discover version X, all is good.

If a webapp is deployed using features from a JVM not known to X, then we discover X but ASM will fail because it doesn't know the new features.

If the version of ASM is then updated to X+n that does know those new features, then we discover X+n, ASM handles the new features and calls the abstract visitor, which handles them as NOOPs

If a webapp is deployed that only uses featues known X-n with ASM X-n, then so long as our ClassVisitor does not implement any methods not supported by X-n, then we are good as we discover X-n and all is good.

The key think is that there is not a scenario where we can get either of the two problematic exceptions:

  • UnsupportedException is thrown if a new feature is discovered but we have not declared a version that supports it. So long as a version of ASM that supports the feature is deployed, we will discover its version and the feature will be a noop rather than an UE. Actually, since we start our scan at version 7, we can get this if deployed with ASM < 7, but that is not something that we need to support.
  • UnknownVersionException is thrown if we pass an unknown version. But we will only ever pass a discovered version (there is no default), so we should never get this exception.

…st ASM Opcodes.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Jetty 9.4.33 automation moved this from Review in progress to Reviewer approved Oct 20, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime merged commit 4ecaf30 into jetty-9.4.x Oct 20, 2020
Jetty 9.4.33 automation moved this from Reviewer approved to Done Oct 20, 2020
@joakime joakime deleted the jetty-9.4.x-5475-ScanForAsmVersion branch October 20, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Jetty 9.4.33
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants