fix: ForkJoinTask captured context is retained until GC (#626) - #627
Open
DZQOX wants to merge 1 commit into
Open
fix: ForkJoinTask captured context is retained until GC (#626)#627DZQOX wants to merge 1 commit into
DZQOX wants to merge 1 commit into
Conversation
The constructor advice captures a context snapshot for every ForkJoinTask, but the exec/run exit only restores and never removes, so eviction depends entirely on the task being collected, the weak key being enqueued, and some later put/get happening to drain the queue. That is not a leak in the strict sense -- the snapshot does not reference the task back, so the weak key is still collectible. It behaves like one, though: the snapshot is the map value and stays strongly reachable from the static cache, so it survives every young GC and is promoted to the old generation, and entries linger once traffic stops because nothing calls check() any more. Worst on JDK 21 virtual threads. VirtualThread.runContinuation is a plain Runnable, so ForkJoinPool.execute(Runnable) allocates a new RunnableExecuteAction on every submit -- one entry per park/unpark, and a full Transmitter.capture() (a fresh HashMap plus deep-copied CallDepth values) on each one. Changes: - Drop the entry at the exec/run exit. This is that task's terminal execution: Completion.run()/exec() are final on the base class and are both tryFire(ASYNC), and the ASYNC branch skips claim(), runs the function and then nulls src/dep/fn, so any later tryFire returns at the entry null check. Same on JDK 8 and 21. - Guard the removal on backup != null, which holds exactly when captured was non-null on entry because replay(null) returns null. Nothing was recorded otherwise, the constructor advice is gated on TraceContextManager and never put an entry, and exec is one of the hottest methods in the JVM. - Add onThrowable to the exit advice. Throwing out of exec is a normal path: RunnableExecuteAction.exec() is a bare runnable.run(), AdaptedCallable.exec() rethrows, and AdaptedXxx.run() is invoke(), which reports by rethrowing, so both the run and exec exits are skipped. Without it the replayed context is never restored and the worker picks up its next task still carrying the previous trace, recording downstream calls under a foreign traceId. That is a pre-existing defect; the new removal would be skipped on the same paths. - CAPTURED_CACHE is declared as WeakCache so the advice can call remove. Cache itself does not get a remove method: TrieCache is a prefix tree and has no matching semantics, and CAPTURED_CACHE is the only caller. Not covered, and called out in the class javadoc: only tasks that reach exec/run get their entry dropped. Completions built by the non-async operators have a null executor and are only driven through tryFire(SYNC)/tryFire(NESTED), and CompletableFuture$Signaller is captured but never submitted to the pool; both still wait for the weak key. There is no safe removal signal inside tryFire -- a null return means both "spun without firing" and "fired with no dependent to propagate", and Completion.isLive() is package-private so inlined advice cannot reach it. Binding the snapshot to a field on the task would remove the map altogether and cover those too, at the cost of a field-injection mechanism. Behaviour change: after reinitialize() the same instance can be forked again and the second execution no longer replays. It previously replayed the snapshot taken at construction, which was already the wrong context. The JDK never calls reinitialize() itself. Verified: arex-agent-bootstrap 138/138, arex-executors 23/23. Mutation checked -- dropping the backup guard fails execAdviceSkipsRemoveWhenNothingWasReplayed, deleting the remove fails execAdviceRemovesCapturedEntryOnExit, and dropping onThrowable fails exitAdviceAlsoRunsOnTheExceptionPath.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Fixes #626
Summary
The constructor advice captures a context snapshot for every
ForkJoinTask, but theexec/runexit restores the context and never removes the entry. Eviction therefore depends on the task being collected, the weak key being enqueued, and some laterput/gethappening to drain the queue.What's wrong
This is not a leak in the strict sense — the snapshot holds no reference back to the task, so the weak key is still collectible. The effect is the same, though. The snapshot is the map value, so it stays strongly reachable from the static cache: it survives every young GC and is promoted to the old generation. And once traffic stops nothing calls
check()again, so the entries simply stay.This is worst on JDK 21 virtual threads.
VirtualThread.runContinuationis a plainRunnable, soForkJoinPool.execute(Runnable)allocates a newRunnableExecuteActionon every submit — one entry per park/unpark, each one also paying a fullTransmitter.capture()(a freshHashMapplus deep-copiedCallDepthvalues).The fix
Drop the entry at the
exec/runexit. That is the task's terminal execution.Completion.run()andexec()are final on the base class and are bothtryFire(ASYNC); the ASYNC branch skipsclaim(), runs the function, then nullssrc/dep/fn, so any latertryFirereturns at the entry null check. Same shape on JDK 8 and 21. For plain ForkJoinTasks,doExec()callsexec()once.Guard the removal on
backup != null. That holds exactly whencapturedwas non-null on entry, becausereplay(null)returns null. Otherwise nothing was being recorded, the constructor advice is gated onTraceContextManagerand never put an entry, andexecis one of the hottest methods in the JVM.Add
onThrowableto the exit advice. Throwing out ofexecis a normal path, not a corner case:RunnableExecuteAction.exec()is a barerunnable.run(),AdaptedCallable.exec()rethrows, andAdaptedXxx.run()isinvoke(), which reports by rethrowing — so both therunandexecexits are skipped. Without it the replayed context is never restored and the worker picks up its next task still carrying the previous trace, recording downstream calls under a foreign traceId. That is a pre-existing defect, and the new removal would be skipped on the same paths.CAPTURED_CACHEis declared asWeakCacheso the advice can callremove.Cacheitself does not gain aremovemethod:TrieCacheis a prefix tree with no matching semantics, andCAPTURED_CACHEis the only caller.Not covered
Called out in the class javadoc so it is not mistaken for a complete fix. Only tasks that reach
exec/runget their entry dropped:thenApply,thenCompose, ...) have a null executor and are only driven throughtryFire(SYNC)/tryFire(NESTED).CompletableFuture$Signalleris captured but never submitted to the pool.cleanStack()).Those still wait for the weak key to be collected. There is no safe removal signal inside
tryFire: a null return means both "spun without firing" and "fired with no dependent to propagate", andCompletion.isLive()is package-private, so inlined advice cannot reach it. Binding the snapshot to a field on the task itself would remove the map altogether and cover these too, at the cost of adding a field-injection mechanism.Behaviour change
After
ForkJoinTask.reinitialize()the same instance can be forked again, and the second execution no longer replays. It previously replayed the snapshot taken at construction time, which was already the wrong context for a fresh run. The JDK never callsreinitialize()itself.Verification
arex-agent-bootstrap138/138,arex-executors23/23.Mutation checked — each guard has a test that fails without it:
backup != nullguardexecAdviceSkipsRemoveWhenNothingWasReplayedremovecallexecAdviceRemovesCapturedEntryOnExitonThrowableexitAdviceAlsoRunsOnTheExceptionPath