Fix async context loss when field injection is unavailable - #12030
Fix async context loss when field injection is unavailable#12030CharlyRien wants to merge 2 commits into
Conversation
When context-store fields cannot be injected (e.g. JDK 25 AOT class linking pre-links carrier classes before the agent can transform them), every ContextStore access falls back to WeakMapContextStore. Its put() silently dropped new entries past a 50k cap measured with WeakConcurrentMap.approximateSize(), which also counts collected but not-yet-expunged entries (expunged ~once per second) - so past ~50k context writes/sec live async context was discarded and spans were orphaned (DataDog#10479). Rebuild the fall-back store on ConcurrentHashMap with identity-based weak keys and no cap - growth is bounded by live carriers, exactly like the injected-field path: - collected keys are drained from a ReferenceQueue on every write and by a periodic background task, so dead entries don't accumulate on idle or read-only stores - reads are allocation-free (reused thread-local lookup key) - context factories run outside the map's own locks, so they may re-enter the store (CHM.computeIfAbsent forbids this) JMH benchmark (old vs new, 1/10/100 threads) shows ~30% faster single-threaded and parity under contention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
785f022 to
78f023d
Compare
|
FYI, we plan to incorporate the new approach under a feature flag - as that will allow switching between the approaches, rather than completely remove the limit (which would work for AOT but could also have an unforeseen impact on non-AOT users) For example: a misbehaving instrumentation could leak store entries - and with no limit or forcible reclamation that would eat up the heap and impact the application. |
Yes it would be great. Thanks for letting me know. |
For the misbehaving instrumentation, I had exactly this one in mind also. 😓 |
I'd prefer to leave the current |
What does this PR do?
Removes the silent-drop size cap from
WeakMapContextStore, the fall-back used when context-store field injection is unavailable, and rebuilds it onConcurrentHashMap+ identity-based weak keys withReferenceQueuecleanup.Motivation
Fixes #10479. Under JDK 25 AOT class linking (
-XX:AOTCache, on by default) carrier classes come pre-linked from the cache, field injection fails, and everyContextStoreaccess falls back to this weak map. Itsput()silently dropped entries past a 50k cap measured withWeakConcurrentMap.approximateSize(), which also counts collected-but-unexpunged entries (expunge runs ~1/s) — so past ~50k context writes/sec live async context was discarded and spans were orphaned. A reproducer loses 98.5% of async context with class linking on, 0% without; with this fix, 0% everywhere (verified up to ~1.4M writes/sec).There is deliberately no cap anymore: growth is bounded by live carriers, exactly like the injected-field path. Dead entries are drained on writes and by a 1s background task (same
AgentTaskSchedulermechanism the oldWeakMaps-backed map used), so idle/read-only stores don't retain collected contexts. Reads are allocation-free (reused thread-local lookup key), and context factories run outside the map's own locks so they may re-enter the store.JMH benchmark included (old vs new, 1/10/100 threads): ~30% faster single-threaded, parity under contention, 0 B/op on the hot paths.
This aligns with the direction of the
mcculls/global-weak-context-storebranch (uncapped weak storage + ReferenceQueue cleanup), scoped to the fall-back store only.Memory characteristics
Measured retained heap (after full GC) of old vs new store:
ConcurrentHashMaptable grows to match and never shrinks. This is a one-time high-water mark, not a leak — repeated bursts on the same store do not accumulate (subsequent rounds retained 8–9 MB when GC ran mid-burst, which is the realistic case since real churn comes with allocation pressure). The uncappedWeakConcurrentMapdirection has the same property; it is inherent to keeping the data instead of dropping it.Additional Notes
Regression tests drive real GC (dead-carrier churn past the former cap) and lock in the factory-reentrancy contract.
🤖 Generated with Claude Code