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

Override IndyBootstrapDispatcher class module #1468

Merged

Conversation

eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Nov 2, 2020

What does this PR do?

Closes #1415
Supersedes #1465

Checklist

  • This is a bugfix
    • I have updated CHANGELOG.asciidoc
    • I have added tests that would fail without this fix - we currently only have integration tests for J9 with Java 8
    • I added a unit test for the module override functionality
    • I tested manually that the fix works on the adoptopenjdk/openjdk11-openj9:jdk-11.0.1.13-alpine-slim docker image with Spring PetClinic
    • I tested manually that the fix works on the adoptopenjdk/openjdk9-openj9:jdk-9.0.4.12_openj9-0.9.0-alpine docker image with Spring PetClinic

@apmmachine
Copy link
Contributor

apmmachine commented Nov 2, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #1468 updated]

  • Start Time: 2020-11-04T14:43:23.735+0000

  • Duration: 47 min 57 sec

Test stats 🧪

Test Results
Failed 0
Passed 1618
Skipped 12
Total 1630

Copy link
Member

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only a few minor comments/questions.

byte[] bootstrapClass = IOUtils.readToBytes(classFileAsStream);
ClassInjector.UsingUnsafe.ofBootLoader().injectRaw(Collections.singletonMap(IndyBootstrapDispatcherModuleSetter.class.getName(), bootstrapClass));

IndyBootstrap.setJavaBaseModule(IndyBootstrapTest.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] Would it be possible to have an exception thrown with an explicit message when this method is called and IndyBootstrapDispatcherModuleSetter hasn't been loaded from the right classloader ? If yes, that might help to diagnose any unexpected state in the field.

Copy link
Contributor Author

@eyalkoren eyalkoren Nov 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't happen - the setJavaBaseModule method tries to load it from the bootstrap CL explicitly, so failing to inject it there will result with ClassNotFoundException (which I think is explanatory enough).
As you can see here, I do load it with the test's class loader, but its single method is never invoked on any class that is not loaded correctly.

@eyalkoren eyalkoren merged commit 953f8dc into elastic:master Nov 5, 2020
@eyalkoren eyalkoren deleted the IndyBootstrapDispatcher-module-override branch November 5, 2020 07:18
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 this pull request may close these issues.

Fix support for Java modules
3 participants