Repository navigation
Prevent context retention in OpenJDK virtual pollers and HttpClient selector - #12626
gh-worker-dd-mergequeue-cf854d[bot] merged 7 commits into
Conversation
amarziali
added
the
tag: concurrency
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This comment has been minimized.
This comment has been minimized.
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The two most critical issues are a production memory-retention bug in PollerInstrumentation where request context leaks into long-lived poller threads when the scope-depth limit is reached, and a test compatibility failure in PollerContextForkedTest that throws NoSuchFieldException on JDK 22 due to a missing POLLERS field.
🤖 Bits Code Review · Commit 99ccea5 · @DataDog review to ask questions
🟢 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. |
|
@DataDog review |
There was a problem hiding this comment.
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
I think the PR title deserves to be more specific, it's about Java's HttpClient and VTs.
Moreover this change already appears outdated as OpenJDK 27 change a few things. Finally since it touches Java's HttpClient on the surface, but actually it's specific to Hotspot based JDKs, that could remain so, but I'll clarify this explicitly in code comments and PR description.
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Between the narrow focus of JDK 26 + HttpClient + HotSpot, the PR brings too much complexity compared to the problem it tries to solve 🤔
@PerfectSlayer which complexity do you mean? This PR only narrows the propagation boundary for one specific case that showed up in a smoke test, and it adds a single extra instrumentation. It isn't meant to cover every case, so I'd rather not widen the scope of this review. We're not running JDK 27 yet, and we'll fix it when we get to test with that. In case the instrumentation won't apply with this JDK it will be harmless. We're applying the same kind of boundary containment we already do for classic platform threads, and in my view we didn't go as in depth as we should when instrumenting virtual threads. All the machinery around observing continuation usage is what's helping us understand and fill in these missing cases. |
amarziali
changed the title
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
The expected merge time in
Build pipeline has failing jobs for 58cec04: 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.
The expected merge time in
andrea.marziali@datadoghq.com cancelled this merge request build |
|
/merge -c |
|
View all feedbacks in Devflow UI.
|
|
View all feedbacks in Devflow UI.
|
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
LGTM, but I believe the code owner is odd.
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
d72c58d
into
master
3 participants
What Does This Do
Prevent request context retention in two specific OpenJDK virtual-thread infrastructure paths:
sun.nio.ch.Poller$Pollers.start()on JDK 22–26, plusPoller$VThreadsPollerGroup.start()and lazy per-carrier read-poller creation throughPoller$PollerPerCarrierPollerGroup.startReadPoller()on JDK 27.jdk.internal.net.http.HttpClientImpl$SelectorManagerfrom virtual-thread context capture, covering the virtual HTTP client selector introduced in JDK 26.Motivation
The smoke-test continuation diagnostics enforced by #12609 exposed request retention when long-lived infrastructure virtual threads were created under an active request. First-use I/O poller initialization could capture that request in process-lifetime
SubPollerthreads; creating a JDK 26HttpClientcould retain it in the client's virtual selector.Capturing a request continuation in these threads can delay trace publication until the threads terminate. Disabling async capture alone is insufficient because virtual-thread instrumentation also stores the raw context independently. Poller startup and lazy per-carrier read-poller creation therefore swap in root context directly, including when the caller has reached the scope-depth limit, and restores the previous context on exit.
Additional Notes
Scope and limitations
This change targets the named internal classes in HotSpot-based OpenJDK implementations. Compatibility with other JVM implementations, such as OpenJ9, has not been established.
The following path remains outside this change:
ThreadPerTaskExecutor$ThreadBoundFuture, so extending the task-name exclusion alone would not cover both paths. See the review finding.Regression coverage added
2and readsPOLLERSon older JDKs orPOLLER_GROUPon JDK 27.3: blocking socket I/O under an active request creates live virtual read pollers without retaining request context or continuation, and restores the requesting virtual thread's context.Validation
testandforkedTestsuites pass on JDK 21, 25, and 27; all 28 tests run and pass on JDK 27.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]