Fix jax-rs/jakarta-rs span double-finish on synchronous AsyncResponse#resume() - #12637
Yeison2020 wants to merge 2 commits into
Conversation
…#resume() AsyncResponse#resume()/cancel() finished the span immediately, even when called synchronously from within the still-running resource method that owns it. The resource method's own exit advice then finished the same span again once it actually returned, and any work done in between was attributed to an already-finished span. Defer to the resource method's own exit advice whenever resume()/cancel() is nested inside it; a per-thread reentrancy counter distinguishes that case from a genuine cross-thread resume, which is unaffected. Fixes #12597.
Yeison2020
added
inst: jax-rs
🟢 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. |
…ead counter Independent code review found the activeSpan()+counter check from the previous commit had three real gaps: it broke when resume()/cancel() was called from a nested @trace helper (activeSpan() became the helper's span, not the resource method's), it could misclassify a genuinely-async resume as synchronous on a shared thread pool (leaking the span instead of double-finishing it), and the counter lived on a per-classloader helper class, so a modular container loading the two advices into different classloaders would silently make the whole check a no-op. Replace the counter with ResourceMethodSpanTracker, a small bootstrap-loaded class (visible across all classloaders, like the existing CallDepthThreadLocalMap) holding a per-thread stack of the actual span references for currently-open resource-method invocations. The check becomes a direct "is this span still the innermost open invocation on this thread", with no activeSpan() comparison needed. Also: clear the stale AsyncResponse->span mapping on the resource method's throwable exit path (previously only done on the normal path), and add jakarta.ws.rs test coverage for this fix, which had none despite being duplicated into that module.
Yeison2020
added
the
tag: override groovy enforcement
|
Adding `tag: override groovy enforcement` for the one new file this flags: `JakartaRsAsyncResponseInstrumentationTest.groovy`. This test needs `InstrumentationSpecification` (the `assertTraces`/`enabledFinishTimingChecks()` DSL that does real ByteBuddy transformation and asserts on actual span/scope lifecycle) to verify this fix at all — a plain unit test calling the advice method directly wouldn't exercise the actual bug (which is about span/scope timing across two separate advices). `InstrumentationSpecification` extends Spock's `DDSpecification`, and there's currently no JUnit5 equivalent in this codebase for that level of instrumentation testing, so this can't be mechanically converted without losing the verification it exists to provide. Context: this test closes a real coverage gap an earlier code-review pass on this PR found — the jakarta.ws.rs path (this fix touches both javax.ws.rs and jakarta.ws.rs, mirrored) had zero automated regression coverage for the exact bug being fixed. The javax.ws.rs path's equivalent coverage lives in the pre-existing `CxfContextPropagationTest.groovy` (modified, not new, so it doesn't trip this check). |
This branch has not been deployed
1 participant
What's the problem?
JAX-RS lets a resource method accept a
@Suspended AsyncResponseand callresume()on it synchronously, mid-method, without ever really suspending. That's spec-legal and it's what real applications do (including our own pre-existing test for this).When that happens, two separate pieces of tracer code each thought they were the one responsible for finishing the span:
AsyncResponse#resume()/cancel()advice finished the span the instantresume()was called — even though the resource method was still running.Between those two points, any work the method did (logging, extra processing, nested spans) got attached to a span that Datadog already considered finished. In a busy production app (this surfaced via TomEE + CXF), that's the kind of "phantom activity under an already-finished span" that can eventually corrupt the tracer's internal scope bookkeeping on a reused worker thread — reported as #12597.
What's the fix?
When
resume()/cancel()is called, check whether we're still, right now, nested inside the resource method that owns this span (as opposed to being called later, from a different thread, which is the normal async pattern). If we are, don't finish the span here — let the resource method's own exit advice do it, exactly once, once the method actually returns.That "are we still nested inside it" check needs two things, not one:
Both are needed together — the first question alone gives a false positive whenever the tracer's own executor instrumentation has propagated this span's context onto a background worker thread for genuinely asynchronous work, which would otherwise look identical from that check alone.
The genuinely-asynchronous case (resume() called later, from a different thread — the standard, intended pattern) is completely unaffected by this change.
How was this verified?
resume(), synchronouscancel(), and a real cross-thread resume via a background executor (the case that specifically validates the fix doesn't regress the normal async pattern).muzzlecompatibility check across the full supportedjavax.ws.rs/jakarta.ws.rsversion matrix.