RUM-17619: Fix deadlock/ANR in DatadogRumMonitor.handleEvent on JVM crash - #3671
RUM-17619: Fix deadlock/ANR in DatadogRumMonitor.handleEvent on JVM crash#3671hamorillo wants to merge 3 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![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: 384f1090c0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val writeContext = sdkCore.getFeature(Feature.RUM_FEATURE_NAME) | ||
| ?.getWriteContextSync(withFeatureContexts = setOf(Feature.SESSION_REPLAY_FEATURE_NAME)) | ||
| if (writeContext != null) { | ||
| val (datadogContext, eventWriteScope) = writeContext | ||
| synchronized(rootScope) { |
There was a problem hiding this comment.
When another thread submits a RUM event after getWriteContextSync completes but before the crashing thread reaches this synchronized block, the context executor can run that event's callback and its pipeline task can acquire rootScope first. A concurrent startView or stopView can therefore mutate the active view before the fatal error is handled, attributing the crash to the wrong view; previously the lock was already held throughout this interval. The context fetch and crash dispatch need to preserve their position in the normal-event ordering rather than leaving this overtaking window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I would say that this concern is partially valid, moving getWriteContextSync outside the lock does introduce a small window where a concurrent startView/stopView from another thread could execute in rootScope before the crash event, attributing the crash to the wrong view.
We considered another approach: route the crash through the normal event pipeline, withWriteContext (context thread) → executorService (pipeline thread) → synchronized(rootScope) — and block the crash thread on a CountDownLatch until the crash is written. This eliminates the race entirely since the crash is serialized through the same queues as all other events.
However, this option introduces a regression for crash reliability: the RUM pipeline executor is a BackPressureExecutorService that can drop tasks when its queue is full. Also adds a second executor hop (context → pipeline) before the crash is written, increasing the window where the OS can kill the process before the event is persisted.
I think the current fix is acceptable as the the race window should be narrow. The consequence is a wrong view ID on the crash event, not a lost crash.
I would like to see other opinions here.
There was a problem hiding this comment.
I'm not sure that it is the best approach, because when this happens it will skew the data, crash will be attributed to the wrong view and this may impact dashboards/error tracking.
Maybe we can rethink synchronised(rootScope) part? Maybe there is something more granular/deadlock aware?
There was a problem hiding this comment.
I've added a new commit with a different approach. Instead of calling synchronized(rootScope) directly on the crash thread, the crash task is now injected into the RUM pipeline queue via BlockingQueue.put() and executed on the pipeline thread — the same thread that handles all rootScope mutations. This eliminates the race entirely. The crash thread waits on a CountDownLatch until the task completes, with a timeout fallback that runs the task directly on the crash thread if the pipeline doesn't respond in time.
There was a problem hiding this comment.
I added a commit with a slightly different approach.
Currently in develop branch we have getWriteContextSync method that schedules a task onto the context thread, waits for it in a blocking way and returns datadogContext+eventWriteScope.
I renamed getWriteContextSync -> withWriteContextSync. Now it accepts a callback that gets executed on context thread. Also moved synchronized(rootScope inside this callback in DatadogRumMonitor.handleEvent.
This way we:
- Solve the initial problem with the deadlock.
- Don't have the problem raised by codex in this thread.
@0xnm WDYT?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3671 +/- ##
===========================================
- Coverage 73.46% 73.43% -0.02%
===========================================
Files 997 997
Lines 36445 36446 +1
Branches 6166 6167 +1
===========================================
- Hits 26771 26764 -7
- Misses 7968 7970 +2
- Partials 1706 1712 +6
🚀 New features to boost your workflow:
|
…rash On the fatal-crash path, getWriteContextSync was called while holding synchronized(rootScope), inverting the lock-acquisition order of the normal-event path (context-thread → pipeline-thread → rootScope). This created a 3-way circular wait between the crashing thread, the context thread, and the RUM pipeline thread, causing an ANR and losing the crash RUM event. Fix: fetch the write context before acquiring the rootScope lock so the ordering matches the normal path. The lock is now only held for the fast in-memory operations (rootScope.handleEvent + updateFeatureContext). Also removes the two stale // TODO RUM-9852 comments, adds a regression test asserting getWriteContextSync is not called while the rootScope monitor is held, and fixes a pre-existing missing timeProvider stub in RumViewManagerScopeTest that was exposed by the new test shifting the Forge seed sequence.
384f109 to
54c7c20
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54c7c207f1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| if (pipelineQueue != null) { | ||
| @Suppress("UnsafeThirdPartyFunctionCall") // InterruptedException is caught below | ||
| pipelineQueue.put(crashTask) |
There was a problem hiding this comment.
When the pipeline queue is full, this put() can block the crashing thread indefinitely before the 500 ms latch timeout is reached. For example, with a small custom backpressure capacity, a user callback from getCurrentSessionId() can block the sole pipeline worker while another request fills the queue; a subsequent JVM crash then never reaches the previous uncaught-exception handler and the process remains hung. Use a bounded insertion or another mechanism that guarantees the crash path itself cannot wait forever.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think that case is quite unlike. This scenario requires the pipeline queue to be completely full (1024 tasks by default with IGNORE_NEWEST back-pressure) AND completely stalled. In practice this is extremely unlikely since the pipeline thread drains continuously.
…ribution Inject the crash task into the RUM pipeline queue via BlockingQueue.put() instead of calling synchronized(rootScope) directly on the crash thread. put() bypasses the IGNORE_NEWEST back-pressure drop policy so the crash task is never silently discarded. The crash thread waits on a CountDownLatch until the pipeline thread has processed the task, ensuring the I/O thread receives the write operation before the process terminates. InterruptedException from put() and await() is caught, logged, and the interrupted flag restored.
54c7c20 to
0391b8c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0391b8c54e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { CANNOT_WRITE_CRASH_WRITE_CONTEXT_IS_NOT_AVAILABLE } | ||
| { CRASH_REPORTING_TIMEOUT_WARNING } | ||
| ) | ||
| crashTask.run() |
There was a problem hiding this comment.
When the 500 ms timeout expires, this runs the same crashTask that remains in pipelineQueue; the pipeline worker can therefore execute it later as well. If older queued events stop or start a view between these executions, the first invocation overtakes them and attributes the crash to the old view, while the queued invocation can emit the fatal event again against the new view. The new evidence in this revision is this direct fallback without cancelling or removing the already-enqueued task.
Useful? React with 👍 / 👎.
| } catch (e: InterruptedException) { | ||
| sdkCore.internalLogger.log( | ||
| InternalLogger.Level.WARN, | ||
| InternalLogger.Target.USER, | ||
| { CRASH_REPORTING_INTERRUPTED_WARNING }, | ||
| e | ||
| ) | ||
| @Suppress("UnsafeThirdPartyFunctionCall") // SecurityException not expected here | ||
| Thread.currentThread().interrupt() |
There was a problem hiding this comment.
If the crashing thread is interrupted during put() or while awaiting the latch, this catch only restores the interrupt flag and returns to the uncaught-exception handler. In the former case the crash task may never have been enqueued, and in the latter case the process can be terminated before the queued task runs, so the fatal RUM event is lost even though the write context was obtained successfully. This path needs a bounded best-effort write rather than abandoning crash handling on interruption.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
4ee0dd5 to
d11056e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
d11056e to
1dd2e20
Compare
5 participants
What does this PR do?
Fixes a deadlock/ANR that occurs when a JVM crash is reported while a normal RUM event is in-flight (#3652).
On the fatal-crash path,
getWriteContextSyncwas called while holdingsynchronized(rootScope), inverting the lock-acquisition order of the normal-event path (context-thread → pipeline-thread → synchronized(rootScope)). This created a 3-way circular wait:main(crashing)rootScopemonitorgetWriteContextSynctaskdatadog-context-thread-1datadog-rum-pipeline-thread-1rootScopemonitor held bymainmain → context-thread → pipeline-thread → main. BecausegetSafeuses an untimedFuture.get(),mainhangs indefinitely → ANR, and the crash event is never written.Fix: fetch the write context first (no lock held), then inject the crash task into the RUM pipeline queue via
BlockingQueue.put()rather than callingsynchronized(rootScope)directly on the crash thread.put()bypasses theIGNORE_NEWESTback-pressure drop policy — unlikesubmit()/offer(), it blocks until a slot is available and never discards. The crash is serialized after any events already in the pipeline, ensuring correct view attribution. The crash thread waits on aCountDownLatchuntil the pipeline thread completes the task, so the I/O thread receives the write operation before the process terminates.Motivation
Reported publicly as GitHub issue #3652. Present in 3.10.0 through 3.12.1 and
develop. The deadlock is not a rare race — it only requires a normal RUM event to be in-flight at the moment of the crash, which is common on any active app.Additional Notes
Also removes two stale
// TODO RUM-9852comments (that ticket is Dropped).Review checklist (to be filled by reviewers)