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.

Sign up for GitHub

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

Jump to bottom

Work around jvm crash on early 1.8 #4345

Merged
merged 5 commits into from
Nov 30, 2021
Merged

Work around jvm crash on early 1.8 #4345

merged 5 commits into from
Nov 30, 2021

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Oct 11, 2021

Resolves #4326
This pr implements a workaround for jvm crashes on oracle jvms earlier than 8u40, an alternative would be to disable agent for these jvm versions. This workaround avoids using lambdas in agent premain method by inserting a callback into sun.launcher.LauncherHelper.checkAndLoadMain so we can run our agent startup right before main where using lambdas doesn't cause issues.

trask
trask reviewed Oct 11, 2021
View reviewed changes
Copy link
Member

@trask trask left a comment •
edited
Loading

Choose a reason for hiding this comment

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

any thoughts how we can add smoke test for an early 1.8 release?

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentStarterImpl.java Outdated
Comment on lines 43 to 49
instrumentation.retransformClasses(c);
return transformer.hookInserted;
Copy link
Member

Choose a reason for hiding this comment

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

is it worth checking hookInserted before calling retransformClasses in case Class.forName triggered the first load? or does that not happen in practice...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'd expect it to be not loaded before the transformer is added and retransformClasses is just a failsafe if it was unexpectedly already loaded.

@iNikem
Copy link
Contributor

iNikem commented Oct 12, 2021

I vote for NOT supporting anything before 8u40 and thus NOT introducing extra complexity.

@trask
Copy link
Member

trask commented Oct 12, 2021

I vote for NOT supporting anything before 8u40 and thus NOT introducing extra complexity.

I'm in favor of supporting pre-8u40 if we have a way to test. Delaying startup to main could be useful for other reasons, e.g. JIT compiler is not active across all Java 8 versions during premain, and native images need to delay at least a portion of startup to main.

Let's at least hold on this until after the 1.7.0 release to give us more time to think/discuss.

@iNikem
Copy link
Contributor

iNikem commented Nov 1, 2021

Let's at least hold on this until after the 1.7.0 release to give us more time to think/discuss.

@trask do you have specific action plan/proposal for this?

@trask
Copy link
Member

trask commented Nov 3, 2021

@trask do you have specific action plan/proposal for this?

I think the added complexity is justified if we have a test for it, otherwise it feels like something that's too easy to break in the future. There are some impressively old Zulu docker images (e.g. 8u05) that could fit the bill.

@laurit laurit force-pushed the delay-agent-start-on-early-18 branch from b59e91c to 452fd73 Compare November 9, 2021 12:02
trask
trask approved these changes Nov 9, 2021
View reviewed changes
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

@open-telemetry/java-instrumentation-approvers please weigh in

smoke-tests/src/test/groovy/io/opentelemetry/smoketest/CrashEarlyJdk8Test.groovy
Comment on lines +19 to +21
// Hotspot versions before 8u40 crash in jit compiled lambdas when javaagent initializes
// java.lang.invoke.CallSite
// This test verifies that such jvm does not crash with opentelemetry agent
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@iNikem
Copy link
Contributor

iNikem commented Nov 22, 2021

@anuraaga @jkwatson do you have an opinion about this?

@jkwatson
Copy link
Contributor

@anuraaga @jkwatson do you have an opinion about this?

Only that I think we shouldn't be doing it. Supporting extremely old, buggy versions of the JDK is time that could be better spent. No opinion other than that.

@iNikem
Copy link
Contributor

iNikem commented Nov 28, 2021

3 votes in favour, 2 votes against. @laurit seems like you are free to merge :(

@trask trask merged commit 10288c6 into open-telemetry:main Nov 30, 2021
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
@laurit @RashmiRam
Work around jvm crash on early 1.8 (open-telemetry#4345)
11f0ef0 
* Work around jvm crash on early 1.8

* skip retransform if class was already transformed during load

* fix imports after rebase

* add test

* disable test on windows
@laurit laurit deleted the delay-agent-start-on-early-18 branch July 6, 2023 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anuraaga anuraaga anuraaga approved these changes

@trask trask trask approved these changes

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek approved these changes

@iNikem iNikem Awaiting requested review from iNikem

@jkwatson jkwatson Awaiting requested review from jkwatson

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Java 8u31 and earlier crash with Java agent
6 participants
@laurit @iNikem @trask @jkwatson @anuraaga @mateuszrzeszutek