Fix JFR startup deadlock without disabling built-in JFR events#11957
Conversation
PR #10096 attempted to preload jdk.jfr.events.Handlers via ClassLoader.loadClass() to prevent an AB-BA deadlock between the JFR Utils monitor and the Handlers class-initialization lock (JDK-8371889). However, ClassLoader.loadClass() only loads the class; it does not run the static initializer and does not acquire the class-initialization lock. Initialization is still deferred to first active use, inside the racing window, so the deadlock is not actually prevented. Use Class.forName(name, true, loader) instead, which forces <clinit> to run on this thread before the JFR code acquires the Utils monitor. Also widen the catch to Throwable, since forcing initialization can surface Errors (ExceptionInInitializerError, NoClassDefFoundError, LinkageError) that the previous catch (Exception) would not have handled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
dd-octo-sts
Bot
added
the
tag: ai generated
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The change correctly fixes the incomplete deadlock mitigation from PR #10096. The original fix using loadClass() only loaded the class but didn't initialize it, leaving the deadlock window open. This PR correctly uses Class.forName(..., true, ...) to force immediate initialization, and broadens exception handling from Exception to Throwable to catch Error subtypes that can occur during initialization. The code is well-commented, scope is minimal, and no behavioral regressions detected.
📊 Validated against 7 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 029c60c · What is Autotest? · Any feedback? Reach out in #autotest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 029c60ce0e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
🎯 Code Coverage (details) 🔗 Commit SHA: 0d01fd1 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
…gistration Two follow-ups from PR review: - The JFR event-holder class was renamed across JDK versions. It is jdk.jfr.events.Handlers only on JDK 17-18; on JDK 19-22 it is jdk.jfr.events.EventConfigurations (same Utils-based eager init, same deadlock); on JDK 23+ the pattern was removed. Targeting only Handlers meant the workaround silently no-op'd (ClassNotFoundException) on JDK 19-22, which are affected. Try both class names. - registerDeadlockDetectionEvent() also registers a JFR event and runs before registerSmapEntryEvent(). Hoist the holder-class initialization into startJmx() ahead of both registrations so it is always fully initialized before any JFR registration acquires the Utils monitor. Extracted the logic into initializeJfrEventHolderClass(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dlers Forcing the holder class's <clinit> (jdk.jfr.events.Handlers on JDK 17-18, EventConfigurations on 19-22) before the JDK's built-in events are registered caches null into its static-final handler fields permanently, silently disabling the built-in socket/file/exception JFR events. Empirically verified on JDK 17.0.17 and 21.0.9 (7/7 fields null when the holder is initialized before FlightRecorder; 0/7 when after). In the default profiling.start-force-first=false config the profiler defers FlightRecorder initialization until after startJmx(), so the previous placement would poison the handlers. Force FlightRecorder init (which runs JDKEvents.initialize()) before the holder <clinit>, mirroring the JDK's own JDK-8371889 fix which initializes the holder only after JDKEvents.initialize(). If FlightRecorder init fails there is nothing to protect, so the holder init is skipped. Collapsed the handling into a single catch(Throwable) with a narrow inner ClassNotFoundException for the Handlers -> EventConfigurations version fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rkennke
changed the title
Adds JfrEventHolderInitForkedTest, which runs the production initialization path (Agent.initializeJfrEventHolderClass) in a fresh JVM and asserts the JDK's built-in JFR event-holder fields are not poisoned to null. This guards the ordering invariant that FlightRecorder must be initialized (registering the built-in events) before the holder class's <clinit> runs; without it the static-final handler fields cache null permanently and silently disable socket/file/exception JFR events. Verified: passes on JDK 17 and 21 with the correct ordering, and fails when the ordering is reversed. Auto-skips on JDKs without the holder class (JDK 8 and 23+). Behavior underpinning the test was reproduced standalone on JDK 17.0.17 and 21.0.9 (7/7 fields null when initialized too early, 0/7 when after FlightRecorder). To make the logic testable, initializeJfrEventHolderClass() takes the ClassLoader as an argument (the production no-arg overload passes AGENT_CLASSLOADER). build.gradle adds --add-opens jdk.jfr/jdk.jfr.events=ALL-UNNAMED so the test can read the holder fields reflectively. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both registerDeadlockDetectionEvent() and registerSmapEntryEvent() shared the same reflective "invoke Factory.registerEvents()" pattern. Extract it into registerJfrEvents(), which force-initializes the JDK JFR event-holder class before registering anything. Routing all startup JFR registration through this one method enforces the ordering invariant structurally, so a future event registration cannot reintroduce the JDK-8371889 deadlock by running before the holder is initialized. The explicit pre-init call in startJmx() is now redundant and removed. Also unifies exception handling: NoClassDefFoundError/ClassNotFoundException/ UnsupportedClassVersionError -> "not supported" (debug); any other Throwable -> error. This is slightly more robust for the smap path, which previously caught only Exception and would have let an Error (e.g. NoClassDefFoundError) propagate out of startup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Correct the Handlers version range to JDK 15-18 (it is absent on 11-14), and note that JDK 11 LTS and earlier predate the holder class, so they are unaffected. - Move the explanatory Javadoc onto the initializeJfrEventHolderClass implementation (it was on the no-arg wrapper, while @link targets and the real logic are on the ClassLoader overload). - Reference the SCP-1278 deadlock mechanism (smap registration vs. a concurrent JMXFetch ByteBuddy transform) and note the event registration would trigger FlightRecorder init anyway; we only order it first. - Capitalize the smap log description so "Smap entry scraping not supported" reads correctly. - Document the regression test's scope: it checks the ordering invariant (non-null handlers), not end-to-end event emission, and assumes a fresh forked JVM that has not started JFR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per review, replace the exception-driven "try Handlers, catch ClassNotFoundException, try EventConfigurations" control flow with an explicit version check. New jfrEventHolderClassName() maps the running JDK to its holder class (Handlers on 15-18, EventConfigurations on 19-22, null otherwise) using JavaVirtualMachine.isJavaVersionAtLeast(), and is the single source of truth shared by the production init path and the regression test (which now uses it to decide the target class and skip JDKs without a holder). The remaining try/catch(Throwable) is genuine error handling (JFR disabled / init failure), not control flow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 820090a: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for cdb88ed: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
The build pipeline's test_base:[semeru17] job failed: on JDK 17 the version check selected jdk.jfr.events.Handlers, but Eclipse OpenJ9 / IBM Semeru ships a different JFR implementation where the holder / Utils handler mechanism does not behave like HotSpot, so the regression test's non-null-handler assertion did not hold. (semeru8/semeru11 passed because those versions have no holder and the test skips.) JDK-8371889 is a HotSpot JFR bug, so gate jfrEventHolderClassName() on JavaVirtualMachine.isHotspot(): it now returns null on non-HotSpot VMs, making the production init a no-op there and the regression test skip. Verified the test still runs (not skipped) and passes on HotSpot JDK 17. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
rkennke/fix-jfr-handlers-clinit-deadlock
branch
2 participants
What Does This Do
Fixes the JFR startup deadlock from #10096 for real, without disabling the JDK's built-in JFR events, and enforces the ordering structurally so it can't silently regress.
registerSmapEntryEvent()could deadlock against JFR class initialization (SCP-1278, upstream JDK-8371889). PR #10096 tried to pre-empt this by callingAGENT_CLASSLOADER.loadClass("jdk.jfr.events.Handlers")early. That doesn't work, for two independent reasons this PR corrects:loadClass()doesn't initialize the class. It loads the class but never runs its<clinit>, so it never acquires the class-initialization lock and never prevents the deadlock. Initialization stays deferred to first active use, inside the racing window.Class.forName(name, true, loader)is required to force<clinit>.jdk.jfr.events.Handlersonly exists on JDK 17-18. On JDK 19-22 the holder isjdk.jfr.events.EventConfigurations(same eagerUtils-based init, same deadlock), so the originalloadClassthrewClassNotFoundExceptionand silently no-op'd there.Motivation
The deadlock (ABBA)
The holder class's
<clinit>looks up the JDK's built-in event handlers throughjdk.jfr.internal.Utils, taking its monitor. In the SCP-1278 thread dump:dd-task-scheduler:registerSmapEntryEvent()→EventType.getEventType()holds theUtilsmonitor, then wants the holder's class-init lock.dd-jmx-collector: JMXFetch YAML parse → ByteBuddy transform is mid-<clinit>of the holder (holds the class-init lock), then wants theUtilsmonitor.Cross-wait → the holder init never completes → every thread that subsequently touches file I/O or classloading (including the application's
main) piles up on the same monitor.Forcing the holder's
<clinit>on a single controlled thread before we register our own JFR events removes the edge our own code contributed (holdingUtilswhile wanting the class-init lock). Once the holder is fully initialized, the later registration finds it already-initialized and needs no class-init lock, so the cycle cannot form — even with JMXFetch running concurrently.Why ordering matters twice
The fix has to sit inside a specific window, which the upstream JDK-8371889 fix makes explicit — it calls the new
EventConfigurations.initialize()betweenJDKEvents.initialize()andJDKEvents.addInstrumentation():After the JDK events are registered. The holder's fields are
static finaland are populated fromUtilsat<clinit>time.Utils.getHandler/getConfigurationreturnsnulluntil the events are registered (viaFlightRecorderinit →JDKEvents.initialize()). Running<clinit>too early cachesnullinto those fields permanently, silently disabling the built-in socket/file/exception JFR events.This matters here because with the default
profiling.start-force-first=false, the profiler defersFlightRecorderinitialization until afterstartJmx(), so a naive earlyforNamewould poison the handlers. Verified empirically on two unpatched JDKs:<clinit>beforeFlightRecorderFlightRecorderHandlersEventConfigurationsSo we force
FlightRecorderinitialization first (which runsJDKEvents.initialize()), then the holder<clinit>. IfFlightRecorderinit fails, JFR is unavailable and there is nothing to protect against, so the holder init is skipped.Before the concurrency. Forcing
<clinit>early, on this thread, before our JFR registrations take theUtilsmonitor.What This Changes
initializeJfrEventHolderClass(ClassLoader)forcesFlightRecorderinit, then the holder<clinit>, tryingjdk.jfr.events.Handlers(17-18) and falling back tojdk.jfr.events.EventConfigurations(19-22).registerDeadlockDetectionEvent()andregisterSmapEntryEvent()shared the same reflective "invokeFactory.registerEvents()" pattern; both now route through oneregisterJfrEvents(...)method that force-initializes the holder first. Routing all startup JFR registration through this one method enforces the ordering structurally — a future event registration cannot reintroduce the deadlock by running before the holder init. This also unifies exception handling (and hardens the smap path, which previously caught onlyExceptionand would have let anErrorescape startup).catch (Throwable)(a missing/renamed holder class or a failed init must never break startup); a narrow innercatch (ClassNotFoundException)only drives theHandlers→EventConfigurationsversion fallback.Throwablerather thanExceptionbecause forcing initialization can surfaceErrors (ExceptionInInitializerError,NoClassDefFoundError,LinkageError).Testing
Adds
JfrEventHolderInitForkedTest(JUnit 5, forked JVM): it runs the production init path in a fresh JVM and asserts none of the holder's handler fields were poisoned tonull. This locks in the FlightRecorder-before-holder ordering — dropping or reordering it fails the test.build.gradleadds--add-opens jdk.jfr/jdk.jfr.events=ALL-UNNAMEDso the test can read the holder fields reflectively.The deadlock itself is timing-dependent and is not reproduced as a test (a spin-loop repro would be flaky in CI); it is instead defended structurally by the choke point above, with the poisoning invariant covered by the test.
JDK Version Notes
The holder class was renamed/removed across versions:
jdk.jfr.events.Handlerson JDK 17-18,jdk.jfr.events.EventConfigurationson JDK 19-22. JDK 23+ removed the eager-init pattern entirely (the deadlock cannot occur), and the JDK-8371889 fix was backported to 21.0.11 — soEventConfigurationscoverage is a safety net for unpatched 21.0.0-21.0.10 and JDK 20. On JDKs without either class theforNamecalls simply fail and are ignored.The proper fix is the JDK-side lock-ordering change in JDK-8371889; this remains the workaround for unpatched JDKs.
Jira ticket: PROF-13025
🤖 Generated with Claude Code