fix(okhttp): capture scope at AsyncCall.<init> to keep traces intact under virtual-thread Dispatcher#11479
Conversation
|
dougqh
requested review from
sarahchen6 and
vandonr
and removed request for
a team
|
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
![chatgpt-codex-connector[bot]](https://ghub.bbstale.com/avatars/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: 8f5eb8a9e3
ℹ️ 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".
| tasks.named("vthread21Test", Test) { | ||
| javaLauncher = javaToolchains.launcherFor { | ||
| languageVersion = JavaLanguageVersion.of(21) | ||
| } |
There was a problem hiding this comment.
This suite is forced to run with a JDK 21 launcher, but it does not declare a task-specific testJvmConstraints { minJavaVersion = JavaVersion.VERSION_21 }. The repo's test-JVM plugin gates tasks from the requested -PtestJvm, not from a later launcher override, so lower-JDK shards such as -PtestJvm=8/11 still consider vthread21Test allowed and then run or resolve this JDK 21-only suite from check. Other per-suite JDK 21 tests in this repo add the constraint on the Test task; doing the same here keeps lower-JDK checks from unexpectedly executing this virtual-thread coverage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I can only run on private repositories.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master resultsStartup Time
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
BitsAI applied the CI fixThe fix has been applied to this PR without validation. Progress 🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟢 Applied Commit: |
…under virtual-thread Dispatcher load
OkHttp's Dispatcher recursively promotes queued AsyncCalls from inside
finished(), running on a worker thread that has a *different* call's
parent scope active. The existing TaskRunnerInstrumentation captures
that wrong scope when the promoted call is executed, so under a
virtual-thread-per-task Dispatcher (or any executor where the same
recursion exposes the issue) okhttp.request spans cross-contaminate
between concurrent caller traces. Single-shot requests don't trigger
this because nothing is queued.
Fix: add AsyncCallInstrumentation that captures the active scope at
AsyncCall.<init> (i.e. the user thread that ran enqueue(), where the
right parent is active) and stores it in the shared
ContextStore<Runnable, State>. RunnableInstrumentation then activates
that state on AsyncCall.run() entry, overriding whatever scope
TaskRunner inherited from the dispatcher-recursion path.
Covers both class locations (3.x and 4.x). Muzzle passes against 24
OkHttp versions in [3.0.0, 5.3.2]. All 168 pre-existing forked OkHttp
tests still pass.
Tests added:
* okhttp-3.0/src/vthread21Test/.../OkHttpVirtualThreadDispatcherTest:
concurrent-burst test (16 parents x 4 requests through a virtual-
thread Dispatcher with capacity 4) plus two single-shot baselines.
The concurrent-burst test fails on master and v1.61.1 without the
fix and passes with it.
* java-concurrent-21.0/src/test/java/ThreadPerTaskExecutorVirtualThreadTest:
pure-executor regression coverage for Thread.ofVirtual().factory(),
the .name(prefix, start) builder variant, recursive submission, and
propagation across virtual-thread unmount.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dougqh
force-pushed
the
fix/okhttp-virtual-thread-dispatcher-trace-contamination
branch
from
eaf3d86 to
58311a9
Compare
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
I disabled the advice and I had two tests over 3 still green. Only concurrentVirtualThreadPerTaskDispatcher_keepsEachTraceSeparate failed. I can suggest to review what's tested and also there are okhttp 4.x related classes that are matched but the test are only run on 3.11.0
| tasks.named("check") { | ||
| dependsOn "vthread21Test" | ||
| } |
There was a problem hiding this comment.
why this is needed? Normally you should use the testJvmConstraints to asses min jvm version for testing
There was a problem hiding this comment.
I think that doesn't quite work here because it is a mix of okhttp and virtual threads that we need to test. The fix is with okhttp, but using virtual threads requires Java 21.
|
|
||
| // Separate suite for tests that need the JDK 21+ virtual-thread API. Kept apart from the | ||
| // default test suite so that the existing OkHttp 3.0 tests keep their Java 1.8 baseline. | ||
| addTestSuite('vthread21Test') |
There was a problem hiding this comment.
the test can simply put under test - no need to have a different test suite
| public String[] knownMatchingTypes() { | ||
| return new String[] { | ||
| "okhttp3.RealCall$AsyncCall", // OkHttp 3.x | ||
| "okhttp3.internal.connection.RealCall$AsyncCall", // OkHttp 4.x |
There was a problem hiding this comment.
according to the gradle build there are no tests at all for 4.x. So how this can be verified?
| * {@code TracingInterceptor.intercept()} on the executor thread using {@code activeSpan()}, so "is | ||
| * the parent scope active on the worker?" is the entire question. | ||
| */ | ||
| class ThreadPerTaskExecutorVirtualThreadTest extends AbstractInstrumentationTest { |
There was a problem hiding this comment.
This is not related to okhttp (directly). I would recommend to add in a different PR
…t suite - Replace javaLauncher override with testJvmConstraints so lower-JDK shards skip vthread21Test rather than forcing JDK 21 - Drop OkHttp 4.x from AsyncCallInstrumentation.knownMatchingTypes() until test coverage exists; trim stale 4.x javadoc - Remove ThreadPerTaskExecutorVirtualThreadTest (no OkHttp dependency, belongs in a separate PR) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
What Does This Do
AsyncCallInstrumentationthat captures the active scope atAsyncCall.<init>(i.e. on the user thread that calledCall.enqueue(callback)) and stores it in the sharedContextStore<Runnable, State>.RunnableInstrumentationthen activates that state onAsyncCall.run()entry, overriding whatever scopeTaskRunnerinherited from a dispatcher-recursion path.Dispatcherwith capacity 4) plus single-shot baselines. This test fails on master and v1.61.1 without the fix and passes with it.Thread.ofVirtual().factory(), the.name(prefix, start)builder variant, recursive worker-thread submission, and propagation across virtual-thread unmount.Motivation
Reported by DataDog/profiling-backend#8520, which swapped its OkHttp dispatcher from
Executors.newCachedThreadPool(...)toExecutors.newThreadPerTaskExecutor(Thread.ofVirtual().name(prefix, start).factory())and started seeing broken traces.OkHttp's
Dispatcherrecursively promotes queuedAsyncCalls from insideDispatcher.finished(), which runs on a dispatcher worker thread with a different call's parent scope still active (re-instated byTaskRunner.run()). The existingTaskRunnerInstrumentationcaptures that wrong scope when the promoted call is submitted to the executor, so under a virtual-thread-per-task dispatcher — or any executor where this recursion exposes itself —okhttp.requestspans cross-contaminate between concurrent caller traces. Single-shot requests don't trigger it because nothing is queued.The right capture point is the moment the user enqueued the call, where the right parent is active. That's exactly
AsyncCall.<init>.Deep dive: failure mode and propagation walk
Why the old code worked
Executors.newCachedThreadPool(...)returns ajava.util.concurrent.ThreadPoolExecutor, covered byThreadPoolExecutorInstrumentation. That instrumentation captures the active continuation into aStatekeyed by theAsyncCallatexecute()time, thenbeforeExecuteactivates it on the worker. Theokhttp.requestspan is created insideAsyncCall.run()with the parent scope reliably active.Why the new code breaks
Executors.newThreadPerTaskExecutor(Thread.ofVirtual()...)returnsjava.util.concurrent.ThreadPerTaskExecutor, which is not aThreadPoolExecutor:ThreadPoolExecutorInstrumentation'sextendsClass(named("java.util.concurrent.ThreadPoolExecutor"))doesn't match. No advice fires on itsexecute(...).JavaExecutorInstrumentationdoesn't match either —ThreadPerTaskExecutoris not in itsknownMatchingTypesanddd.trace.executors.allis off by default.The only things that fire are:
TaskRunnerInstrumentation— advisesThreadPerTaskExecutor$TaskRunner.<init>andrun(). The constructor stashes a continuation in aStatekeyed by the TaskRunner, not the user'sAsyncCall.VirtualThreadInstrumentation— captures the wholeContextatVirtualThread.<init>, swaps it in/out on eachmount/unmount.In principle this propagates the scope. In practice, both fire on whatever thread calls
executor.execute(...). That's the user thread for the first call from a given caller — but it's a dispatcher worker for any call that gets queued.Dispatcher recursion → cross-trace contamination
readyAsyncCalls.AsyncCall.run()'sfinally,dispatcher.finished(this)runs — still on Parent_A's worker thread with Parent_A's scope active (reinstated byTaskRunner.run).finished()callspromoteAndExecute(), which callsexecutor.execute(nextCall)for a queued call belonging to some other parent.TaskRunnerInstrumentation.Constructcaptures whatever scope is active on the current worker — which is Parent_A, not the parent that actually calledenqueue()fornextCall.okhttp.requestspan lands in Parent_A's trace.Repeat at scale → Parent_A's trace accumulates strays; Parent_B / Parent_C / etc. end up with empty traces. The single-shot tests miss it entirely because nothing is in the ready queue when
enqueue()returns.Empirical confirmation
Concurrent-burst test failures before the fix (same source, just on different branches):
mastertrace rooted at parent 1 has wrong child count ==> expected: <4> but was: <0>(that parent's calls were stolen, its trace ended up empty)v1.61.1trace rooted at parent 1 has wrong child count ==> expected: <4> but was: <17>(that parent absorbed 13 strays from siblings)How the fix propagates correctly
With
AsyncCallInstrumentation.Constructin place:realCall.enqueue(cb)→new AsyncCall(cb)→Constructadvice fires → captures Parent_Y's continuation on the AsyncCall inContextStore<Runnable, State>. ✓executor.execute(asyncCall_Y)→TaskRunner.<init>→ captures Parent_A on the TaskRunner. (Irrelevant noise — we don't try to fixTaskRunner.)TaskRunner.run():TaskRunnerInstrumentation.Runactivates Parent_A on the worker.TaskRunner.run()callsasyncCall_Y.run().RunnableInstrumentation.RunnableAdvicefires on entry → reads theStatestored in step 1 → activates Parent_Y on top of Parent_A.getResponseWithInterceptorChain()runs theTracingInterceptor→activeSpan()returns Parent_Y →okhttp.requestis parented under Parent_Y. ✓RunnableAdvicecloses Parent_Y's scope, thenTaskRunnercloses Parent_A's. Stack unwinds cleanly.Two key properties make this safe:
AdviceUtils.captureonly writes a continuation if one isn't already there and async propagation is on, so we never overwrite a previously captured state.RunnableInstrumentation's advice runs insideTaskRunner.run(), afterTaskRunner's activation. That inner scope wins for the entire duration ofasyncCall.run()— which is exactly when theokhttp.requestspan is created.Compatibility
Covers both class locations:
okhttp3.RealCall$AsyncCallokhttp3.internal.connection.RealCall$AsyncCallMuzzle passes against 24 OkHttp versions in
[3.0.0, 5.3.2]. The capture re-uses the existing"okhttp"/"okhttp-3"instrumentation names — no new feature flag, so OkHttp tracing being enabled implies this capture is enabled.Test plan
:dd-java-agent:instrumentation:okhttp:okhttp-3.0:vthread21Test— new suite, 3 tests pass:dd-java-agent:instrumentation:okhttp:okhttp-3.0:test— IAST suite, 2 tests pass:dd-java-agent:instrumentation:okhttp:okhttp-3.0:forkedTest— 168 pre-existing OkHttp forked tests still pass (sync + async, V0 + V1):dd-java-agent:instrumentation:java:java-concurrent:java-concurrent-21.0:test—ThreadPerTaskExecutorVirtualThreadTest(4 new) +VirtualThreadPerTaskExecutorTest(9 pre-existing) all pass:dd-java-agent:instrumentation:okhttp:okhttp-3.0:muzzle— passes against 24 OkHttp versions🤖 Generated with Claude Code