From 8a5e7cbad4234dce02c3999ec2672667f65d9dbc Mon Sep 17 00:00:00 2001 From: Yeison Casado Date: Thu, 24 Sep 2026 16:38:19 -0400 Subject: [PATCH 1/2] Fix jax-rs/jakarta-rs span double-finish on synchronous AsyncResponse#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. --- .../groovy/CxfContextPropagationTest.groovy | 200 +++++++++++++++++- .../src/test/java/AsyncCancelResource.java | 22 ++ .../src/test/java/AsyncResumeResource.java | 17 ++ .../test/java/TrueAsyncResumeResource.java | 36 ++++ .../JakartaRsAnnotationsDecorator.java | 51 +++++ .../JakartaRsAnnotationsInstrumentation.java | 2 + ...JakartaRsAsyncResponseInstrumentation.java | 22 +- .../jaxrs2/JaxRsAnnotationsDecorator.java | 51 +++++ .../JaxRsAnnotationsInstrumentation.java | 2 + .../JaxRsAsyncResponseInstrumentation.java | 22 +- 10 files changed, 418 insertions(+), 7 deletions(-) create mode 100644 dd-java-agent/instrumentation/cxf-2.1/src/test/java/AsyncCancelResource.java create mode 100644 dd-java-agent/instrumentation/cxf-2.1/src/test/java/AsyncResumeResource.java create mode 100644 dd-java-agent/instrumentation/cxf-2.1/src/test/java/TrueAsyncResumeResource.java diff --git a/dd-java-agent/instrumentation/cxf-2.1/src/test/groovy/CxfContextPropagationTest.groovy b/dd-java-agent/instrumentation/cxf-2.1/src/test/groovy/CxfContextPropagationTest.groovy index 9e8e660b781..49682dfe8e7 100644 --- a/dd-java-agent/instrumentation/cxf-2.1/src/test/groovy/CxfContextPropagationTest.groovy +++ b/dd-java-agent/instrumentation/cxf-2.1/src/test/groovy/CxfContextPropagationTest.groovy @@ -22,12 +22,18 @@ class CxfContextPropagationTest extends InstrumentationSpecification { @Override void setupSpec() { JAXRSServerFactoryBean sf = new JAXRSServerFactoryBean() - sf.setResourceClasses(TestResource) + sf.setResourceClasses(TestResource, AsyncResumeResource, TrueAsyncResumeResource, AsyncCancelResource) List providers = [new TestExceptionMapper()] sf.setProviders(providers) sf.setResourceProvider(TestResource, new SingletonResourceProvider(new TestResource(), true)) + sf.setResourceProvider(AsyncResumeResource, + new SingletonResourceProvider(new AsyncResumeResource(), true)) + sf.setResourceProvider(TrueAsyncResumeResource, + new SingletonResourceProvider(new TrueAsyncResumeResource(), true)) + sf.setResourceProvider(AsyncCancelResource, + new SingletonResourceProvider(new AsyncCancelResource(), true)) sf.setAddress("http://localhost:0") server = sf.create() @@ -40,6 +46,15 @@ class CxfContextPropagationTest extends InstrumentationSpecification { server?.stop() } + @Override + protected boolean enabledFinishTimingChecks() { + // Regression guard for https://github.com/DataDog/dd-trace-java/issues/12597: + // fails the test with the exact "finished more than once" stack traces if the + // jax-rs.request span is ever finished twice (e.g. once from AsyncResponse#resume() + // and again from the resource method's own exit advice). + return true + } + def "should propagate context on async request resume"() { setup: def client = OkHttpUtils.client() @@ -97,4 +112,187 @@ class CxfContextPropagationTest extends InstrumentationSpecification { } } } + + def "resume() called synchronously from within the resource method finishes the span only once"() { + // Regression test for https://github.com/DataDog/dd-trace-java/issues/12597: when + // AsyncResponse#resume() is called synchronously (not truly suspended, the resource + // method keeps running), the resource method's own jax-rs.request scope is still on + // top of the scope stack. Before the fix, JakartaRsAsyncResponseInstrumentation / + // JaxRsAsyncResponseInstrumentation would eagerly finish the span there, so any work + // done afterwards (here: doWorkAfterResume()) would be attributed as a child of an + // already-finished span, and the resource method's own exit advice would finish the + // same span a second time (caught by enabledFinishTimingChecks()). + setup: + def client = OkHttpUtils.client() + when: + def response = client.newCall(new Request.Builder() + .url("http://localhost:$port/asyncresume") + .get().build()).execute() + then: + assert response.code() == 200 + assert response.body().string() == "OK" + + assertTraces(1) { + trace(3) { + sortSpansByStart() + span { + operationName "servlet.request" + resourceName "GET /asyncresume" + spanType DDSpanTypes.HTTP_SERVER + errored false + parent() + tags { + "$Tags.COMPONENT" "jax-rs" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + "$Tags.PEER_HOST_IPV4" "127.0.0.1" + "$Tags.PEER_PORT" Integer + "$Tags.HTTP_URL" "http://localhost:$port/asyncresume" + "$Tags.HTTP_HOSTNAME" "localhost" + "$Tags.HTTP_METHOD" "GET" + "$Tags.HTTP_STATUS" 200 + "$Tags.HTTP_ROUTE" String + "servlet.path" { it == null || it == "/asyncresume" } + "$Tags.HTTP_USER_AGENT" String + "$Tags.HTTP_CLIENT_IP" "127.0.0.1" + "$Tags.NETWORK_CLIENT_IP" "127.0.0.1" + withCustomIntegrationName("jetty-server") + defaultTags() + } + } + span { + operationName "jax-rs.request" + resourceName "AsyncResumeResource.resumeThenWork" + spanType DDSpanTypes.HTTP_SERVER + errored false + childOfPrevious() + tags { + "$Tags.COMPONENT" "jax-rs-controller" + defaultTags() + } + } + // Still parented under jax-rs.request: proves that span wasn't finished (and its + // scope wasn't popped) by resume() itself, before the resource method returned. + TraceUtils.basicSpan(it, "trace.annotation", "AsyncResumeResource.doWorkAfterResume", span(1), null, ["component": "trace"]) + } + } + } + + def "cancel() called synchronously from within the resource method finishes the span only once"() { + // Same regression as above, but for AsyncResponseCancelAdvice: cancel() is called + // synchronously and the resource method keeps running afterwards. + setup: + def client = OkHttpUtils.client() + when: + def response = client.newCall(new Request.Builder() + .url("http://localhost:$port/asynccancel") + .get().build()).execute() + then: + assert response.code() == 503 + + assertTraces(1) { + trace(3) { + sortSpansByStart() + span { + operationName "servlet.request" + resourceName "GET /asynccancel" + spanType DDSpanTypes.HTTP_SERVER + errored true + parent() + tags { + "$Tags.COMPONENT" "jax-rs" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + "$Tags.PEER_HOST_IPV4" "127.0.0.1" + "$Tags.PEER_PORT" Integer + "$Tags.HTTP_URL" "http://localhost:$port/asynccancel" + "$Tags.HTTP_HOSTNAME" "localhost" + "$Tags.HTTP_METHOD" "GET" + "$Tags.HTTP_STATUS" 503 + "$Tags.HTTP_ROUTE" String + "servlet.path" { it == null || it == "/asynccancel" } + "$Tags.HTTP_USER_AGENT" String + "$Tags.HTTP_CLIENT_IP" "127.0.0.1" + "$Tags.NETWORK_CLIENT_IP" "127.0.0.1" + withCustomIntegrationName("jetty-server") + defaultTags() + } + } + span { + operationName "jax-rs.request" + resourceName "AsyncCancelResource.cancelThenWork" + spanType DDSpanTypes.HTTP_SERVER + errored false + childOfPrevious() + tags { + "$Tags.COMPONENT" "jax-rs-controller" + "canceled" true + defaultTags() + } + } + // Still parented under jax-rs.request: proves the span wasn't finished (and its + // scope wasn't popped) by cancel() itself, before the resource method returned. + TraceUtils.basicSpan(it, "trace.annotation", "AsyncCancelResource.doWorkAfterCancel", span(1), null, ["component": "trace"]) + } + } + } + + def "resume() called from a genuinely different thread (textbook async pattern) is unaffected"() { + // Regression guard the other way: the fix must not change the standard cross-thread + // async pattern, where the resource method returns without resolving anything and a + // completely different thread calls resume() later. Here the span IS finished by the + // resume() advice (activeSpan() on that other thread is not this span), exactly as + // before the fix. + setup: + def client = OkHttpUtils.client() + when: + def response = client.newCall(new Request.Builder() + .url("http://localhost:$port/trueasyncresume") + .get().build()).execute() + then: + assert response.code() == 200 + assert response.body().string() == "OK" + + assertTraces(1) { + trace(3) { + sortSpansByStart() + span { + operationName "servlet.request" + resourceName "GET /trueasyncresume" + spanType DDSpanTypes.HTTP_SERVER + errored false + parent() + tags { + "$Tags.COMPONENT" "jax-rs" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + "$Tags.PEER_HOST_IPV4" "127.0.0.1" + "$Tags.PEER_PORT" Integer + "$Tags.HTTP_URL" "http://localhost:$port/trueasyncresume" + "$Tags.HTTP_HOSTNAME" "localhost" + "$Tags.HTTP_METHOD" "GET" + "$Tags.HTTP_STATUS" 200 + "$Tags.HTTP_ROUTE" String + "servlet.path" { it == null || it == "/trueasyncresume" } + "$Tags.HTTP_USER_AGENT" String + "$Tags.HTTP_CLIENT_IP" "127.0.0.1" + "$Tags.NETWORK_CLIENT_IP" "127.0.0.1" + withCustomIntegrationName("jetty-server") + defaultTags() + } + } + span { + operationName "jax-rs.request" + resourceName "TrueAsyncResumeResource.suspendThenResumeFromAnotherThread" + spanType DDSpanTypes.HTTP_SERVER + errored false + childOfPrevious() + tags { + "$Tags.COMPONENT" "jax-rs-controller" + defaultTags() + } + } + // Runs on the background thread, before resume() -- still correctly parented + // under the (still-open, cross-thread-propagated) jax-rs.request span. + TraceUtils.basicSpan(it, "trace.annotation", "TrueAsyncResumeResource.doWorkOnBackgroundThread", span(1), null, ["component": "trace"]) + } + } + } } diff --git a/dd-java-agent/instrumentation/cxf-2.1/src/test/java/AsyncCancelResource.java b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/AsyncCancelResource.java new file mode 100644 index 00000000000..742b105d16e --- /dev/null +++ b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/AsyncCancelResource.java @@ -0,0 +1,22 @@ +import datadog.trace.api.Trace; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.container.AsyncResponse; +import javax.ws.rs.container.Suspended; + +/** + * Same GH-12597 pattern as {@link AsyncResumeResource}, but exercising {@code + * AsyncResponse#cancel()} instead of {@code resume()} -- the third advice touched by the fix + * ({@code AsyncResponseCancelAdvice}). + */ +@Path("/asynccancel") +public class AsyncCancelResource { + @GET + public void cancelThenWork(@Suspended final AsyncResponse response) { + response.cancel(); + doWorkAfterCancel(); + } + + @Trace + private void doWorkAfterCancel() {} +} diff --git a/dd-java-agent/instrumentation/cxf-2.1/src/test/java/AsyncResumeResource.java b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/AsyncResumeResource.java new file mode 100644 index 00000000000..705a67ae38a --- /dev/null +++ b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/AsyncResumeResource.java @@ -0,0 +1,17 @@ +import datadog.trace.api.Trace; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.container.AsyncResponse; +import javax.ws.rs.container.Suspended; + +@Path("/asyncresume") +public class AsyncResumeResource { + @GET + public void resumeThenWork(@Suspended final AsyncResponse response) { + response.resume("OK"); + doWorkAfterResume(); + } + + @Trace + private void doWorkAfterResume() {} +} diff --git a/dd-java-agent/instrumentation/cxf-2.1/src/test/java/TrueAsyncResumeResource.java b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/TrueAsyncResumeResource.java new file mode 100644 index 00000000000..c8020db9f75 --- /dev/null +++ b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/TrueAsyncResumeResource.java @@ -0,0 +1,36 @@ +import datadog.trace.api.Trace; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.container.AsyncResponse; +import javax.ws.rs.container.Suspended; + +/** + * The "textbook" async pattern, for regression coverage alongside {@link AsyncResumeResource}: the + * resource method returns without resuming, and a different thread resumes it later. The GH-12597 + * fix must not change behavior on this path. + */ +@Path("/trueasyncresume") +public class TrueAsyncResumeResource { + + private static final ScheduledExecutorService EXECUTOR = Executors.newScheduledThreadPool(2); + + @GET + public void suspendThenResumeFromAnotherThread(@Suspended final AsyncResponse response) { + // A short delay ensures the request thread has genuinely returned/suspended before + // resume() is called, matching how a real background worker would behave (avoids a + // resume-before-suspend-completes race in the test transport). + EXECUTOR.schedule( + () -> { + doWorkOnBackgroundThread(); + response.resume("OK"); + }, + 50, + TimeUnit.MILLISECONDS); + } + + @Trace + private void doWorkOnBackgroundThread() {} +} diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java index 57d41ff0012..4cbb90f063a 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.jakarta3; +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.decorator.http.HttpResourceDecorator.HTTP_RESOURCE_DECORATOR; import datadog.trace.api.GenericClassValue; @@ -38,6 +39,56 @@ public class JakartaRsAnnotationsDecorator extends BaseDecorator { private static final ClassValue>> RESOURCE_NAMES = GenericClassValue.constructing(ConcurrentHashMap.class); + /** + * Counts, per thread, how many jakarta-rs annotated resource-method invocations are currently on + * the stack. Shared (via this helper class, injected for both instrumenters) between {@code + * JakartaRsAnnotationsInstrumentation}, which increments/decrements it around every resource + * method call, and {@code JakartaRsAsyncResponseInstrumentation}, which reads it to tell a + * synchronous {@code AsyncResponse#resume()}/{@code cancel()} call -- one nested inside the + * still-running resource method that owns the response -- apart from a genuinely asynchronous one + * on a different thread whose scope/span happen to have been propagated onto this thread (e.g. + * via an instrumented {@code ExecutorService}), which would otherwise look identical from {@code + * activeSpan()} alone. A plain {@code int[1]} avoids boxing on every call. + */ + private static final ThreadLocal ACTIVE_RESOURCE_METHOD_INVOCATIONS = new ThreadLocal<>(); + + public static void enterResourceMethod() { + int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); + if (counter == null) { + counter = new int[1]; + ACTIVE_RESOURCE_METHOD_INVOCATIONS.set(counter); + } + counter[0]++; + } + + public static void exitResourceMethod() { + final int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); + if (counter != null) { + counter[0]--; + } + } + + public static boolean isCurrentThreadInsideResourceMethod() { + final int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); + return counter != null && counter[0] > 0; + } + + /** + * True if {@code span} is both the currently active span on this thread and this thread + * is dynamically inside a jakarta-rs annotated resource-method invocation right now -- i.e. + * {@code resume()}/{@code cancel()} was called synchronously, nested inside the still-running + * resource method that owns this span, on this exact call stack. + * + *

Checking {@code activeSpan() == span} alone is not enough: an instrumented {@code + * ExecutorService} (or similar) can propagate a captured scope for this exact span onto a + * completely different, genuinely-asynchronous worker thread, which would otherwise look + * identical. That thread never entered a resource method, so {@link + * #isCurrentThreadInsideResourceMethod()} correctly returns {@code false} there. + */ + public static boolean isSynchronousResumeFromWithinResourceMethod(final AgentSpan span) { + return activeSpan() == span && isCurrentThreadInsideResourceMethod(); + } + @Override protected String[] instrumentationNames() { return new String[0]; diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsInstrumentation.java index c0c2ec9031c..5c0e578bafa 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsInstrumentation.java @@ -97,6 +97,7 @@ public static ContextScope nameSpan( @Advice.Origin final Method method, @Advice.AllArguments final Object[] args, @Advice.Local("asyncResponse") AsyncResponse asyncResponse) { + JakartaRsAnnotationsDecorator.enterResourceMethod(); ContextStore contextStore = null; for (final Object arg : args) { if (arg instanceof AsyncResponse) { @@ -138,6 +139,7 @@ public static void stopSpan( @Advice.Enter final ContextScope scope, @Advice.Thrown final Throwable throwable, @Advice.Local("asyncResponse") final AsyncResponse asyncResponse) { + JakartaRsAnnotationsDecorator.exitResourceMethod(); if (scope == null) { return; } diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAsyncResponseInstrumentation.java b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAsyncResponseInstrumentation.java index 1ae11e76090..bec555057a5 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAsyncResponseInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAsyncResponseInstrumentation.java @@ -3,6 +3,7 @@ import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; import static datadog.trace.instrumentation.jakarta3.JakartaRsAnnotationsDecorator.DECORATE; +import static datadog.trace.instrumentation.jakarta3.JakartaRsAnnotationsDecorator.isSynchronousResumeFromWithinResourceMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; @@ -74,8 +75,15 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { - contextStore.put(asyncResponse, null); DECORATE.onError(span, throwable); + if (isSynchronousResumeFromWithinResourceMethod(span)) { + // Let the resource method's own exit advice close the scope and finish the span + // (it will see asyncResponse.isSuspended() == false) instead of finishing it here, + // which would both double-finish the span and finish it prematurely while the + // resource method may still be doing work under it. + return; + } + contextStore.put(asyncResponse, null); DECORATE.beforeFinish(span); span.finish(); } @@ -94,8 +102,12 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { - contextStore.put(asyncResponse, null); DECORATE.onError(span, throwable); + if (isSynchronousResumeFromWithinResourceMethod(span)) { + // see comment in AsyncResponseAdvice#stopSpan + return; + } + contextStore.put(asyncResponse, null); DECORATE.beforeFinish(span); span.finish(); } @@ -113,12 +125,16 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { - contextStore.put(asyncResponse, null); if (throwable != null) { DECORATE.onError(span, throwable); } else { span.setTag("canceled", true); } + if (isSynchronousResumeFromWithinResourceMethod(span)) { + // see comment in AsyncResponseAdvice#stopSpan + return; + } + contextStore.put(asyncResponse, null); DECORATE.beforeFinish(span); span.finish(); } diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java index a7c8e82f2d8..96da2e765e6 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.jaxrs2; +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.decorator.http.HttpResourceDecorator.HTTP_RESOURCE_DECORATOR; import datadog.trace.api.GenericClassValue; @@ -37,6 +38,56 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { private static final ClassValue>> RESOURCE_NAMES = GenericClassValue.constructing(ConcurrentHashMap.class); + /** + * Counts, per thread, how many jax-rs annotated resource-method invocations are currently on the + * stack. Shared (via this helper class, injected for both instrumenters) between {@code + * JaxRsAnnotationsInstrumentation}, which increments/decrements it around every resource method + * call, and {@code JaxRsAsyncResponseInstrumentation}, which reads it to tell a synchronous + * {@code AsyncResponse#resume()}/{@code cancel()} call -- one nested inside the still-running + * resource method that owns the response -- apart from a genuinely asynchronous one on a + * different thread whose scope/span happen to have been propagated onto this thread (e.g. via an + * instrumented {@code ExecutorService}), which would otherwise look identical from {@code + * activeSpan()} alone. A plain {@code int[1]} avoids boxing on every call. + */ + private static final ThreadLocal ACTIVE_RESOURCE_METHOD_INVOCATIONS = new ThreadLocal<>(); + + public static void enterResourceMethod() { + int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); + if (counter == null) { + counter = new int[1]; + ACTIVE_RESOURCE_METHOD_INVOCATIONS.set(counter); + } + counter[0]++; + } + + public static void exitResourceMethod() { + final int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); + if (counter != null) { + counter[0]--; + } + } + + public static boolean isCurrentThreadInsideResourceMethod() { + final int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); + return counter != null && counter[0] > 0; + } + + /** + * True if {@code span} is both the currently active span on this thread and this thread + * is dynamically inside a jax-rs annotated resource-method invocation right now -- i.e. {@code + * resume()}/{@code cancel()} was called synchronously, nested inside the still-running resource + * method that owns this span, on this exact call stack. + * + *

Checking {@code activeSpan() == span} alone is not enough: an instrumented {@code + * ExecutorService} (or similar) can propagate a captured scope for this exact span onto a + * completely different, genuinely-asynchronous worker thread, which would otherwise look + * identical. That thread never entered a resource method, so {@link + * #isCurrentThreadInsideResourceMethod()} correctly returns {@code false} there. + */ + public static boolean isSynchronousResumeFromWithinResourceMethod(final AgentSpan span) { + return activeSpan() == span && isCurrentThreadInsideResourceMethod(); + } + @Override protected String[] instrumentationNames() { return new String[0]; diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java index 4f7605a508e..219547edf6b 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java @@ -105,6 +105,7 @@ public static ContextScope nameSpan( @Advice.Origin final Method method, @Advice.AllArguments final Object[] args, @Advice.Local("asyncResponse") AsyncResponse asyncResponse) { + JaxRsAnnotationsDecorator.enterResourceMethod(); ContextStore contextStore = null; for (final Object arg : args) { if (arg instanceof AsyncResponse) { @@ -145,6 +146,7 @@ public static void stopSpan( @Advice.Enter final ContextScope scope, @Advice.Thrown final Throwable throwable, @Advice.Local("asyncResponse") final AsyncResponse asyncResponse) { + JaxRsAnnotationsDecorator.exitResourceMethod(); if (scope == null) { return; } diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAsyncResponseInstrumentation.java b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAsyncResponseInstrumentation.java index 9972a720d8c..5d0e530b0c4 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAsyncResponseInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAsyncResponseInstrumentation.java @@ -3,6 +3,7 @@ import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; import static datadog.trace.instrumentation.jaxrs2.JaxRsAnnotationsDecorator.DECORATE; +import static datadog.trace.instrumentation.jaxrs2.JaxRsAnnotationsDecorator.isSynchronousResumeFromWithinResourceMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; @@ -74,8 +75,15 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { - contextStore.put(asyncResponse, null); DECORATE.onError(span, throwable); + if (isSynchronousResumeFromWithinResourceMethod(span)) { + // Let the resource method's own exit advice close the scope and finish the span + // (it will see asyncResponse.isSuspended() == false) instead of finishing it here, + // which would both double-finish the span and finish it prematurely while the + // resource method may still be doing work under it. + return; + } + contextStore.put(asyncResponse, null); DECORATE.beforeFinish(span); span.finish(); } @@ -94,8 +102,12 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { - contextStore.put(asyncResponse, null); DECORATE.onError(span, throwable); + if (isSynchronousResumeFromWithinResourceMethod(span)) { + // see comment in AsyncResponseAdvice#stopSpan + return; + } + contextStore.put(asyncResponse, null); DECORATE.beforeFinish(span); span.finish(); } @@ -113,12 +125,16 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { - contextStore.put(asyncResponse, null); if (throwable != null) { DECORATE.onError(span, throwable); } else { span.setTag("canceled", true); } + if (isSynchronousResumeFromWithinResourceMethod(span)) { + // see comment in AsyncResponseAdvice#stopSpan + return; + } + contextStore.put(asyncResponse, null); DECORATE.beforeFinish(span); span.finish(); } From 1a62c3aeecfa8c7d7ee0a32f036e78bf417438f7 Mon Sep 17 00:00:00 2001 From: Yeison Casado Date: Fri, 25 Sep 2026 11:23:37 -0400 Subject: [PATCH 2/2] Track resource-method spans on a bootstrap stack instead of a per-thread 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. --- .../bootstrap/ResourceMethodSpanTracker.java | 53 +++++ .../groovy/CxfContextPropagationTest.groovy | 68 +++++- .../src/test/java/NestedResumeResource.java | 25 +++ .../test/java/TrueAsyncResumeResource.java | 25 ++- .../jakarta-rs-annotations-3.0/build.gradle | 1 + .../JakartaRsAnnotationsDecorator.java | 51 ----- .../JakartaRsAnnotationsInstrumentation.java | 15 +- ...JakartaRsAsyncResponseInstrumentation.java | 19 +- ...aRsAsyncResponseInstrumentationTest.groovy | 204 ++++++++++++++++++ .../src/test/java/FakeAsyncResponse.java | 102 +++++++++ .../jaxrs2/JaxRsAnnotationsDecorator.java | 51 ----- .../JaxRsAnnotationsInstrumentation.java | 15 +- .../JaxRsAsyncResponseInstrumentation.java | 19 +- 13 files changed, 516 insertions(+), 132 deletions(-) create mode 100644 dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/ResourceMethodSpanTracker.java create mode 100644 dd-java-agent/instrumentation/cxf-2.1/src/test/java/NestedResumeResource.java create mode 100644 dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/groovy/JakartaRsAsyncResponseInstrumentationTest.groovy create mode 100644 dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/java/FakeAsyncResponse.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/ResourceMethodSpanTracker.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/ResourceMethodSpanTracker.java new file mode 100644 index 00000000000..cbf94267328 --- /dev/null +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/ResourceMethodSpanTracker.java @@ -0,0 +1,53 @@ +package datadog.trace.bootstrap; + +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import java.util.ArrayDeque; + +/** + * Tracks, per thread, which annotated resource-method invocation (identified by its span) is + * currently the innermost one still on the call stack -- i.e. whose exit advice has not yet run. + * + *

Used by the JAX-RS/Jakarta-RS instrumentation to tell a synchronous {@code + * AsyncResponse#resume()}/{@code cancel()} call -- one nested inside the still-running resource + * method that owns the response -- apart from a genuinely asynchronous one called later, from a + * different thread or after an intervening unrelated instrumented call. A generic "is any resource + * method open on this thread" counter is not enough for that: it can't tell one resource method's + * invocation apart from another's on a shared thread pool, and it can't see past a nested + * instrumented call (e.g. a {@code @Trace}-annotated helper) that becomes the current active span + * without popping this stack. Comparing the specific span object against the top of this stack + * answers the exact question that matters, without either failure mode. + * + *

Deliberately bootstrap-loaded (like {@link CallDepthThreadLocalMap}) rather than living on a + * per-instrumentation helper class: helper classes are injected once per target classloader, so a + * container that loads the resource-method advice and the AsyncResponse advice into different + * classloaders (e.g. a modular server where the JAX-RS runtime and the deployed application are in + * separate classloaders) would otherwise give each advice its own, disconnected copy of this state. + */ +public final class ResourceMethodSpanTracker { + + private static final ThreadLocal> STACK = new ThreadLocal<>(); + + private ResourceMethodSpanTracker() {} + + public static void enter(final AgentSpan span) { + ArrayDeque stack = STACK.get(); + if (stack == null) { + stack = new ArrayDeque<>(4); + STACK.set(stack); + } + stack.push(span); + } + + public static void exit() { + final ArrayDeque stack = STACK.get(); + if (stack != null) { + stack.pop(); + } + } + + /** True if {@code span} is the innermost still-open resource-method invocation on this thread. */ + public static boolean isInnermost(final AgentSpan span) { + final ArrayDeque stack = STACK.get(); + return stack != null && stack.peek() == span; + } +} diff --git a/dd-java-agent/instrumentation/cxf-2.1/src/test/groovy/CxfContextPropagationTest.groovy b/dd-java-agent/instrumentation/cxf-2.1/src/test/groovy/CxfContextPropagationTest.groovy index 49682dfe8e7..c17215be81d 100644 --- a/dd-java-agent/instrumentation/cxf-2.1/src/test/groovy/CxfContextPropagationTest.groovy +++ b/dd-java-agent/instrumentation/cxf-2.1/src/test/groovy/CxfContextPropagationTest.groovy @@ -22,7 +22,7 @@ class CxfContextPropagationTest extends InstrumentationSpecification { @Override void setupSpec() { JAXRSServerFactoryBean sf = new JAXRSServerFactoryBean() - sf.setResourceClasses(TestResource, AsyncResumeResource, TrueAsyncResumeResource, AsyncCancelResource) + sf.setResourceClasses(TestResource, AsyncResumeResource, TrueAsyncResumeResource, AsyncCancelResource, NestedResumeResource) List providers = [new TestExceptionMapper()] sf.setProviders(providers) @@ -34,6 +34,8 @@ class CxfContextPropagationTest extends InstrumentationSpecification { new SingletonResourceProvider(new TrueAsyncResumeResource(), true)) sf.setResourceProvider(AsyncCancelResource, new SingletonResourceProvider(new AsyncCancelResource(), true)) + sf.setResourceProvider(NestedResumeResource, + new SingletonResourceProvider(new NestedResumeResource(), true)) sf.setAddress("http://localhost:0") server = sf.create() @@ -295,4 +297,68 @@ class CxfContextPropagationTest extends InstrumentationSpecification { } } } + + def "resume() called synchronously from a nested @Trace helper finishes the span only once"() { + // Regression test for a gap found reviewing the fix for GH-12597: resume() is called + // synchronously, but from a @Trace-annotated helper method rather than directly from + // the resource method's own body. At that moment, the *helper's* span is the active + // one on this thread, not the resource method's -- checking activeSpan() against the + // resource method's span directly (an earlier version of this fix) would wrongly treat + // this as a genuinely-async resume and finish the span right there, then finish it + // again when the resource method itself returns (caught by enabledFinishTimingChecks()). + setup: + def client = OkHttpUtils.client() + when: + def response = client.newCall(new Request.Builder() + .url("http://localhost:$port/nestedresume") + .get().build()).execute() + then: + assert response.code() == 200 + assert response.body().string() == "OK" + + assertTraces(1) { + trace(3) { + sortSpansByStart() + span { + operationName "servlet.request" + resourceName "GET /nestedresume" + spanType DDSpanTypes.HTTP_SERVER + errored false + parent() + tags { + "$Tags.COMPONENT" "jax-rs" + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + "$Tags.PEER_HOST_IPV4" "127.0.0.1" + "$Tags.PEER_PORT" Integer + "$Tags.HTTP_URL" "http://localhost:$port/nestedresume" + "$Tags.HTTP_HOSTNAME" "localhost" + "$Tags.HTTP_METHOD" "GET" + "$Tags.HTTP_STATUS" 200 + "$Tags.HTTP_ROUTE" String + "servlet.path" { it == null || it == "/nestedresume" } + "$Tags.HTTP_USER_AGENT" String + "$Tags.HTTP_CLIENT_IP" "127.0.0.1" + "$Tags.NETWORK_CLIENT_IP" "127.0.0.1" + withCustomIntegrationName("jetty-server") + defaultTags() + } + } + span { + operationName "jax-rs.request" + resourceName "NestedResumeResource.resumeViaHelper" + spanType DDSpanTypes.HTTP_SERVER + errored false + childOfPrevious() + tags { + "$Tags.COMPONENT" "jax-rs-controller" + defaultTags() + } + } + // The helper that actually calls resume() -- still parented under jax-rs.request, + // proving the resource-method span wasn't finished/popped while the helper (and + // resume() inside it) was still running. + TraceUtils.basicSpan(it, "trace.annotation", "NestedResumeResource.resumeFromHelper", span(1), null, ["component": "trace"]) + } + } + } } diff --git a/dd-java-agent/instrumentation/cxf-2.1/src/test/java/NestedResumeResource.java b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/NestedResumeResource.java new file mode 100644 index 00000000000..e3cebedd916 --- /dev/null +++ b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/NestedResumeResource.java @@ -0,0 +1,25 @@ +import datadog.trace.api.Trace; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.container.AsyncResponse; +import javax.ws.rs.container.Suspended; + +/** + * Regression coverage for a gap found in code review of GH-12597's fix: {@code resume()} called + * from a {@code @Trace}-annotated helper, not directly from the resource method's own body. At the + * moment {@code resume()} runs, the helper's own span is the active one, not the resource method's + * -- an early version of the fix compared {@code activeSpan()} directly against the resource + * method's span and wrongly treated this as a genuinely-async call. + */ +@Path("/nestedresume") +public class NestedResumeResource { + @GET + public void resumeViaHelper(@Suspended final AsyncResponse response) { + resumeFromHelper(response); + } + + @Trace + private void resumeFromHelper(final AsyncResponse response) { + response.resume("OK"); + } +} diff --git a/dd-java-agent/instrumentation/cxf-2.1/src/test/java/TrueAsyncResumeResource.java b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/TrueAsyncResumeResource.java index c8020db9f75..d26501de069 100644 --- a/dd-java-agent/instrumentation/cxf-2.1/src/test/java/TrueAsyncResumeResource.java +++ b/dd-java-agent/instrumentation/cxf-2.1/src/test/java/TrueAsyncResumeResource.java @@ -1,6 +1,6 @@ import datadog.trace.api.Trace; +import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import javax.ws.rs.GET; import javax.ws.rs.Path; @@ -15,20 +15,27 @@ @Path("/trueasyncresume") public class TrueAsyncResumeResource { - private static final ScheduledExecutorService EXECUTOR = Executors.newScheduledThreadPool(2); + private static final ExecutorService EXECUTOR = Executors.newFixedThreadPool(2); @GET public void suspendThenResumeFromAnotherThread(@Suspended final AsyncResponse response) { - // A short delay ensures the request thread has genuinely returned/suspended before - // resume() is called, matching how a real background worker would behave (avoids a - // resume-before-suspend-completes race in the test transport). - EXECUTOR.schedule( + EXECUTOR.submit( () -> { + try { + // Wait for the actual condition (the container has genuinely suspended the + // response) instead of guessing a fixed delay, so this can't race under a slow + // or overloaded test runner. + final long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (!response.isSuspended() && System.nanoTime() < deadline) { + Thread.sleep(1); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } doWorkOnBackgroundThread(); response.resume("OK"); - }, - 50, - TimeUnit.MILLISECONDS); + }); } @Trace diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/build.gradle b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/build.gradle index 516456dd2da..ef80cd6480e 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/build.gradle +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/build.gradle @@ -31,6 +31,7 @@ dependencies { testImplementation project(':dd-java-agent:instrumentation:servlet:javax-servlet:javax-servlet-3.0') testImplementation group: 'jakarta.ws.rs', name: 'jakarta.ws.rs-api', version: '3.0.0' testImplementation group: 'jakarta.xml.bind', name: 'jakarta.xml.bind-api', version: '3.0.0' + testRuntimeOnly project(':dd-java-agent:instrumentation:datadog:tracing:trace-annotation') latestDepTestImplementation group: 'jakarta.ws.rs', name: 'jakarta.ws.rs-api', version: '3.0.+' latestDepTestImplementation group: 'jakarta.xml.bind', name: 'jakarta.xml.bind-api', version: '3.0.+' diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java index 4cbb90f063a..57d41ff0012 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsDecorator.java @@ -1,6 +1,5 @@ package datadog.trace.instrumentation.jakarta3; -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.decorator.http.HttpResourceDecorator.HTTP_RESOURCE_DECORATOR; import datadog.trace.api.GenericClassValue; @@ -39,56 +38,6 @@ public class JakartaRsAnnotationsDecorator extends BaseDecorator { private static final ClassValue>> RESOURCE_NAMES = GenericClassValue.constructing(ConcurrentHashMap.class); - /** - * Counts, per thread, how many jakarta-rs annotated resource-method invocations are currently on - * the stack. Shared (via this helper class, injected for both instrumenters) between {@code - * JakartaRsAnnotationsInstrumentation}, which increments/decrements it around every resource - * method call, and {@code JakartaRsAsyncResponseInstrumentation}, which reads it to tell a - * synchronous {@code AsyncResponse#resume()}/{@code cancel()} call -- one nested inside the - * still-running resource method that owns the response -- apart from a genuinely asynchronous one - * on a different thread whose scope/span happen to have been propagated onto this thread (e.g. - * via an instrumented {@code ExecutorService}), which would otherwise look identical from {@code - * activeSpan()} alone. A plain {@code int[1]} avoids boxing on every call. - */ - private static final ThreadLocal ACTIVE_RESOURCE_METHOD_INVOCATIONS = new ThreadLocal<>(); - - public static void enterResourceMethod() { - int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); - if (counter == null) { - counter = new int[1]; - ACTIVE_RESOURCE_METHOD_INVOCATIONS.set(counter); - } - counter[0]++; - } - - public static void exitResourceMethod() { - final int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); - if (counter != null) { - counter[0]--; - } - } - - public static boolean isCurrentThreadInsideResourceMethod() { - final int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); - return counter != null && counter[0] > 0; - } - - /** - * True if {@code span} is both the currently active span on this thread and this thread - * is dynamically inside a jakarta-rs annotated resource-method invocation right now -- i.e. - * {@code resume()}/{@code cancel()} was called synchronously, nested inside the still-running - * resource method that owns this span, on this exact call stack. - * - *

Checking {@code activeSpan() == span} alone is not enough: an instrumented {@code - * ExecutorService} (or similar) can propagate a captured scope for this exact span onto a - * completely different, genuinely-asynchronous worker thread, which would otherwise look - * identical. That thread never entered a resource method, so {@link - * #isCurrentThreadInsideResourceMethod()} correctly returns {@code false} there. - */ - public static boolean isSynchronousResumeFromWithinResourceMethod(final AgentSpan span) { - return activeSpan() == span && isCurrentThreadInsideResourceMethod(); - } - @Override protected String[] instrumentationNames() { return new String[0]; diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsInstrumentation.java index 5c0e578bafa..f01f56ded1f 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAnnotationsInstrumentation.java @@ -7,6 +7,8 @@ import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.isAnnotatedWith; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedOneOf; +import static datadog.trace.bootstrap.ResourceMethodSpanTracker.enter; +import static datadog.trace.bootstrap.ResourceMethodSpanTracker.exit; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan; @@ -97,7 +99,6 @@ public static ContextScope nameSpan( @Advice.Origin final Method method, @Advice.AllArguments final Object[] args, @Advice.Local("asyncResponse") AsyncResponse asyncResponse) { - JakartaRsAnnotationsDecorator.enterResourceMethod(); ContextStore contextStore = null; for (final Object arg : args) { if (arg instanceof AsyncResponse) { @@ -129,6 +130,9 @@ public static ContextScope nameSpan( if (contextStore != null && asyncResponse != null) { contextStore.put(asyncResponse, span); + // Only tracked for methods that can hand off to AsyncResponse#resume()/cancel(); + // see ResourceMethodSpanTracker for why this can't just be a bare counter. + enter(span); } return scope; @@ -139,12 +143,19 @@ public static void stopSpan( @Advice.Enter final ContextScope scope, @Advice.Thrown final Throwable throwable, @Advice.Local("asyncResponse") final AsyncResponse asyncResponse) { - JakartaRsAnnotationsDecorator.exitResourceMethod(); if (scope == null) { return; } + if (asyncResponse != null) { + exit(); + } final AgentSpan span = spanFromScope(scope); if (throwable != null) { + if (asyncResponse != null) { + // Clear span from the asyncResponse so a later resume()/cancel() call (e.g. from + // container exception-mapping) doesn't find a stale mapping and double-finish it. + InstrumentationContext.get(AsyncResponse.class, AgentSpan.class).put(asyncResponse, null); + } DECORATE.onError(span, throwable); DECORATE.beforeFinish(span); scope.close(); diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAsyncResponseInstrumentation.java b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAsyncResponseInstrumentation.java index bec555057a5..9f58e4e29c7 100644 --- a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAsyncResponseInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/main/java/datadog/trace/instrumentation/jakarta3/JakartaRsAsyncResponseInstrumentation.java @@ -2,8 +2,8 @@ import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static datadog.trace.bootstrap.ResourceMethodSpanTracker.isInnermost; import static datadog.trace.instrumentation.jakarta3.JakartaRsAnnotationsDecorator.DECORATE; -import static datadog.trace.instrumentation.jakarta3.JakartaRsAnnotationsDecorator.isSynchronousResumeFromWithinResourceMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; @@ -76,11 +76,14 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { DECORATE.onError(span, throwable); - if (isSynchronousResumeFromWithinResourceMethod(span)) { - // Let the resource method's own exit advice close the scope and finish the span - // (it will see asyncResponse.isSuspended() == false) instead of finishing it here, - // which would both double-finish the span and finish it prematurely while the - // resource method may still be doing work under it. + if (isInnermost(span)) { + // resume()/cancel() was called synchronously, nested inside the still-running + // resource method that owns this span (ResourceMethodSpanTracker.isInnermost(span) + // says its invocation is still the innermost open one on this thread). Let that + // method's own exit advice close the scope and finish the span (it will see + // asyncResponse.isSuspended() == false) instead of finishing it here, which would + // both double-finish the span and finish it prematurely while the resource method + // may still be doing work under it. return; } contextStore.put(asyncResponse, null); @@ -103,7 +106,7 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { DECORATE.onError(span, throwable); - if (isSynchronousResumeFromWithinResourceMethod(span)) { + if (isInnermost(span)) { // see comment in AsyncResponseAdvice#stopSpan return; } @@ -130,7 +133,7 @@ public static void stopSpan( } else { span.setTag("canceled", true); } - if (isSynchronousResumeFromWithinResourceMethod(span)) { + if (isInnermost(span)) { // see comment in AsyncResponseAdvice#stopSpan return; } diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/groovy/JakartaRsAsyncResponseInstrumentationTest.groovy b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/groovy/JakartaRsAsyncResponseInstrumentationTest.groovy new file mode 100644 index 00000000000..b4fa8e036e4 --- /dev/null +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/groovy/JakartaRsAsyncResponseInstrumentationTest.groovy @@ -0,0 +1,204 @@ +import datadog.trace.agent.test.InstrumentationSpecification +import datadog.trace.agent.test.utils.TraceUtils +import datadog.trace.api.Trace +import datadog.trace.bootstrap.instrumentation.api.Tags +import jakarta.ws.rs.GET +import jakarta.ws.rs.Path +import jakarta.ws.rs.container.AsyncResponse +import jakarta.ws.rs.container.Suspended + +import java.util.concurrent.ExecutorService +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit + +/** + * Regression coverage for GH-12597 (jax-rs.request span double-finish / premature finish on + * synchronous AsyncResponse#resume()/cancel()) directly against the jakarta.ws.rs advice, with no + * real JAX-RS server involved -- resource methods are invoked directly, exactly like the existing + * JakartaRsAnnotations3InstrumentationTest does. This exercises the same advice code as the + * cxf-2.1 module's CxfContextPropagationTest (which only covers the javax.ws.rs path). + */ +class JakartaRsAsyncResponseInstrumentationTest extends InstrumentationSpecification { + + @Override + protected boolean enabledFinishTimingChecks() { + // Fails the test with the exact "finished more than once" stack traces if a jax-rs.request + // span is ever finished twice -- see CxfContextPropagationTest for the same guard. + return true + } + + def "resume() called synchronously from within the resource method finishes the span only once"() { + setup: + def response = new FakeAsyncResponse() + + when: + new AsyncResumeResource().resumeThenWork(response) + + then: + assertTraces(1) { + trace(2) { + sortSpansByStart() + span { + operationName "jakarta-rs.request" + resourceName "GET /asyncresume" + spanType "web" + errored false + parent() + tags { + "$Tags.COMPONENT" "jakarta-rs-controller" + "$Tags.HTTP_ROUTE" "/asyncresume" + defaultTags() + } + } + // Still parented under jakarta-rs.request: proves the span wasn't finished (and its + // scope wasn't popped) by resume() itself, before the resource method returned. + TraceUtils.basicSpan(it, "trace.annotation", "AsyncResumeResource.doWorkAfterResume", span(0), null, ["component": "trace"]) + } + } + } + + def "cancel() called synchronously from within the resource method finishes the span only once"() { + setup: + def response = new FakeAsyncResponse() + + when: + new AsyncCancelResource().cancelThenWork(response) + + then: + assertTraces(1) { + trace(2) { + sortSpansByStart() + span { + operationName "jakarta-rs.request" + resourceName "GET /asynccancel" + spanType "web" + errored false + parent() + tags { + "$Tags.COMPONENT" "jakarta-rs-controller" + "$Tags.HTTP_ROUTE" "/asynccancel" + "canceled" true + defaultTags() + } + } + TraceUtils.basicSpan(it, "trace.annotation", "AsyncCancelResource.doWorkAfterCancel", span(0), null, ["component": "trace"]) + } + } + } + + def "resume() called synchronously from a nested @Trace helper finishes the span only once"() { + // The gap found in code review: resume() called from a @Trace-annotated helper, not + // directly from the resource method's own body. activeSpan() at that moment is the + // helper's span, not the resource method's. + setup: + def response = new FakeAsyncResponse() + + when: + new NestedResumeResource().resumeViaHelper(response) + + then: + assertTraces(1) { + trace(2) { + sortSpansByStart() + span { + operationName "jakarta-rs.request" + resourceName "GET /nestedresume" + spanType "web" + errored false + parent() + tags { + "$Tags.COMPONENT" "jakarta-rs-controller" + "$Tags.HTTP_ROUTE" "/nestedresume" + defaultTags() + } + } + TraceUtils.basicSpan(it, "trace.annotation", "NestedResumeResource.resumeFromHelper", span(0), null, ["component": "trace"]) + } + } + } + + def "resume() called from a genuinely different thread is unaffected"() { + setup: + def response = new FakeAsyncResponse() + + when: + new TrueAsyncResumeResource().suspendThenResumeFromAnotherThread(response) + + then: + assertTraces(1) { + trace(2) { + sortSpansByStart() + span { + operationName "jakarta-rs.request" + resourceName "GET /trueasyncresume" + spanType "web" + errored false + parent() + tags { + "$Tags.COMPONENT" "jakarta-rs-controller" + "$Tags.HTTP_ROUTE" "/trueasyncresume" + defaultTags() + } + } + TraceUtils.basicSpan(it, "trace.annotation", "TrueAsyncResumeResource.doWorkOnBackgroundThread", span(0), null, ["component": "trace"]) + } + } + } + + @Path("/asyncresume") + static class AsyncResumeResource { + @GET + void resumeThenWork(@Suspended final AsyncResponse response) { + response.resume("OK") + doWorkAfterResume() + } + + @Trace + private void doWorkAfterResume() {} + } + + @Path("/asynccancel") + static class AsyncCancelResource { + @GET + void cancelThenWork(@Suspended final AsyncResponse response) { + response.cancel() + doWorkAfterCancel() + } + + @Trace + private void doWorkAfterCancel() {} + } + + @Path("/nestedresume") + static class NestedResumeResource { + @GET + void resumeViaHelper(@Suspended final AsyncResponse response) { + resumeFromHelper(response) + } + + @Trace + private void resumeFromHelper(final AsyncResponse response) { + response.resume("OK") + } + } + + @Path("/trueasyncresume") + static class TrueAsyncResumeResource { + private static final ExecutorService EXECUTOR = Executors.newFixedThreadPool(2) + + @GET + void suspendThenResumeFromAnotherThread(@Suspended final AsyncResponse response) { + EXECUTOR.submit({ + def deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5) + while (!response.isSuspended() && System.nanoTime() < deadline) { + Thread.sleep(1) + } + doWorkOnBackgroundThread() + response.resume("OK") + }) + } + + @Trace + private void doWorkOnBackgroundThread() {} + } +} diff --git a/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/java/FakeAsyncResponse.java b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/java/FakeAsyncResponse.java new file mode 100644 index 00000000000..c9770551c50 --- /dev/null +++ b/dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/src/test/java/FakeAsyncResponse.java @@ -0,0 +1,102 @@ +import jakarta.ws.rs.container.AsyncResponse; +import jakarta.ws.rs.container.TimeoutHandler; +import java.util.Collection; +import java.util.Collections; +import java.util.Date; +import java.util.Map; +import java.util.concurrent.TimeUnit; + +/** + * Minimal {@link AsyncResponse} implementation for testing the + * JakartaRsAsyncResponseInstrumentation advice directly (no real JAX-RS container/server involved) + * -- only {@code resume}/{@code cancel}/{@code isSuspended} carry real semantics; everything else + * is a no-op. + */ +public class FakeAsyncResponse implements AsyncResponse { + + private boolean suspended = true; + private boolean cancelled = false; + + @Override + public boolean resume(final Object response) { + if (!suspended) { + return false; + } + suspended = false; + return true; + } + + @Override + public boolean resume(final Throwable response) { + if (!suspended) { + return false; + } + suspended = false; + return true; + } + + @Override + public boolean cancel() { + if (!suspended) { + return false; + } + suspended = false; + cancelled = true; + return true; + } + + @Override + public boolean cancel(final int retryAfter) { + return cancel(); + } + + @Override + public boolean cancel(final Date retryAfter) { + return cancel(); + } + + @Override + public boolean isSuspended() { + return suspended; + } + + @Override + public boolean isCancelled() { + return cancelled; + } + + @Override + public boolean isDone() { + return !suspended; + } + + @Override + public boolean setTimeout(final long time, final TimeUnit unit) { + return true; + } + + @Override + public void setTimeoutHandler(final TimeoutHandler handler) {} + + @Override + public Collection> register(final Class callback) { + return Collections.emptyList(); + } + + @Override + public Map, Collection>> register( + final Class callback, final Class... callbacks) { + return Collections.emptyMap(); + } + + @Override + public Collection> register(final Object callback) { + return Collections.emptyList(); + } + + @Override + public Map, Collection>> register( + final Object callback, final Object... callbacks) { + return Collections.emptyMap(); + } +} diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java index 96da2e765e6..a7c8e82f2d8 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsDecorator.java @@ -1,6 +1,5 @@ package datadog.trace.instrumentation.jaxrs2; -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.decorator.http.HttpResourceDecorator.HTTP_RESOURCE_DECORATOR; import datadog.trace.api.GenericClassValue; @@ -38,56 +37,6 @@ public class JaxRsAnnotationsDecorator extends BaseDecorator { private static final ClassValue>> RESOURCE_NAMES = GenericClassValue.constructing(ConcurrentHashMap.class); - /** - * Counts, per thread, how many jax-rs annotated resource-method invocations are currently on the - * stack. Shared (via this helper class, injected for both instrumenters) between {@code - * JaxRsAnnotationsInstrumentation}, which increments/decrements it around every resource method - * call, and {@code JaxRsAsyncResponseInstrumentation}, which reads it to tell a synchronous - * {@code AsyncResponse#resume()}/{@code cancel()} call -- one nested inside the still-running - * resource method that owns the response -- apart from a genuinely asynchronous one on a - * different thread whose scope/span happen to have been propagated onto this thread (e.g. via an - * instrumented {@code ExecutorService}), which would otherwise look identical from {@code - * activeSpan()} alone. A plain {@code int[1]} avoids boxing on every call. - */ - private static final ThreadLocal ACTIVE_RESOURCE_METHOD_INVOCATIONS = new ThreadLocal<>(); - - public static void enterResourceMethod() { - int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); - if (counter == null) { - counter = new int[1]; - ACTIVE_RESOURCE_METHOD_INVOCATIONS.set(counter); - } - counter[0]++; - } - - public static void exitResourceMethod() { - final int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); - if (counter != null) { - counter[0]--; - } - } - - public static boolean isCurrentThreadInsideResourceMethod() { - final int[] counter = ACTIVE_RESOURCE_METHOD_INVOCATIONS.get(); - return counter != null && counter[0] > 0; - } - - /** - * True if {@code span} is both the currently active span on this thread and this thread - * is dynamically inside a jax-rs annotated resource-method invocation right now -- i.e. {@code - * resume()}/{@code cancel()} was called synchronously, nested inside the still-running resource - * method that owns this span, on this exact call stack. - * - *

Checking {@code activeSpan() == span} alone is not enough: an instrumented {@code - * ExecutorService} (or similar) can propagate a captured scope for this exact span onto a - * completely different, genuinely-asynchronous worker thread, which would otherwise look - * identical. That thread never entered a resource method, so {@link - * #isCurrentThreadInsideResourceMethod()} correctly returns {@code false} there. - */ - public static boolean isSynchronousResumeFromWithinResourceMethod(final AgentSpan span) { - return activeSpan() == span && isCurrentThreadInsideResourceMethod(); - } - @Override protected String[] instrumentationNames() { return new String[0]; diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java index 219547edf6b..cb57a5ba42b 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java @@ -8,6 +8,8 @@ import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.isAnnotatedWith; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedOneOf; +import static datadog.trace.bootstrap.ResourceMethodSpanTracker.enter; +import static datadog.trace.bootstrap.ResourceMethodSpanTracker.exit; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan; @@ -105,7 +107,6 @@ public static ContextScope nameSpan( @Advice.Origin final Method method, @Advice.AllArguments final Object[] args, @Advice.Local("asyncResponse") AsyncResponse asyncResponse) { - JaxRsAnnotationsDecorator.enterResourceMethod(); ContextStore contextStore = null; for (final Object arg : args) { if (arg instanceof AsyncResponse) { @@ -136,6 +137,9 @@ public static ContextScope nameSpan( if (contextStore != null && asyncResponse != null) { contextStore.put(asyncResponse, span); + // Only tracked for methods that can hand off to AsyncResponse#resume()/cancel(); + // see ResourceMethodSpanTracker for why this can't just be a bare counter. + enter(span); } return scope; @@ -146,12 +150,19 @@ public static void stopSpan( @Advice.Enter final ContextScope scope, @Advice.Thrown final Throwable throwable, @Advice.Local("asyncResponse") final AsyncResponse asyncResponse) { - JaxRsAnnotationsDecorator.exitResourceMethod(); if (scope == null) { return; } + if (asyncResponse != null) { + exit(); + } final AgentSpan span = spanFromScope(scope); if (throwable != null) { + if (asyncResponse != null) { + // Clear span from the asyncResponse so a later resume()/cancel() call (e.g. from + // container exception-mapping) doesn't find a stale mapping and double-finish it. + InstrumentationContext.get(AsyncResponse.class, AgentSpan.class).put(asyncResponse, null); + } DECORATE.onError(span, throwable); DECORATE.beforeFinish(span); scope.close(); diff --git a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAsyncResponseInstrumentation.java b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAsyncResponseInstrumentation.java index 5d0e530b0c4..0a308de91cf 100644 --- a/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAsyncResponseInstrumentation.java +++ b/dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAsyncResponseInstrumentation.java @@ -2,8 +2,8 @@ import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static datadog.trace.bootstrap.ResourceMethodSpanTracker.isInnermost; import static datadog.trace.instrumentation.jaxrs2.JaxRsAnnotationsDecorator.DECORATE; -import static datadog.trace.instrumentation.jaxrs2.JaxRsAnnotationsDecorator.isSynchronousResumeFromWithinResourceMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; @@ -76,11 +76,14 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { DECORATE.onError(span, throwable); - if (isSynchronousResumeFromWithinResourceMethod(span)) { - // Let the resource method's own exit advice close the scope and finish the span - // (it will see asyncResponse.isSuspended() == false) instead of finishing it here, - // which would both double-finish the span and finish it prematurely while the - // resource method may still be doing work under it. + if (isInnermost(span)) { + // resume()/cancel() was called synchronously, nested inside the still-running + // resource method that owns this span (ResourceMethodSpanTracker.isInnermost(span) + // says its invocation is still the innermost open one on this thread). Let that + // method's own exit advice close the scope and finish the span (it will see + // asyncResponse.isSuspended() == false) instead of finishing it here, which would + // both double-finish the span and finish it prematurely while the resource method + // may still be doing work under it. return; } contextStore.put(asyncResponse, null); @@ -103,7 +106,7 @@ public static void stopSpan( final AgentSpan span = contextStore.get(asyncResponse); if (span != null) { DECORATE.onError(span, throwable); - if (isSynchronousResumeFromWithinResourceMethod(span)) { + if (isInnermost(span)) { // see comment in AsyncResponseAdvice#stopSpan return; } @@ -130,7 +133,7 @@ public static void stopSpan( } else { span.setTag("canceled", true); } - if (isSynchronousResumeFromWithinResourceMethod(span)) { + if (isInnermost(span)) { // see comment in AsyncResponseAdvice#stopSpan return; }