From 04c2d1a1060473609303ccc49429346ea5beb152 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Wed, 23 Sep 2026 15:47:56 +0200 Subject: [PATCH 1/2] Finish HTTP client spans and close scopes when AppSec blocks requests --- .../apachehttpclient/HelperMethods.java | 16 +++- .../ApacheHttpClientBlockingForkedTest.groovy | 80 ++++++++++++++++++ .../CommonsHttpClientInstrumentation.java | 13 ++- ...CommonsHttpClientBlockingForkedTest.groovy | 70 ++++++++++++++++ .../httpclient/SendAdvice.java | 13 ++- .../httpclient/SendAsyncAdvice.java | 13 ++- .../JavaHttpClientBlockingForkedTest.groovy | 82 +++++++++++++++++++ 7 files changed, 282 insertions(+), 5 deletions(-) create mode 100644 dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/test/groovy/ApacheHttpClientBlockingForkedTest.groovy create mode 100644 dd-java-agent/instrumentation/commons-httpclient-2.0/src/test/groovy/CommonsHttpClientBlockingForkedTest.groovy create mode 100644 dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/test/groovy/datadog/trace/instrumentation/httpclient/JavaHttpClientBlockingForkedTest.groovy diff --git a/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpclient/HelperMethods.java b/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpclient/HelperMethods.java index b4f77d405c6..7a9d74de25d 100644 --- a/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpclient/HelperMethods.java +++ b/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpclient/HelperMethods.java @@ -9,6 +9,7 @@ import static datadog.trace.instrumentation.apachehttpclient.ApacheHttpClientDecorator.HTTP_REQUEST; import static datadog.trace.instrumentation.apachehttpclient.HttpHeadersInjectAdapter.SETTER; +import datadog.appsec.api.blocking.BlockingException; import datadog.context.ContextScope; import datadog.trace.bootstrap.CallDepthThreadLocalMap; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; @@ -40,8 +41,19 @@ private static ContextScope activateHttpSpan(final HttpUriRequest request) { final AgentSpan span = startSpan(APACHE_HTTP_CLIENT.toString(), HTTP_REQUEST); final ContextScope scope = activateSpan(span); - DECORATE.afterStart(span); - DECORATE.onRequest(span, request); + try { + DECORATE.afterStart(span); + DECORATE.onRequest(span, request); + } catch (BlockingException e) { + try { + DECORATE.onError(span, e); + DECORATE.beforeFinish(span); + } finally { + scope.close(); + span.finish(); + } + throw e; + } return scope; } diff --git a/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/test/groovy/ApacheHttpClientBlockingForkedTest.groovy b/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/test/groovy/ApacheHttpClientBlockingForkedTest.groovy new file mode 100644 index 00000000000..78c316bfb36 --- /dev/null +++ b/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/test/groovy/ApacheHttpClientBlockingForkedTest.groovy @@ -0,0 +1,80 @@ +import datadog.appsec.api.blocking.BlockingContentType +import datadog.appsec.api.blocking.BlockingException +import datadog.trace.agent.test.InstrumentationSpecification +import datadog.trace.api.gateway.Flow +import datadog.trace.api.gateway.RequestContextSlot +import datadog.trace.bootstrap.CallDepthThreadLocalMap +import datadog.trace.bootstrap.instrumentation.api.AgentTracer +import datadog.trace.bootstrap.instrumentation.api.TagContext +import org.apache.http.HttpHost +import org.apache.http.client.HttpClient +import org.apache.http.client.methods.HttpGet +import org.apache.http.impl.client.DefaultHttpClient +import org.apache.http.message.BasicHttpRequest + +import java.util.function.BiFunction + +import static datadog.trace.api.gateway.Events.EVENTS + +/** Forked so AppSec configuration is applied before instrumentation is installed. */ +class ApacheHttpClientBlockingForkedTest extends InstrumentationSpecification { + @Override + protected void configurePreAgent() { + super.configurePreAgent() + injectSysConfig('appsec.enabled', 'true') + injectSysConfig('appsec.rasp.enabled', 'true') + } + + def 'blocking restores the parent and finishes each client span'() { + given: + def subscription = AgentTracer.get().getSubscriptionService(RequestContextSlot.APPSEC) + def blockedSpans = [] + def flow = Stub(Flow) { + getAction() >> new Flow.Action.RequestBlockingAction(403, BlockingContentType.JSON) + } + subscription.registerCallback(EVENTS.httpClientRequest(), { ctx, request -> + blockedSpans.add(AgentTracer.activeSpan()) + flow + } as BiFunction) + def parent = TEST_TRACER.startSpan('test', 'parent', + new TagContext().withRequestContextDataAppSec(new Object())) + def parentScope = AgentTracer.activateSpan(parent) + def client = new DefaultHttpClient() + + when: + 2.times { + try { + if (hostRequest) { + client.execute(new HttpHost('localhost', 1), new BasicHttpRequest('GET', '/blocked')) + } else { + client.execute(new HttpGet('http://localhost:1/blocked')) + } + assert false: 'The request must be blocked before reaching the client' + } catch (BlockingException expected) { + assert AgentTracer.activeSpan().is(parent) + assert CallDepthThreadLocalMap.getCallDepth(HttpClient) == 0 + assert blockedSpans.last().finished + } + } + parentScope.close() + parentScope = null + parent.finish() + + then: + blockedSpans.size() == 2 + blockedSpans.every { it.parentId == parent.spanId } + TEST_WRITER.waitForTraces(1) + TEST_WRITER.size() == 1 + TEST_WRITER[0].size() == 3 + + cleanup: + parentScope?.close() + if (parent != null && !parent.finished) { + parent.finish() + } + subscription.reset() + + where: + hostRequest << [false, true] + } +} diff --git a/dd-java-agent/instrumentation/commons-httpclient-2.0/src/main/java/datadog/trace/instrumentation/commonshttpclient/CommonsHttpClientInstrumentation.java b/dd-java-agent/instrumentation/commons-httpclient-2.0/src/main/java/datadog/trace/instrumentation/commonshttpclient/CommonsHttpClientInstrumentation.java index f7a64539c31..322ffc65b95 100644 --- a/dd-java-agent/instrumentation/commons-httpclient-2.0/src/main/java/datadog/trace/instrumentation/commonshttpclient/CommonsHttpClientInstrumentation.java +++ b/dd-java-agent/instrumentation/commons-httpclient-2.0/src/main/java/datadog/trace/instrumentation/commonshttpclient/CommonsHttpClientInstrumentation.java @@ -60,6 +60,7 @@ public static class ExecAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static ContextScope methodEnter(@Advice.Argument(1) final HttpMethod httpMethod) { + ContextScope scope = null; try { final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(HttpClient.class); if (callDepth > 0) { @@ -67,7 +68,7 @@ public static ContextScope methodEnter(@Advice.Argument(1) final HttpMethod http } final AgentSpan span = startSpan("commons-http-client", HTTP_REQUEST); - final ContextScope scope = activateSpan(span); + scope = activateSpan(span); DECORATE.afterStart(span); DECORATE.onRequest(span, httpMethod); @@ -75,6 +76,16 @@ public static ContextScope methodEnter(@Advice.Argument(1) final HttpMethod http return scope; } catch (BlockingException e) { CallDepthThreadLocalMap.reset(HttpClient.class); + if (scope != null) { + final AgentSpan span = spanFromScope(scope); + try { + DECORATE.onError(span, e); + DECORATE.beforeFinish(span); + } finally { + scope.close(); + span.finish(); + } + } // re-throw blocking exceptions throw e; } diff --git a/dd-java-agent/instrumentation/commons-httpclient-2.0/src/test/groovy/CommonsHttpClientBlockingForkedTest.groovy b/dd-java-agent/instrumentation/commons-httpclient-2.0/src/test/groovy/CommonsHttpClientBlockingForkedTest.groovy new file mode 100644 index 00000000000..f33b6a2932b --- /dev/null +++ b/dd-java-agent/instrumentation/commons-httpclient-2.0/src/test/groovy/CommonsHttpClientBlockingForkedTest.groovy @@ -0,0 +1,70 @@ +import datadog.appsec.api.blocking.BlockingContentType +import datadog.appsec.api.blocking.BlockingException +import datadog.trace.agent.test.InstrumentationSpecification +import datadog.trace.api.gateway.Flow +import datadog.trace.api.gateway.RequestContextSlot +import datadog.trace.bootstrap.CallDepthThreadLocalMap +import datadog.trace.bootstrap.instrumentation.api.AgentTracer +import datadog.trace.bootstrap.instrumentation.api.TagContext +import org.apache.commons.httpclient.HttpClient +import org.apache.commons.httpclient.methods.GetMethod + +import java.util.function.BiFunction + +import static datadog.trace.api.gateway.Events.EVENTS + +/** Forked so AppSec configuration is applied before instrumentation is installed. */ +class CommonsHttpClientBlockingForkedTest extends InstrumentationSpecification { + @Override + protected void configurePreAgent() { + super.configurePreAgent() + injectSysConfig('appsec.enabled', 'true') + injectSysConfig('appsec.rasp.enabled', 'true') + } + + def 'blocking restores the parent and finishes each client span'() { + given: + def subscription = AgentTracer.get().getSubscriptionService(RequestContextSlot.APPSEC) + def blockedSpans = [] + def flow = Stub(Flow) { + getAction() >> new Flow.Action.RequestBlockingAction(403, BlockingContentType.JSON) + } + subscription.registerCallback(EVENTS.httpClientRequest(), { ctx, request -> + blockedSpans.add(AgentTracer.activeSpan()) + flow + } as BiFunction) + def parent = TEST_TRACER.startSpan('test', 'parent', + new TagContext().withRequestContextDataAppSec(new Object())) + def parentScope = AgentTracer.activateSpan(parent) + def client = new HttpClient() + + when: + 2.times { + try { + client.executeMethod(new GetMethod('http://localhost:1/blocked')) + assert false: 'The request must be blocked before reaching the client' + } catch (BlockingException expected) { + assert AgentTracer.activeSpan().is(parent) + assert CallDepthThreadLocalMap.getCallDepth(HttpClient) == 0 + assert blockedSpans.last().finished + } + } + parentScope.close() + parentScope = null + parent.finish() + + then: + blockedSpans.size() == 2 + blockedSpans.every { it.parentId == parent.spanId } + TEST_WRITER.waitForTraces(1) + TEST_WRITER.size() == 1 + TEST_WRITER[0].size() == 3 + + cleanup: + parentScope?.close() + if (parent != null && !parent.finished) { + parent.finish() + } + subscription.reset() + } +} diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/main/java11/datadog/trace/instrumentation/httpclient/SendAdvice.java b/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/main/java11/datadog/trace/instrumentation/httpclient/SendAdvice.java index 389d0a9ff6f..c57ecda7e43 100644 --- a/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/main/java11/datadog/trace/instrumentation/httpclient/SendAdvice.java +++ b/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/main/java11/datadog/trace/instrumentation/httpclient/SendAdvice.java @@ -20,6 +20,7 @@ public class SendAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static ContextScope methodEnter( @Advice.Argument(value = 0) final HttpRequest httpRequest) { + ContextScope scope = null; try { if (DECORATE.isAgentRequest(httpRequest)) { return null; @@ -34,7 +35,7 @@ public static ContextScope methodEnter( } DECORATE.allowContextInjection(); final AgentSpan span = startSpan(INSTRUMENTATION_NAME, OPERATION_NAME); - final ContextScope scope = activateSpan(span); + scope = activateSpan(span); DECORATE.afterStart(span); DECORATE.onRequest(span, httpRequest); @@ -44,6 +45,16 @@ public static ContextScope methodEnter( } catch (BlockingException e) { CallDepthThreadLocalMap.reset(HttpClient.class); DECORATE.blockContextInjection(); + if (scope != null) { + final AgentSpan span = spanFromScope(scope); + try { + DECORATE.onError(span, e); + DECORATE.beforeFinish(span); + } finally { + scope.close(); + span.finish(); + } + } // re-throw blocking exceptions throw e; } diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/main/java11/datadog/trace/instrumentation/httpclient/SendAsyncAdvice.java b/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/main/java11/datadog/trace/instrumentation/httpclient/SendAsyncAdvice.java index f9838510417..333596606e9 100644 --- a/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/main/java11/datadog/trace/instrumentation/httpclient/SendAsyncAdvice.java +++ b/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/main/java11/datadog/trace/instrumentation/httpclient/SendAsyncAdvice.java @@ -22,6 +22,7 @@ public class SendAsyncAdvice { public static ContextScope methodEnter( @Advice.Argument(value = 0) final HttpRequest httpRequest, @Advice.Argument(value = 1, readOnly = false) HttpResponse.BodyHandler bodyHandler) { + ContextScope scope = null; try { if (DECORATE.isAgentRequest(httpRequest)) { return null; @@ -36,7 +37,7 @@ public static ContextScope methodEnter( } DECORATE.allowContextInjection(); final AgentSpan span = startSpan(INSTRUMENTATION_NAME, OPERATION_NAME); - final ContextScope scope = activateSpan(span); + scope = activateSpan(span); if (bodyHandler != null) { // Pass span directly — BodyHandlerWrapper captures the continuation lazily in apply(), // only once response headers arrive. This avoids leaking a continuation when the @@ -52,6 +53,16 @@ public static ContextScope methodEnter( } catch (BlockingException e) { CallDepthThreadLocalMap.reset(HttpClient.class); DECORATE.blockContextInjection(); + if (scope != null) { + final AgentSpan span = spanFromScope(scope); + try { + DECORATE.onError(span, e); + DECORATE.beforeFinish(span); + } finally { + scope.close(); + span.finish(); + } + } // re-throw blocking exceptions throw e; } diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/test/groovy/datadog/trace/instrumentation/httpclient/JavaHttpClientBlockingForkedTest.groovy b/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/test/groovy/datadog/trace/instrumentation/httpclient/JavaHttpClientBlockingForkedTest.groovy new file mode 100644 index 00000000000..b372062a774 --- /dev/null +++ b/dd-java-agent/instrumentation/java/java-net/java-net-11.0/src/test/groovy/datadog/trace/instrumentation/httpclient/JavaHttpClientBlockingForkedTest.groovy @@ -0,0 +1,82 @@ +package datadog.trace.instrumentation.httpclient + +import datadog.appsec.api.blocking.BlockingContentType +import datadog.appsec.api.blocking.BlockingException +import datadog.trace.agent.test.InstrumentationSpecification +import datadog.trace.api.gateway.Flow +import datadog.trace.api.gateway.RequestContextSlot +import datadog.trace.bootstrap.CallDepthThreadLocalMap +import datadog.trace.bootstrap.instrumentation.api.AgentTracer +import datadog.trace.bootstrap.instrumentation.api.TagContext +import java.net.http.HttpClient +import java.net.http.HttpRequest +import java.net.http.HttpResponse + +import java.util.function.BiFunction + +import static datadog.trace.api.gateway.Events.EVENTS + +/** Forked so AppSec configuration is applied before instrumentation is installed. */ +class JavaHttpClientBlockingForkedTest extends InstrumentationSpecification { + @Override + protected void configurePreAgent() { + super.configurePreAgent() + injectSysConfig('appsec.enabled', 'true') + injectSysConfig('appsec.rasp.enabled', 'true') + } + + def 'blocking restores the parent and finishes each client span'() { + given: + def subscription = AgentTracer.get().getSubscriptionService(RequestContextSlot.APPSEC) + def blockedSpans = [] + def flow = Stub(Flow) { + getAction() >> new Flow.Action.RequestBlockingAction(403, BlockingContentType.JSON) + } + subscription.registerCallback(EVENTS.httpClientRequest(), { ctx, clientRequest -> + blockedSpans.add(AgentTracer.activeSpan()) + flow + } as BiFunction) + def parent = TEST_TRACER.startSpan('test', 'parent', + new TagContext().withRequestContextDataAppSec(new Object())) + def parentScope = AgentTracer.activateSpan(parent) + def client = HttpClient.newHttpClient() + def request = HttpRequest.newBuilder(URI.create('http://localhost:1/blocked')).build() + + when: + 2.times { + try { + if (async) { + client.sendAsync(request, HttpResponse.BodyHandlers.discarding()) + } else { + client.send(request, HttpResponse.BodyHandlers.discarding()) + } + assert false: 'The request must be blocked before reaching the client' + } catch (BlockingException expected) { + assert AgentTracer.activeSpan().is(parent) + assert CallDepthThreadLocalMap.getCallDepth(HttpClient) == 0 + assert !JavaNetClientDecorator.DECORATE.isContextInjectionAllowed() + assert blockedSpans.last().finished + } + } + parentScope.close() + parentScope = null + parent.finish() + + then: + blockedSpans.size() == 2 + blockedSpans.every { it.parentId == parent.spanId } + TEST_WRITER.waitForTraces(1) + TEST_WRITER.size() == 1 + TEST_WRITER[0].size() == 3 + + cleanup: + parentScope?.close() + if (parent != null && !parent.finished) { + parent.finish() + } + subscription.reset() + + where: + async << [false, true] + } +} From f514fb21d436524cf4f2d3ff1e75d51a9c63f5c6 Mon Sep 17 00:00:00 2001 From: Andrea Marziali Date: Thu, 24 Sep 2026 16:25:54 +0200 Subject: [PATCH 2/2] remove try-finally --- .../apachehttpclient/HelperMethods.java | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpclient/HelperMethods.java b/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpclient/HelperMethods.java index 7a9d74de25d..e6afc6202b5 100644 --- a/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpclient/HelperMethods.java +++ b/dd-java-agent/instrumentation/apache-httpclient/apache-httpclient-4.0/src/main/java/datadog/trace/instrumentation/apachehttpclient/HelperMethods.java @@ -45,13 +45,10 @@ private static ContextScope activateHttpSpan(final HttpUriRequest request) { DECORATE.afterStart(span); DECORATE.onRequest(span, request); } catch (BlockingException e) { - try { - DECORATE.onError(span, e); - DECORATE.beforeFinish(span); - } finally { - scope.close(); - span.finish(); - } + DECORATE.onError(span, e); + DECORATE.beforeFinish(span); + scope.close(); + span.finish(); throw e; }