Improve structured concurrency instrumentation#11759
Conversation
PerfectSlayer
added
type: enhancement
![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: 405ab8c46d
ℹ️ 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".
| if (subtask instanceof Runnable && StructuredTaskScopeHelper.isCancelled(scope)) { | ||
| ContextStore<Runnable, State> contextStore = get(Runnable.class, State.class); | ||
| cancelTask(contextStore, (Runnable) subtask); |
There was a problem hiding this comment.
When a scope is canceled by another subtask or by a timeout after StructuredTaskScopeImpl.fork has already started this subtask but before this exit advice runs, this check treats the returned subtask as if it was never started and cancels its captured continuation. If the subtask thread has not entered run() yet, the Runnable advice will then find no continuation, so started subtasks under cancel-on-completion joiners such as anySuccessfulResultOrThrow() (or failure-canceling joiners) can run without the parent trace context. This cleanup should be limited to subtasks that were actually skipped, not every subtask returned while the scope is canceled at method exit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is an issue, I will handle it next week.
🟢 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. |
PerfectSlayer
force-pushed
the
bbujon/virtual-threads
branch
from
405ab8c to
2c1064f
Compare
amarziali
left a comment
amarziali
left a comment
There was a problem hiding this comment.
looks fine to me. Left one suggestion. Also agree with the issue raised by codex
| ClassLoader classLoader = StructuredTaskScopeHelper.class.getClassLoader(); | ||
| Class<?> scopeClass = | ||
| Class.forName("java.util.concurrent.StructuredTaskScope", false, classLoader); | ||
| return publicLookup().findVirtual(scopeClass, "isCancelled", methodType(boolean.class)); |
There was a problem hiding this comment.
you can use also our datadog.trace.util.MethodHandles class that already handle class loading, method finding and exception suppression
There was a problem hiding this comment.
Interesting! I will have a look if I still need it 👍
Yeah, I did the testing migration and fixes by hand but I let Claude handles the joiner case. |
JUnit testing and trace assert make unit testing possible.
PerfectSlayer
force-pushed
the
bbujon/virtual-threads
branch
from
6cdff97 to
829131e
Compare
2 participants
What Does This Do
This PRs improves the structured concurrency instrumentation, in charge of context tracking for the JEP 505 (JDK 25), JEP 525 (JDK 26) and JEP 533 (JDK 27). In particular it:
Motivation
Prevent continuation (hence traces) leaks and improves reliability with dedicated unit tests.
Additional Notes
Structucred Concurrency is still a JDK feature in preview.
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 issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: [PROJ-IDENT]