Preserve context when the same task instance is submitted twice - #12283
Preserve context when the same task instance is submitted twice#12283amarziali wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
The fallback wrapper can make priority executors reject a second comparable task with ClassCastException. It also loses a non-root context that has no active span.
🤖 Datadog Autotest · Commit de6e25a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| // This same instance is already queued with a pending continuation, and its single State | ||
| // slot cannot hold ours as well. Fall back to wrapping so this submission carries its own | ||
| // context instead of silently running under the root context. | ||
| task = Wrapper.wrap(task); |
There was a problem hiding this comment.
The second submission fails and the application task does not enter the queue.
Assertion details
- Input: Submit the same Comparable Runnable twice while its first submission remains in a naturally ordered PriorityBlockingQueue.
- Expected:
The executor must accept both submissions and keep both queue entries comparable. - Actual:
The queue compares ComparableRunnable with the raw task. The generated bridge casts the raw task to ComparableRunnable and throws ClassCastException.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| // This same instance is already queued with a pending continuation, and its single State | ||
| // slot cannot hold ours as well. Fall back to wrapping so this submission carries its own | ||
| // context instead of silently running under the root context. | ||
| task = Wrapper.wrap(task); |
There was a problem hiding this comment.
Features that store data in Context can lose that data during duplicate task submission.
Assertion details
- Input: Submit the same Runnable twice from non-root contexts that contain no AgentSpan while the first submission remains pending.
- Expected:
The fallback must give the second submission its own carrier for the full current Context. - Actual:
Wrapper.wrap calls captureActiveSpan. That call returns a root continuation when no span exists. The method then returns the original task, so the occupied State still drops the second context.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de6e25a6eb
ℹ️ 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 same instance is already queued with a pending continuation, and its single State | ||
| // slot cannot hold ours as well. Fall back to wrapping so this submission carries its own | ||
| // context instead of silently running under the root context. | ||
| task = Wrapper.wrap(task); |
There was a problem hiding this comment.
When the reused task implements Comparable and the executor uses a PriorityBlockingQueue, the first queued entry is the original task while this fallback turns the second into a ComparableRunnable. During the second offer, the queue invokes ComparableRunnable.compareTo with the original task; its generated bridge casts that argument to ComparableRunnable, causing a ClassCastException and rejecting the submission. The collision carrier must remain mutually comparable with an unwrapped entry or avoid mixing wrapped and unwrapped tasks.
Useful? React with 👍 / 👎.
| // This same instance is already queued with a pending continuation, and its single State | ||
| // slot cannot hold ours as well. Fall back to wrapping so this submission carries its own | ||
| // context instead of silently running under the root context. | ||
| task = Wrapper.wrap(task); |
There was a problem hiding this comment.
When the current context is non-root but contains no AgentSpan, AdviceUtils.shouldCapture deliberately accepts it and the failed state capture reaches this branch, but Wrapper.wrap uses captureActiveSpan(), which returns the root continuation when no span is active and consequently returns the original task unchanged. The repeated submission therefore still loses custom spanless context, contrary to the new fallback contract; the carrier needs to capture the Context.current() value that failed to enter the state slot.
Useful? React with 👍 / 👎.
| // This same instance is already queued with a pending continuation, and its single State | ||
| // slot cannot hold ours as well. Fall back to wrapping so this submission carries its own | ||
| // context instead of silently running under the root context. | ||
| task = Wrapper.wrap(task); |
There was a problem hiding this comment.
When the first execution has started but its state was still occupied when the same task was submitted again, the queued second submission is now a Wrapper whose identity and equals differ from the submitted task. A later ThreadPoolExecutor.remove(task) therefore returns false and leaves that queued execution in place, whereas without instrumentation it removes the second occurrence; similarly, removing both queued occurrences by calling remove(task) twice leaves the wrapped one behind. Avoid exposing a distinct queue element or ensure removal can locate the wrapper by its delegate.
Useful? React with 👍 / 👎.
| // This same instance is already queued with a pending continuation, and its single State | ||
| // slot cannot hold ours as well. Fall back to wrapping so this submission carries its own | ||
| // context instead of silently running under the root context. | ||
| task = Wrapper.wrap(task); |
There was a problem hiding this comment.
With a pool containing at least two workers, the wrapped second submission can reach delegate.run() before the worker that dequeued the original submission enters beforeExecute. Wrapper.run() first activates the second context, but the delegate's RunnableInstrumentation then consumes the still-pending first continuation from the shared State and activates it on top, so the second run observes the first parent and the original run later has no parent. The fallback must isolate the delegate from its earlier state rather than relying on FIFO dequeue order to imply scope-activation order.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
🟢 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. |
|
Closing for now since fixing an edge case might introduce more issues to handle |
What Does This Do
The non-wrapping
ThreadPoolExecutorpath (the default) stores the captured context in aStateattached to the task instance, and a State holds a single continuation. So when the same Runnable instance is submitted again while an earlier submission is still queued, the second capture is dropped and that execution runs under the root context, losing the parent.AdviceUtils.capturenow reports that conflict, and theexecuteadvice falls back to wrapping for the losing submission so it carries its own context.Affected every reused named class and anonymous inner class; JVM lambdas were accidentally safe already, since they are always wrapped. Only the
ThreadPoolExecutorpath is fixed here: ForkJoin tasks cannot be wrapped (since no field injected)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]