Avoid calling synchronized wrappers when capturing - #12106
Conversation
|
Bits Code status: ✅ Done Comment @DataDog to request changes |
|
I can only run on private repositories. |
|
🎯 Code Coverage (details) 🔗 Commit SHA: 80f278e | Docs | Datadog PR Page | Give us feedback! |
Debugger benchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 6 unstable metrics. See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (336.83 µs) : 305, 369
. : milestone, 337,
basic (296.479 µs) : 290, 303
. : milestone, 296,
loop (8.982 ms) : 8977, 8987
. : milestone, 8982,
section candidate
noprobe (342.832 µs) : 299, 387
. : milestone, 343,
basic (298.62 µs) : 292, 305
. : milestone, 299,
loop (8.985 ms) : 8980, 8990
. : milestone, 8985,
|
🟢 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. |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
![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 synchronized-wrapper guard covers every direct JDK Collections synchronized variant observed on Java 21, including SynchronizedRandomAccessList, while leaving ordinary and unmodifiable collections unchanged. No diff-only behavioral regression was found; the focused Gradle tests remain unexecuted because the sandbox lacks the repository's required Java 25 toolchain.
📊 Validated against 10 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 9fd8f4f · What is Autotest? · @DataDog review to ask questions · 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: 9fd8f4f0f8
ℹ️ 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".
jpbempel
changed the title
jpbempel
left a comment
jpbempel
left a comment
There was a problem hiding this comment.
LGTM as a quick fix for this particular case.
but needs rework for more cases and get back to an identical feature as non-synchronized collections
…rialization Calling size()/entrySet()/iterator() on java.util.Collections$Synchronized* wrappers acquires the wrapper mutex on the instrumented thread, which can deadlock the application. Treat them as unsafe so they are serialized as regular objects instead.
Add a direct WellKnownClasses.isSafe test over the nine JDK Collections$Synchronized* types and the collections that must stay safe, so the prefix check cannot regress unnoticed. Harden the serialization regression test: the mutex holder is a daemon thread and the latch wait moved inside the try block, so a failed wait cannot leave the monitor held and block the test JVM. The assertion now checks the backing map is emitted as a field.
ListValue.isEmpty() was the only expression language entry point that called into a collection without consulting WellKnownClasses, so an isEmpty() condition or log template over a Collections.synchronizedList could still take the wrapper mutex on the instrumented thread and deadlock. Mirror the guard already present in SetValue and MapValue.
dfebb2f to
80f278e
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 8105e33: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
PR already in the queue with status in_progress |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
3 participants
What Does This Do
Treats
java.util.Collections$Synchronized*wrappers as unsafe to call during snapshot capture, so they are serialized as plain objects instead of throughsize()/entrySet()/iterator().Fixes DYNIS-64
Motivation
Snapshot capture runs on the instrumented thread, and every method on a synchronized wrapper acquires the wrapper's mutex. Capture therefore makes the application thread take an application lock, in an order the application itself never uses. A customer hit a three-thread deadlock this way and had to disable Exception Replay to recover.
Monitor entry is untimed and uninterruptible, so neither the eval timeout nor the existing exception handling can unwind a thread that is already blocked. Not calling into the wrapper at all is the only reliable fix.
Additional Notes
java.basedoes not openjava.utilto the agent, so these values are captured without contents. Improving that rendering is a follow-up.Unsupported Collection class: ...instead of evaluating, since the expression language shares the same safety check.ListValue.isEmpty()was missing that check and is guarded here too, so anisEmpty()condition can no longer take the mutex.Vector,Stack,HashtableandPropertieslock on the instance,Collections.unmodifiableMap(synchronizedMap(...))delegates through to the wrapper, and Guava'sSynchronized$*wrappers carry the same hazard. Left for a follow-up, which should land after the rendering fix above.Testing
WellKnownClassesTestcovers the nine JDK synchronized wrapper types plus the collections that must stay safe.IsEmptyExpressionTestcovers the guardedisEmpty()path for a synchronized list and map.SnapshotSerializationTest.synchronizedMapLockedByAnotherThreadserializes a synchronized map while another thread holds its mutex. It blocks until the timeout before the fix and completes after it.Contributor Checklist
type:andcomp:labels