Repository navigation
Fix context propagation for delayed Netty scheduled task - #12541
gh-worker-dd-mergequeue-cf854d[bot] merged 14 commits into
Conversation
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. |
amarziali
changed the title
![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: 24279fbd2a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new matcher has no Netty bytecode reference. Unsupported versions pass Muzzle validation, so the inverse compatibility task fails and blocks CI.
🤖 Datadog Autotest · Commit 24279fb · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
🟢 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 |
|
@codex review |
There was a problem hiding this comment.
More details
Netty 4.1.44 and later keep the captured context until runTask() starts. Older Netty versions continue to activate the context in RunnableFuture.run().
🤖 Datadog Autotest · Commit a5ba81c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5ba81ce3d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
More details
Regular and shaded delayed tasks retain context at execution time. Older Netty tasks keep the existing run path.
🤖 Datadog Autotest · Commit 3a9cabf · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a9cabf941
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
sarahchen6
left a comment
sarahchen6
left a comment
There was a problem hiding this comment.
Left a small comment about scope but otherwise looks reasonable to me!
btw - looks like the failing CI jobs can be fixed by adding
testImplementation project(':dd-java-agent:instrumentation:netty:netty-concurrent-4.0')
to dd-java-agent/instrumentation/armeria/armeria-grpc-0.84/build.gradle. Seems like Armeria's use of java-concurrent now depend on NettyPromiseTaskInstrumentation and require netty-concurrent-4.0..... "Armeria’s tests load java-concurrent, which still captures continuations when Netty scheduled tasks are constructed. This PR makes that instrumentation skip ScheduledFutureTask.run(). So, Activation and release now depend on NettyPromiseTaskInstrumentation in netty-concurrent-4.0."
…-netty-scheduled-future-context
Keep the task scope active until ScheduledFutureTask.run() exits, including success and failure listeners. Cover nested activation and scope cleanup. Add the Netty concurrent test dependency required by Armeria gRPC tests.
112148d to
ab033b9
Compare
AlexeyKuznetsov-DD
left a comment
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
LGTM, but probably lock-files should be regenerated before merge.
Minor note: tests kind-of-hard to read for human... Maybe worth adding some comments explaining what test is doing?
No worries I added more comments in 949a8bb |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
4 participants
What Does This Do
Fixes context propagation for delayed Netty scheduled tasks while moving the Netty-specific execution handling out of the generic
RunnableFutureadvice.Starting with Netty 4.1.44,
ScheduledFutureTask.run()may be called once while a delayed task is being enqueued and again when the delay expires. Activating the task scope during the enqueue-time call consumes the captured continuation, leaving no context available when the task actually executes.For Netty versions that expose
PromiseTask.runTask(), the genericRunnableFutureinstrumentation no longer activates context aroundScheduledFutureTask.run(). Thenetty-concurrentinstrumentation insteadactivates the existing
RunnableFuturestate aroundPromiseTask.runTask(), which is the point where the task body actually executes.Older Netty versions without
runTask()continue using the existing genericRunnableFuture.run()Instrumentation.This approach:
RunnableFuture -> Statecontext store.Motivation
Additional Notes
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]