From 7ec97a0bcbf13aa698d6330dd86860dbe759276b Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Mon, 21 Sep 2026 16:35:38 +0530 Subject: [PATCH 1/8] Fix HttpClient 5.x callback span handling --- CHANGES.md | 1 + .../apm/plugin/httpclient/v5/Constants.java | 2 + .../HttpAsyncClientDoExecuteInterceptor.java | 2 +- .../v5/IOSessionImplPollInterceptor.java | 1 + .../apm/plugin/httpclient/v5/OwnedSpans.java | 47 +++++ .../wrapper/AsyncResponseConsumerWrapper.java | 7 + .../v5/wrapper/FutureCallbackWrapper.java | 30 ++- .../v5/FutureCallbackWrapperTest.java | 185 ++++++++++++++++++ 8 files changed, 264 insertions(+), 11 deletions(-) create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/OwnedSpans.java create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java diff --git a/CHANGES.md b/CHANGES.md index a175738df6..034cd6a168 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,6 +4,7 @@ Release Notes. 9.8.0 ------------------ +* Fix `httpclient-5.x-plugin` closing the caller thread's active span when `FutureCallback` executes on the caller thread (apache/skywalking#14097). * Fix the `spring-ai-1.x-plugin` `ChatModelStreamInterceptor` leaking its async span when `ChatModel#stream(Prompt)` fails synchronously, which silently dropped the whole `TraceSegment` diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/Constants.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/Constants.java index 2497ca8cbc..84bff48dc2 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/Constants.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/Constants.java @@ -20,4 +20,6 @@ public class Constants { public static String SKYWALKING_CONTEXT_SNAPSHOT = "skywalking-context-snapshot"; + + public static String SKYWALKING_LOCAL_SPAN = "skywalking-local-span"; } diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java index 68267fcc6f..06b60dc5da 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java @@ -39,7 +39,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr HttpContext context = (HttpContext) allArguments[4]; FutureCallback callback = (FutureCallback) allArguments[5]; allArguments[2] = new AsyncResponseConsumerWrapper(consumer); - allArguments[5] = new FutureCallbackWrapper(callback); + allArguments[5] = new FutureCallbackWrapper(callback, context); if (ContextManager.isActive()) { context.setAttribute(Constants.SKYWALKING_CONTEXT_SNAPSHOT, ContextManager.capture()); } diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/IOSessionImplPollInterceptor.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/IOSessionImplPollInterceptor.java index fc8ef190d4..f28848114e 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/IOSessionImplPollInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/IOSessionImplPollInterceptor.java @@ -62,6 +62,7 @@ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allA AbstractSpan localSpan = ContextManager.createLocalSpan("httpasyncclient/local"); localSpan.setComponent(ComponentsDefine.HTTP_ASYNC_CLIENT); localSpan.setLayer(SpanLayer.HTTP); + httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, localSpan); ContextManager.continued(snapshot); final ContextCarrier contextCarrier = new ContextCarrier(); diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/OwnedSpans.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/OwnedSpans.java new file mode 100644 index 0000000000..17cee16443 --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/OwnedSpans.java @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5; + +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; + +/** + * The async HttpClient callbacks may run on a thread other than the I/O thread that created the spans, for example + * the business thread when {@code HttpAsyncClients.classic(...)} is used. Only the span created for the request may + * be finished by the plugin, and only when it is the active span of the current thread. + */ +public final class OwnedSpans { + + private OwnedSpans() { + } + + /** + * @return the local span created for this request if, and only if, it is the active span of the current thread. + */ + public static AbstractSpan activeOwnedSpan(HttpContext context) { + if (context == null || !ContextManager.isActive()) { + return null; + } + Object owned = context.getAttribute(Constants.SKYWALKING_LOCAL_SPAN); + if (owned != null && owned == ContextManager.activeSpan()) { + return (AbstractSpan) owned; + } + return null; + } +} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java index 9dec7d109a..2a41cfc849 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java @@ -28,6 +28,7 @@ import org.apache.skywalking.apm.agent.core.context.ContextManager; import org.apache.skywalking.apm.agent.core.context.tag.Tags; import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.plugin.httpclient.v5.OwnedSpans; import java.io.IOException; import java.nio.ByteBuffer; @@ -52,6 +53,12 @@ public void consumeResponse(HttpResponse response, EntityDetails entityDetails, span.errorOccurred(); } ContextManager.stopSpan(); + // consumeResponse runs on the I/O thread that created the local span. Finish it here, otherwise it + // would stay on this thread's span stack when the FutureCallback runs on another thread. + AbstractSpan localSpan = OwnedSpans.activeOwnedSpan(context); + if (localSpan != null) { + ContextManager.stopSpan(localSpan); + } } consumer.consumeResponse(response, entityDetails, context, resultCallback); } diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java index f606856edf..5052131a1e 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java @@ -18,20 +18,28 @@ package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; import org.apache.hc.core5.concurrent.FutureCallback; +import org.apache.hc.core5.http.protocol.HttpContext; import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.plugin.httpclient.v5.OwnedSpans; public class FutureCallbackWrapper implements FutureCallback { - private FutureCallback callback; + private final FutureCallback callback; + private final HttpContext context; - public FutureCallbackWrapper(FutureCallback callback) { + public FutureCallbackWrapper(FutureCallback callback, HttpContext context) { this.callback = callback; + this.context = context; } @Override public void completed(T o) { - if (ContextManager.isActive()) { - ContextManager.stopSpan(); + // The callback may run on the caller thread (e.g. HttpAsyncClients.classic), whose active span + // does not belong to this request. Only finish the span created for this request. + AbstractSpan span = OwnedSpans.activeOwnedSpan(context); + if (span != null) { + ContextManager.stopSpan(span); } if (callback != null) { callback.completed(o); @@ -40,9 +48,10 @@ public void completed(T o) { @Override public void failed(Exception e) { - if (ContextManager.isActive()) { - ContextManager.activeSpan().errorOccurred().log(e); - ContextManager.stopSpan(); + AbstractSpan span = OwnedSpans.activeOwnedSpan(context); + if (span != null) { + span.errorOccurred().log(e); + ContextManager.stopSpan(span); } if (callback != null) { callback.failed(e); @@ -51,9 +60,10 @@ public void failed(Exception e) { @Override public void cancelled() { - if (ContextManager.isActive()) { - ContextManager.activeSpan().errorOccurred(); - ContextManager.stopSpan(); + AbstractSpan span = OwnedSpans.activeOwnedSpan(context); + if (span != null) { + span.errorOccurred(); + ContextManager.stopSpan(span); } if (callback != null) { callback.cancelled(); diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java new file mode 100644 index 0000000000..7bb6dc6046 --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java @@ -0,0 +1,185 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5; + +import org.apache.hc.core5.concurrent.FutureCallback; +import org.apache.hc.core5.http.HttpResponse; +import org.apache.hc.core5.http.nio.AsyncResponseConsumer; +import org.apache.hc.core5.http.protocol.BasicHttpContext; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.skywalking.apm.agent.core.boot.ServiceManager; +import org.apache.skywalking.apm.agent.core.context.ContextCarrier; +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractTracingSpan; +import org.apache.skywalking.apm.agent.core.context.trace.TraceSegment; +import org.apache.skywalking.apm.agent.test.helper.SegmentHelper; +import org.apache.skywalking.apm.agent.test.helper.SpanHelper; +import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; +import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; +import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; +import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; +import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncResponseConsumerWrapper; +import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.FutureCallbackWrapper; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +import java.util.List; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@RunWith(TracingSegmentRunner.class) +public class FutureCallbackWrapperTest { + + @SegmentStoragePoint + private SegmentStorage segmentStorage; + + @Rule + public AgentServiceRule agentServiceRule = new AgentServiceRule(); + @Rule + public MockitoRule rule = MockitoJUnit.rule(); + + @Mock + private FutureCallback delegate; + + @Mock + private AsyncResponseConsumer consumer; + @Mock + private HttpResponse response; + + private HttpContext httpContext; + + @Before + public void setUp() { + ServiceManager.INSTANCE.boot(); + httpContext = new BasicHttpContext(); + } + + @Test + public void completedOnIoThreadStopsOwnedSpan() { + AbstractSpan owned = ContextManager.createLocalSpan("httpasyncclient/local"); + httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, owned); + + new FutureCallbackWrapper<>(delegate, httpContext).completed("ok"); + + assertThat(ContextManager.isActive(), is(false)); + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + verify(delegate).completed("ok"); + } + + @Test + public void failedOnIoThreadMarksOwnedSpanAsError() { + AbstractSpan owned = ContextManager.createLocalSpan("httpasyncclient/local"); + httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, owned); + Exception cause = new RuntimeException("boom"); + + new FutureCallbackWrapper<>(delegate, httpContext).failed(cause); + + assertThat(ContextManager.isActive(), is(false)); + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + List spans = SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); + assertThat(SpanHelper.getErrorOccurred(spans.get(0)), is(true)); + verify(delegate).failed(cause); + } + + @Test + public void completedOnCallerThreadKeepsCallerSpanActive() throws Exception { + // The request span is created on an "I/O thread", as IOSessionImplPollInterceptor does. + Thread ioThread = new Thread(() -> { + AbstractSpan owned = ContextManager.createLocalSpan("httpasyncclient/local"); + httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, owned); + }); + ioThread.start(); + ioThread.join(); + + // The callback then runs on the caller/business thread, which has its own active Entry span. + AbstractSpan entry = ContextManager.createEntrySpan("/business", null); + + new FutureCallbackWrapper<>(delegate, httpContext).completed("ok"); + + assertThat(ContextManager.isActive(), is(true)); + assertThat(ContextManager.activeSpan() == entry, is(true)); + verify(delegate).completed("ok"); + + ContextManager.stopSpan(entry); + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + TraceSegment segment = segmentStorage.getTraceSegments().get(0); + assertThat(SegmentHelper.getSpans(segment).size(), is(1)); + } + + /** + * Full lifecycle of the reported scenario: spans are created and finished on the I/O thread, the FutureCallback + * then runs on the business thread which owns an Entry span that must survive. + */ + @Test + public void ioThreadSegmentIsFinishedAndCallerEntrySpanSurvives() throws Exception { + when(response.getCode()).thenReturn(200); + Throwable[] ioError = new Throwable[1]; + Thread ioThread = new Thread(() -> { + try { + // what IOSessionImplPollInterceptor does + AbstractSpan local = ContextManager.createLocalSpan("httpasyncclient/local"); + httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, local); + ContextManager.createExitSpan("/hello", new ContextCarrier(), "127.0.0.1:8080"); + // response headers arrive on the I/O thread + new AsyncResponseConsumerWrapper<>(consumer).consumeResponse(response, null, httpContext, null); + } catch (Throwable t) { + ioError[0] = t; + } + }); + ioThread.start(); + ioThread.join(); + assertThat(ioError[0] == null, is(true)); + + // local + exit span are both finished, so the I/O thread's segment is complete + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + assertThat(SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)).size(), is(2)); + + // entity read to EOF on the business thread triggers the callback there + AbstractSpan entry = ContextManager.createEntrySpan("/business", null); + new FutureCallbackWrapper<>(delegate, httpContext).completed("body"); + + assertThat(ContextManager.isActive(), is(true)); + assertThat(ContextManager.activeSpan() == entry, is(true)); + ContextManager.stopSpan(entry); + + assertThat(segmentStorage.getTraceSegments().size(), is(2)); + assertThat(SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(1)).size(), is(1)); + verify(delegate).completed("body"); + } + + @Test + public void nullContextNeverStopsForeignSpan() { + AbstractSpan entry = ContextManager.createEntrySpan("/business", null); + + new FutureCallbackWrapper<>(delegate, null).completed("ok"); + + assertThat(ContextManager.isActive(), is(true)); + ContextManager.stopSpan(entry); + verify(delegate).completed("ok"); + } +} From 13a5e173f84e92d5e2a11a01aaec0329665614a4 Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Tue, 22 Sep 2026 21:34:13 +0530 Subject: [PATCH 2/8] Fix HttpClient 5.x callback span handling --- .../plugin/httpclient/v5/AsyncExitSpan.java | 91 ++++++++++++ .../apm/plugin/httpclient/v5/Constants.java | 2 - .../HttpAsyncClientDoExecuteInterceptor.java | 31 ++-- .../v5/IOSessionImplPollInterceptor.java | 93 ------------ .../apm/plugin/httpclient/v5/OwnedSpans.java | 47 ------ .../wrapper/AsyncRequestProducerWrapper.java | 125 ++++++++++++++++ .../wrapper/AsyncResponseConsumerWrapper.java | 80 +++++----- .../v5/wrapper/FutureCallbackWrapper.java | 35 ++--- .../src/main/resources/skywalking-plugin.def | 1 - .../v5/FutureCallbackWrapperTest.java | 139 ++++++------------ 10 files changed, 322 insertions(+), 322 deletions(-) create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncExitSpan.java delete mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/IOSessionImplPollInterceptor.java delete mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/OwnedSpans.java create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncExitSpan.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncExitSpan.java new file mode 100644 index 0000000000..2565b2579c --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncExitSpan.java @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5; + +import org.apache.hc.core5.http.HttpHost; +import org.apache.skywalking.apm.agent.core.context.tag.Tags; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; + +public class AsyncExitSpan { + private final HttpHost target; + private volatile Thread creator = Thread.currentThread(); + private AbstractSpan span; + + public AsyncExitSpan(HttpHost target) { + this.target = target; + } + + public HttpHost getTarget() { + return target; + } + + public boolean claimCreation() { + if (creator != Thread.currentThread()) { + return false; + } + creator = null; + return true; + } + + public void callerReturned() { + creator = null; + } + + public synchronized void start(AbstractSpan span) { + this.span = span; + } + + public synchronized void onResponse(int statusCode) { + if (span != null) { + Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode); + if (statusCode >= 400) { + span.errorOccurred(); + } + } + } + + public synchronized void finish() { + end(false, null); + } + + public synchronized void fail(Throwable cause) { + end(true, cause); + } + + public synchronized void abort() { + end(true, null); + } + + private void end(boolean error, Throwable cause) { + if (span == null) { + return; + } + + if (error) { + span.errorOccurred(); + } + + if (cause != null) { + span.log(cause); + } + + span.asyncFinish(); + span = null; + } +} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/Constants.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/Constants.java index 84bff48dc2..2497ca8cbc 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/Constants.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/Constants.java @@ -20,6 +20,4 @@ public class Constants { public static String SKYWALKING_CONTEXT_SNAPSHOT = "skywalking-context-snapshot"; - - public static String SKYWALKING_LOCAL_SPAN = "skywalking-local-span"; } diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java index 06b60dc5da..71e0b1ca2d 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java @@ -18,42 +18,53 @@ package org.apache.skywalking.apm.plugin.httpclient.v5; +import java.lang.reflect.Method; import org.apache.hc.core5.concurrent.FutureCallback; +import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.nio.AsyncRequestProducer; import org.apache.hc.core5.http.nio.AsyncResponseConsumer; -import org.apache.hc.core5.http.protocol.HttpContext; import org.apache.skywalking.apm.agent.core.context.ContextManager; import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance; import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor; import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; +import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncRequestProducerWrapper; import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncResponseConsumerWrapper; import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.FutureCallbackWrapper; -import java.lang.reflect.Method; - public class HttpAsyncClientDoExecuteInterceptor implements InstanceMethodsAroundInterceptor { @Override public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, MethodInterceptResult result) throws Throwable { + if (!ContextManager.isActive()) { + return; + } + + AsyncExitSpan exitSpan = new AsyncExitSpan((HttpHost) allArguments[0]); + + AsyncRequestProducer producer = (AsyncRequestProducer) allArguments[1]; AsyncResponseConsumer consumer = (AsyncResponseConsumer) allArguments[2]; - HttpContext context = (HttpContext) allArguments[4]; FutureCallback callback = (FutureCallback) allArguments[5]; - allArguments[2] = new AsyncResponseConsumerWrapper(consumer); - allArguments[5] = new FutureCallbackWrapper(callback, context); - if (ContextManager.isActive()) { - context.setAttribute(Constants.SKYWALKING_CONTEXT_SNAPSHOT, ContextManager.capture()); - } + + allArguments[1] = new AsyncRequestProducerWrapper(producer, exitSpan); + allArguments[2] = new AsyncResponseConsumerWrapper(consumer, exitSpan); + allArguments[5] = new FutureCallbackWrapper(callback, exitSpan); } @Override public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, Object ret) throws Throwable { + if (allArguments[1] instanceof AsyncRequestProducerWrapper) { + ((AsyncRequestProducerWrapper) allArguments[1]).getExitSpan().callerReturned(); + } return ret; } @Override public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, Throwable t) { - + if (allArguments[1] instanceof AsyncRequestProducerWrapper) { + ((AsyncRequestProducerWrapper) allArguments[1]).getExitSpan().fail(t); + } } } diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/IOSessionImplPollInterceptor.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/IOSessionImplPollInterceptor.java deleted file mode 100644 index f28848114e..0000000000 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/IOSessionImplPollInterceptor.java +++ /dev/null @@ -1,93 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package org.apache.skywalking.apm.plugin.httpclient.v5; - -import org.apache.hc.client5.http.protocol.HttpClientContext; -import org.apache.hc.core5.http.message.BasicHttpRequest; -import org.apache.hc.core5.http.nio.command.RequestExecutionCommand; -import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.hc.core5.reactor.Command; -import org.apache.skywalking.apm.agent.core.context.CarrierItem; -import org.apache.skywalking.apm.agent.core.context.ContextCarrier; -import org.apache.skywalking.apm.agent.core.context.ContextManager; -import org.apache.skywalking.apm.agent.core.context.ContextSnapshot; -import org.apache.skywalking.apm.agent.core.context.tag.Tags; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; -import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer; -import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance; -import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor; -import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; -import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; - -import java.lang.reflect.Method; -import java.net.URI; - -public class IOSessionImplPollInterceptor implements InstanceMethodsAroundInterceptor { - - @Override - public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, - MethodInterceptResult result) throws Throwable { - - } - - @Override - public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, - Object ret) throws Throwable { - Command command = (Command) ret; - if (!(command instanceof RequestExecutionCommand)) { - return ret; - } - HttpContext httpContext = ((RequestExecutionCommand) command).getContext(); - ContextSnapshot snapshot = (ContextSnapshot) httpContext.getAttribute(Constants.SKYWALKING_CONTEXT_SNAPSHOT); - if (snapshot == null) { - return ret; - } - httpContext.removeAttribute(Constants.SKYWALKING_CONTEXT_SNAPSHOT); - AbstractSpan localSpan = ContextManager.createLocalSpan("httpasyncclient/local"); - localSpan.setComponent(ComponentsDefine.HTTP_ASYNC_CLIENT); - localSpan.setLayer(SpanLayer.HTTP); - httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, localSpan); - ContextManager.continued(snapshot); - - final ContextCarrier contextCarrier = new ContextCarrier(); - BasicHttpRequest request = (BasicHttpRequest) httpContext.getAttribute(HttpClientContext.HTTP_REQUEST); - URI uri = request.getUri(); - - String operationName = uri.getPath(); - int port = uri.getPort(); - AbstractSpan span = ContextManager - .createExitSpan(operationName, contextCarrier, uri.getHost() + ":" + (port == -1 ? 80 : port)); - span.setComponent(ComponentsDefine.HTTP_ASYNC_CLIENT); - Tags.URL.set(span, uri.toURL().toString()); - Tags.HTTP.METHOD.set(span, request.getMethod()); - SpanLayer.asHttp(span); - CarrierItem next = contextCarrier.items(); - while (next.hasNext()) { - next = next.next(); - request.setHeader(next.getHeadKey(), next.getHeadValue()); - } - return ret; - } - - @Override - public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, - Class[] argumentsTypes, Throwable t) { - - } -} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/OwnedSpans.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/OwnedSpans.java deleted file mode 100644 index 17cee16443..0000000000 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/OwnedSpans.java +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.skywalking.apm.plugin.httpclient.v5; - -import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.skywalking.apm.agent.core.context.ContextManager; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; - -/** - * The async HttpClient callbacks may run on a thread other than the I/O thread that created the spans, for example - * the business thread when {@code HttpAsyncClients.classic(...)} is used. Only the span created for the request may - * be finished by the plugin, and only when it is the active span of the current thread. - */ -public final class OwnedSpans { - - private OwnedSpans() { - } - - /** - * @return the local span created for this request if, and only if, it is the active span of the current thread. - */ - public static AbstractSpan activeOwnedSpan(HttpContext context) { - if (context == null || !ContextManager.isActive()) { - return null; - } - Object owned = context.getAttribute(Constants.SKYWALKING_LOCAL_SPAN); - if (owned != null && owned == ContextManager.activeSpan()) { - return (AbstractSpan) owned; - } - return null; - } -} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java new file mode 100644 index 0000000000..5a81b592e0 --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; + +import java.io.IOException; +import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.nio.AsyncRequestProducer; +import org.apache.hc.core5.http.nio.DataStreamChannel; +import org.apache.hc.core5.http.nio.RequestChannel; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.skywalking.apm.agent.core.context.CarrierItem; +import org.apache.skywalking.apm.agent.core.context.ContextCarrier; +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.tag.Tags; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer; +import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; +import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncExitSpan; + +public class AsyncRequestProducerWrapper implements AsyncRequestProducer { + + private final AsyncRequestProducer producer; + private final AsyncExitSpan exitSpan; + + public AsyncRequestProducerWrapper(AsyncRequestProducer producer, AsyncExitSpan exitSpan) { + this.producer = producer; + this.exitSpan = exitSpan; + } + + public AsyncExitSpan getExitSpan() { + return exitSpan; + } + + @Override + public void sendRequest(RequestChannel channel, HttpContext context) throws IOException, HttpException { + producer.sendRequest((request, entityDetails, requestContext) -> { + if (exitSpan.claimCreation()) { + try { + startExitSpan(request); + } catch (Throwable ignored) { + // Never let tracing instrumentation break the user's HTTP request. + } + } + + channel.sendRequest(request, entityDetails, requestContext); + }, context); + } + + private void startExitSpan(HttpRequest request) { + String operationName = request.getRequestUri(); + String remotePeer = exitSpan.getTarget().toHostString(); + + ContextCarrier contextCarrier = new ContextCarrier(); + AbstractSpan span = ContextManager.createExitSpan( + operationName, + contextCarrier, + remotePeer + ); + + boolean nested = ContextManager.activeSpan().isExit(); + + if (!nested) { + span.setComponent(ComponentsDefine.HTTP_ASYNC_CLIENT); + Tags.URL.set(span, request.getRequestUri()); + SpanLayer.asHttp(span); + } + + CarrierItem next = contextCarrier.items(); + while (next.hasNext()) { + request.setHeader(next.getHeadKey(), next.getHeadValue()); + next = next.next(); + } + + if (!nested) { + span.prepareForAsync(); + } + + ContextManager.stopSpan(span); + + if (!nested) { + exitSpan.start(span); + } + } + + @Override + public boolean isRepeatable() { + return producer.isRepeatable(); + } + + @Override + public void produce(DataStreamChannel channel) throws IOException { + producer.produce(channel); + } + + @Override + public int available() { + return producer.available(); + } + + @Override + public void failed(Exception cause) { + producer.failed(cause); + } + + @Override + public void releaseResources() { + producer.releaseResources(); + } +} \ No newline at end of file diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java index 2a41cfc849..cb5b0c361d 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java @@ -17,75 +17,53 @@ package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; -import org.apache.hc.core5.concurrent.FutureCallback; +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.List; import org.apache.hc.core5.http.EntityDetails; -import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.nio.AsyncResponseConsumer; import org.apache.hc.core5.http.nio.CapacityChannel; import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.skywalking.apm.agent.core.context.ContextManager; -import org.apache.skywalking.apm.agent.core.context.tag.Tags; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; -import org.apache.skywalking.apm.plugin.httpclient.v5.OwnedSpans; - -import java.io.IOException; -import java.nio.ByteBuffer; -import java.util.List; +import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.concurrent.FutureCallback; +import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncExitSpan; public class AsyncResponseConsumerWrapper implements AsyncResponseConsumer { - private AsyncResponseConsumer consumer; + private final AsyncResponseConsumer consumer; + private final AsyncExitSpan exitSpan; - public AsyncResponseConsumerWrapper(AsyncResponseConsumer consumer) { + public AsyncResponseConsumerWrapper( + AsyncResponseConsumer consumer, AsyncExitSpan exitSpan) { this.consumer = consumer; + this.exitSpan = exitSpan; } @Override - public void consumeResponse(HttpResponse response, EntityDetails entityDetails, HttpContext context, + public void consumeResponse( + HttpResponse response, + EntityDetails entityDetails, + HttpContext context, FutureCallback resultCallback) throws HttpException, IOException { - if (ContextManager.isActive()) { - int statusCode = response.getCode(); - AbstractSpan span = ContextManager.activeSpan(); - Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode); - if (statusCode >= 400) { - span.errorOccurred(); - } - ContextManager.stopSpan(); - // consumeResponse runs on the I/O thread that created the local span. Finish it here, otherwise it - // would stay on this thread's span stack when the FutureCallback runs on another thread. - AbstractSpan localSpan = OwnedSpans.activeOwnedSpan(context); - if (localSpan != null) { - ContextManager.stopSpan(localSpan); - } + + exitSpan.onResponse(response.getCode()); + + if (entityDetails == null) { + exitSpan.finish(); } + consumer.consumeResponse(response, entityDetails, context, resultCallback); } @Override - public void informationResponse(HttpResponse response, HttpContext context) throws HttpException, IOException { - if (ContextManager.isActive()) { - int statusCode = response.getCode(); - AbstractSpan span = ContextManager.activeSpan(); - Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode); - if (statusCode >= 400) { - span.errorOccurred(); - } - ContextManager.stopSpan(); - } + public void informationResponse( + HttpResponse response, + HttpContext context) throws HttpException, IOException { consumer.informationResponse(response, context); } - @Override - public void failed(Exception cause) { - if (ContextManager.isActive()) { - ContextManager.activeSpan().errorOccurred().log(cause); - ContextManager.stopSpan(); - } - consumer.failed(cause); - } - @Override public void updateCapacity(CapacityChannel capacityChannel) throws IOException { consumer.updateCapacity(capacityChannel); @@ -98,11 +76,19 @@ public void consume(ByteBuffer src) throws IOException { @Override public void streamEnd(List trailers) throws HttpException, IOException { + exitSpan.finish(); consumer.streamEnd(trailers); } + @Override + public void failed(Exception cause) { + exitSpan.fail(cause); + consumer.failed(cause); + } + @Override public void releaseResources() { + exitSpan.abort(); consumer.releaseResources(); } -} +} \ No newline at end of file diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java index 5052131a1e..eb63792c88 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java @@ -18,29 +18,22 @@ package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; import org.apache.hc.core5.concurrent.FutureCallback; -import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.skywalking.apm.agent.core.context.ContextManager; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; -import org.apache.skywalking.apm.plugin.httpclient.v5.OwnedSpans; +import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncExitSpan; public class FutureCallbackWrapper implements FutureCallback { private final FutureCallback callback; - private final HttpContext context; + private final AsyncExitSpan exitSpan; - public FutureCallbackWrapper(FutureCallback callback, HttpContext context) { + public FutureCallbackWrapper(FutureCallback callback, AsyncExitSpan exitSpan) { this.callback = callback; - this.context = context; + this.exitSpan = exitSpan; } @Override public void completed(T o) { - // The callback may run on the caller thread (e.g. HttpAsyncClients.classic), whose active span - // does not belong to this request. Only finish the span created for this request. - AbstractSpan span = OwnedSpans.activeOwnedSpan(context); - if (span != null) { - ContextManager.stopSpan(span); - } + exitSpan.finish(); + if (callback != null) { callback.completed(o); } @@ -48,11 +41,8 @@ public void completed(T o) { @Override public void failed(Exception e) { - AbstractSpan span = OwnedSpans.activeOwnedSpan(context); - if (span != null) { - span.errorOccurred().log(e); - ContextManager.stopSpan(span); - } + exitSpan.fail(e); + if (callback != null) { callback.failed(e); } @@ -60,13 +50,10 @@ public void failed(Exception e) { @Override public void cancelled() { - AbstractSpan span = OwnedSpans.activeOwnedSpan(context); - if (span != null) { - span.errorOccurred(); - ContextManager.stopSpan(span); - } + exitSpan.abort(); + if (callback != null) { callback.cancelled(); } } -} +} \ No newline at end of file diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/resources/skywalking-plugin.def b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/resources/skywalking-plugin.def index dc6622a88f..63c6348953 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/resources/skywalking-plugin.def +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/resources/skywalking-plugin.def @@ -17,4 +17,3 @@ httpclient-5.x=org.apache.skywalking.apm.plugin.httpclient.v5.define.MinimalHttpClientInstrumentation httpclient-5.x=org.apache.skywalking.apm.plugin.httpclient.v5.define.InternalHttpClientInstrumentation httpclient-5.x=org.apache.skywalking.apm.plugin.httpclient.v5.define.HttpAsyncClientInstrumentation -httpclient-5.x=org.apache.skywalking.apm.plugin.httpclient.v5.define.IOSessionImplInstrumentation diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java index 7bb6dc6046..386a3cf1f7 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java @@ -19,23 +19,15 @@ package org.apache.skywalking.apm.plugin.httpclient.v5; import org.apache.hc.core5.concurrent.FutureCallback; -import org.apache.hc.core5.http.HttpResponse; -import org.apache.hc.core5.http.nio.AsyncResponseConsumer; -import org.apache.hc.core5.http.protocol.BasicHttpContext; -import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.HttpHost; import org.apache.skywalking.apm.agent.core.boot.ServiceManager; import org.apache.skywalking.apm.agent.core.context.ContextCarrier; import org.apache.skywalking.apm.agent.core.context.ContextManager; import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractTracingSpan; -import org.apache.skywalking.apm.agent.core.context.trace.TraceSegment; -import org.apache.skywalking.apm.agent.test.helper.SegmentHelper; -import org.apache.skywalking.apm.agent.test.helper.SpanHelper; import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; -import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncResponseConsumerWrapper; import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.FutureCallbackWrapper; import org.junit.Before; import org.junit.Rule; @@ -45,12 +37,9 @@ import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; -import java.util.List; - import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; @RunWith(TracingSegmentRunner.class) public class FutureCallbackWrapperTest { @@ -60,126 +49,80 @@ public class FutureCallbackWrapperTest { @Rule public AgentServiceRule agentServiceRule = new AgentServiceRule(); + @Rule public MockitoRule rule = MockitoJUnit.rule(); @Mock private FutureCallback delegate; - @Mock - private AsyncResponseConsumer consumer; - @Mock - private HttpResponse response; - - private HttpContext httpContext; - @Before public void setUp() { ServiceManager.INSTANCE.boot(); - httpContext = new BasicHttpContext(); } - @Test - public void completedOnIoThreadStopsOwnedSpan() { - AbstractSpan owned = ContextManager.createLocalSpan("httpasyncclient/local"); - httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, owned); - - new FutureCallbackWrapper<>(delegate, httpContext).completed("ok"); + private AsyncExitSpan createStartedExitSpan() { + AsyncExitSpan exitSpan = new AsyncExitSpan( + new HttpHost("http", "127.0.0.1", 8080)); - assertThat(ContextManager.isActive(), is(false)); - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - verify(delegate).completed("ok"); - } - - @Test - public void failedOnIoThreadMarksOwnedSpanAsError() { - AbstractSpan owned = ContextManager.createLocalSpan("httpasyncclient/local"); - httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, owned); - Exception cause = new RuntimeException("boom"); + AbstractSpan requestSpan = ContextManager.createExitSpan( + "/hello", + new ContextCarrier(), + "127.0.0.1:8080"); - new FutureCallbackWrapper<>(delegate, httpContext).failed(cause); + exitSpan.start(requestSpan); + requestSpan.prepareForAsync(); + ContextManager.stopSpan(requestSpan); - assertThat(ContextManager.isActive(), is(false)); - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - List spans = SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); - assertThat(SpanHelper.getErrorOccurred(spans.get(0)), is(true)); - verify(delegate).failed(cause); + return exitSpan; } @Test - public void completedOnCallerThreadKeepsCallerSpanActive() throws Exception { - // The request span is created on an "I/O thread", as IOSessionImplPollInterceptor does. - Thread ioThread = new Thread(() -> { - AbstractSpan owned = ContextManager.createLocalSpan("httpasyncclient/local"); - httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, owned); - }); - ioThread.start(); - ioThread.join(); + public void completedKeepsCallerSpanActive() { + AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); + AsyncExitSpan exitSpan = createStartedExitSpan(); - // The callback then runs on the caller/business thread, which has its own active Entry span. - AbstractSpan entry = ContextManager.createEntrySpan("/business", null); - - new FutureCallbackWrapper<>(delegate, httpContext).completed("ok"); + new FutureCallbackWrapper<>(delegate, exitSpan).completed("ok"); assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == entry, is(true)); + assertThat(ContextManager.activeSpan() == callerSpan, is(true)); verify(delegate).completed("ok"); - ContextManager.stopSpan(entry); + ContextManager.stopSpan(callerSpan); + assertThat(segmentStorage.getTraceSegments().size(), is(1)); - TraceSegment segment = segmentStorage.getTraceSegments().get(0); - assertThat(SegmentHelper.getSpans(segment).size(), is(1)); } - /** - * Full lifecycle of the reported scenario: spans are created and finished on the I/O thread, the FutureCallback - * then runs on the business thread which owns an Entry span that must survive. - */ @Test - public void ioThreadSegmentIsFinishedAndCallerEntrySpanSurvives() throws Exception { - when(response.getCode()).thenReturn(200); - Throwable[] ioError = new Throwable[1]; - Thread ioThread = new Thread(() -> { - try { - // what IOSessionImplPollInterceptor does - AbstractSpan local = ContextManager.createLocalSpan("httpasyncclient/local"); - httpContext.setAttribute(Constants.SKYWALKING_LOCAL_SPAN, local); - ContextManager.createExitSpan("/hello", new ContextCarrier(), "127.0.0.1:8080"); - // response headers arrive on the I/O thread - new AsyncResponseConsumerWrapper<>(consumer).consumeResponse(response, null, httpContext, null); - } catch (Throwable t) { - ioError[0] = t; - } - }); - ioThread.start(); - ioThread.join(); - assertThat(ioError[0] == null, is(true)); - - // local + exit span are both finished, so the I/O thread's segment is complete - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - assertThat(SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)).size(), is(2)); + public void failedKeepsCallerSpanActive() { + AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); + AsyncExitSpan exitSpan = createStartedExitSpan(); + Exception cause = new RuntimeException("boom"); - // entity read to EOF on the business thread triggers the callback there - AbstractSpan entry = ContextManager.createEntrySpan("/business", null); - new FutureCallbackWrapper<>(delegate, httpContext).completed("body"); + new FutureCallbackWrapper<>(delegate, exitSpan).failed(cause); assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == entry, is(true)); - ContextManager.stopSpan(entry); + assertThat(ContextManager.activeSpan() == callerSpan, is(true)); + verify(delegate).failed(cause); - assertThat(segmentStorage.getTraceSegments().size(), is(2)); - assertThat(SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(1)).size(), is(1)); - verify(delegate).completed("body"); + ContextManager.stopSpan(callerSpan); + + assertThat(segmentStorage.getTraceSegments().size(), is(1)); } @Test - public void nullContextNeverStopsForeignSpan() { - AbstractSpan entry = ContextManager.createEntrySpan("/business", null); + public void cancelledKeepsCallerSpanActive() { + AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); + AsyncExitSpan exitSpan = createStartedExitSpan(); - new FutureCallbackWrapper<>(delegate, null).completed("ok"); + new FutureCallbackWrapper<>(delegate, exitSpan).cancelled(); assertThat(ContextManager.isActive(), is(true)); - ContextManager.stopSpan(entry); - verify(delegate).completed("ok"); + assertThat(ContextManager.activeSpan() == callerSpan, is(true)); + verify(delegate).cancelled(); + + ContextManager.stopSpan(callerSpan); + + assertThat(segmentStorage.getTraceSegments().size(), is(1)); } -} +} \ No newline at end of file From f1dfe75e4b4bd67fc245954658fcc835d4d3341e Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Tue, 22 Sep 2026 22:43:31 +0530 Subject: [PATCH 3/8] Refine HttpClient 5.x async span handling --- .../wrapper/AsyncRequestProducerWrapper.java | 89 +++++++++++++------ 1 file changed, 63 insertions(+), 26 deletions(-) diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java index 5a81b592e0..ba03990eae 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java @@ -18,7 +18,10 @@ package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; import java.io.IOException; +import java.net.URI; +import java.net.URISyntaxException; import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.nio.AsyncRequestProducer; import org.apache.hc.core5.http.nio.DataStreamChannel; @@ -30,11 +33,15 @@ import org.apache.skywalking.apm.agent.core.context.tag.Tags; import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer; +import org.apache.skywalking.apm.agent.core.logging.api.ILog; +import org.apache.skywalking.apm.agent.core.logging.api.LogManager; import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncExitSpan; public class AsyncRequestProducerWrapper implements AsyncRequestProducer { + private static final ILog LOGGER = LogManager.getLogger(AsyncRequestProducerWrapper.class); + private final AsyncRequestProducer producer; private final AsyncExitSpan exitSpan; @@ -53,8 +60,8 @@ public void sendRequest(RequestChannel channel, HttpContext context) throws IOEx if (exitSpan.claimCreation()) { try { startExitSpan(request); - } catch (Throwable ignored) { - // Never let tracing instrumentation break the user's HTTP request. + } catch (Throwable t) { + LOGGER.error("Failed to trace the async HTTP request.", t); } } @@ -62,39 +69,69 @@ public void sendRequest(RequestChannel channel, HttpContext context) throws IOEx }, context); } - private void startExitSpan(HttpRequest request) { - String operationName = request.getRequestUri(); - String remotePeer = exitSpan.getTarget().toHostString(); - - ContextCarrier contextCarrier = new ContextCarrier(); - AbstractSpan span = ContextManager.createExitSpan( - operationName, - contextCarrier, - remotePeer - ); + private void startExitSpan(HttpRequest request) throws URISyntaxException { + URI uri = request.getUri(); + HttpHost target = exitSpan.getTarget(); - boolean nested = ContextManager.activeSpan().isExit(); + String scheme = target != null ? target.getSchemeName() : uri.getScheme(); + String host = target != null ? target.getHostName() : uri.getHost(); + int port = target != null ? target.getPort() : uri.getPort(); - if (!nested) { - span.setComponent(ComponentsDefine.HTTP_ASYNC_CLIENT); - Tags.URL.set(span, request.getRequestUri()); - SpanLayer.asHttp(span); + if (host == null) { + return; } - CarrierItem next = contextCarrier.items(); - while (next.hasNext()) { - request.setHeader(next.getHeadKey(), next.getHeadValue()); - next = next.next(); + if (scheme == null) { + scheme = "http"; } - if (!nested) { - span.prepareForAsync(); + if (port < 0) { + port = "https".equalsIgnoreCase(scheme) ? 443 : 80; } - ContextManager.stopSpan(span); + String peer = host + ":" + port; + + String path = uri.getPath() == null || uri.getPath().isEmpty() + ? "/" + : uri.getPath(); + + String url = scheme + "://" + peer + path + + (uri.getRawQuery() == null ? "" : "?" + uri.getRawQuery()); + + /* + * Check whether an exit span was already active BEFORE creating + * this request's span. + */ + boolean nested = ContextManager.isActive() && ContextManager.activeSpan().isExit(); + + AbstractSpan span = ContextManager.createExitSpan(path, peer); + + try { + if (!nested) { + span.setComponent(ComponentsDefine.HTTP_ASYNC_CLIENT); + Tags.URL.set(span, url); + Tags.HTTP.METHOD.set(span, request.getMethod()); + SpanLayer.asHttp(span); + } + + ContextCarrier carrier = new ContextCarrier(); + ContextManager.inject(carrier); - if (!nested) { - exitSpan.start(span); + CarrierItem next = carrier.items(); + while (next.hasNext()) { + next = next.next(); + request.setHeader(next.getHeadKey(), next.getHeadValue()); + } + } finally { + if (!nested) { + span.prepareForAsync(); + } + + ContextManager.stopSpan(span); + + if (!nested) { + exitSpan.start(span); + } } } From 2235033b28e8a821c377ac98ec71d88dfda0c4d0 Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Tue, 22 Sep 2026 22:52:16 +0530 Subject: [PATCH 4/8] Add async request producer tracing test --- .../v5/AsyncRequestProducerWrapperTest.java | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java new file mode 100644 index 0000000000..50864cbc7d --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5; + +import java.util.concurrent.atomic.AtomicReference; +import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.nio.AsyncRequestProducer; +import org.apache.hc.core5.http.nio.RequestChannel; +import org.apache.hc.core5.http.protocol.BasicHttpContext; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.message.BasicHttpRequest; +import org.apache.skywalking.apm.agent.core.boot.ServiceManager; +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; +import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; +import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; +import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; +import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncRequestProducerWrapper; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.verify; + +@RunWith(TracingSegmentRunner.class) +public class AsyncRequestProducerWrapperTest { + + @SegmentStoragePoint + private SegmentStorage segmentStorage; + + @Rule + public AgentServiceRule agentServiceRule = new AgentServiceRule(); + + @Rule + public MockitoRule rule = MockitoJUnit.rule(); + + @Mock + private AsyncRequestProducer producer; + + @Mock + private RequestChannel requestChannel; + + @Before + public void setUp() { + ServiceManager.INSTANCE.boot(); + } + + @Test + public void createsExitSpanAndInjectsPropagationHeader() throws Exception { + AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); + + HttpHost target = new HttpHost("http", "127.0.0.1", 8080); + AsyncExitSpan exitSpan = new AsyncExitSpan(target); + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper(producer, exitSpan); + + HttpRequest request = new BasicHttpRequest( + "GET", + "http://127.0.0.1:8080/hello" + ); + HttpContext context = new BasicHttpContext(); + AtomicReference sentRequest = new AtomicReference<>(); + + doAnswer(invocation -> { + RequestChannel wrappedChannel = invocation.getArgument(0); + + wrappedChannel.sendRequest(request, null, context); + return null; + }).when(producer).sendRequest(any(RequestChannel.class), any(HttpContext.class)); + + doAnswer(invocation -> { + sentRequest.set(invocation.getArgument(0)); + return null; + }).when(requestChannel).sendRequest(any(), any(), any()); + + wrapper.sendRequest(requestChannel, context); + + assertThat(ContextManager.isActive(), is(true)); + assertThat(ContextManager.activeSpan() == callerSpan, is(true)); + assertThat(sentRequest.get(), notNullValue()); + assertThat(sentRequest.get().getFirstHeader("sw8") != null, is(true)); + + exitSpan.finish(); + + ContextManager.stopSpan(callerSpan); + + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + + verify(producer).sendRequest(any(RequestChannel.class), any(HttpContext.class)); + verify(requestChannel).sendRequest(any(), any(), any()); + } +} \ No newline at end of file From 185aea3cc1008eb25ca0227c0e6de39d7b0936f9 Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Tue, 22 Sep 2026 23:12:37 +0530 Subject: [PATCH 5/8] Add interleaved async request test --- .../v5/AsyncRequestProducerWrapperTest.java | 292 +++++++++++------- 1 file changed, 176 insertions(+), 116 deletions(-) diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java index 50864cbc7d..2cef86fdfe 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java @@ -1,117 +1,177 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.skywalking.apm.plugin.httpclient.v5; - -import java.util.concurrent.atomic.AtomicReference; -import org.apache.hc.core5.http.HttpHost; -import org.apache.hc.core5.http.HttpRequest; -import org.apache.hc.core5.http.nio.AsyncRequestProducer; -import org.apache.hc.core5.http.nio.RequestChannel; -import org.apache.hc.core5.http.protocol.BasicHttpContext; -import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.hc.core5.http.message.BasicHttpRequest; -import org.apache.skywalking.apm.agent.core.boot.ServiceManager; -import org.apache.skywalking.apm.agent.core.context.ContextManager; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; -import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; -import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; -import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; -import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; -import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncRequestProducerWrapper; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnit; -import org.mockito.junit.MockitoRule; - -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.notNullValue; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.verify; - -@RunWith(TracingSegmentRunner.class) -public class AsyncRequestProducerWrapperTest { - - @SegmentStoragePoint - private SegmentStorage segmentStorage; - - @Rule - public AgentServiceRule agentServiceRule = new AgentServiceRule(); - - @Rule - public MockitoRule rule = MockitoJUnit.rule(); - - @Mock - private AsyncRequestProducer producer; - - @Mock - private RequestChannel requestChannel; - - @Before - public void setUp() { - ServiceManager.INSTANCE.boot(); - } - - @Test - public void createsExitSpanAndInjectsPropagationHeader() throws Exception { - AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); - - HttpHost target = new HttpHost("http", "127.0.0.1", 8080); - AsyncExitSpan exitSpan = new AsyncExitSpan(target); - AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper(producer, exitSpan); - - HttpRequest request = new BasicHttpRequest( - "GET", - "http://127.0.0.1:8080/hello" - ); - HttpContext context = new BasicHttpContext(); - AtomicReference sentRequest = new AtomicReference<>(); - - doAnswer(invocation -> { - RequestChannel wrappedChannel = invocation.getArgument(0); - - wrappedChannel.sendRequest(request, null, context); - return null; - }).when(producer).sendRequest(any(RequestChannel.class), any(HttpContext.class)); - - doAnswer(invocation -> { - sentRequest.set(invocation.getArgument(0)); - return null; - }).when(requestChannel).sendRequest(any(), any(), any()); - - wrapper.sendRequest(requestChannel, context); - - assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == callerSpan, is(true)); - assertThat(sentRequest.get(), notNullValue()); - assertThat(sentRequest.get().getFirstHeader("sw8") != null, is(true)); - - exitSpan.finish(); - - ContextManager.stopSpan(callerSpan); - - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - - verify(producer).sendRequest(any(RequestChannel.class), any(HttpContext.class)); - verify(requestChannel).sendRequest(any(), any(), any()); - } +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5; + +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.nio.AsyncRequestProducer; +import org.apache.hc.core5.http.nio.RequestChannel; +import org.apache.hc.core5.http.protocol.BasicHttpContext; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.message.BasicHttpRequest; +import org.apache.skywalking.apm.agent.core.boot.ServiceManager; +import org.apache.skywalking.apm.agent.core.context.ContextCarrier; +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractTracingSpan; +import org.apache.skywalking.apm.agent.test.helper.SegmentHelper; +import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; +import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; +import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; +import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; +import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncRequestProducerWrapper; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.verify; + +@RunWith(TracingSegmentRunner.class) +public class AsyncRequestProducerWrapperTest { + + @SegmentStoragePoint + private SegmentStorage segmentStorage; + + @Rule + public AgentServiceRule agentServiceRule = new AgentServiceRule(); + + @Rule + public MockitoRule rule = MockitoJUnit.rule(); + + @Mock + private AsyncRequestProducer producer; + + @Mock + private RequestChannel requestChannel; + + @Before + public void setUp() { + ServiceManager.INSTANCE.boot(); + } + + @Test + public void createsExitSpanAndInjectsPropagationHeader() throws Exception { + AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); + + HttpHost target = new HttpHost("http", "127.0.0.1", 8080); + AsyncExitSpan exitSpan = new AsyncExitSpan(target); + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper(producer, exitSpan); + + HttpRequest request = new BasicHttpRequest( + "GET", + "http://127.0.0.1:8080/hello" + ); + HttpContext context = new BasicHttpContext(); + AtomicReference sentRequest = new AtomicReference<>(); + + doAnswer(invocation -> { + RequestChannel wrappedChannel = invocation.getArgument(0); + + wrappedChannel.sendRequest(request, null, context); + return null; + }).when(producer).sendRequest(any(RequestChannel.class), any(HttpContext.class)); + + doAnswer(invocation -> { + sentRequest.set(invocation.getArgument(0)); + return null; + }).when(requestChannel).sendRequest(any(), any(), any()); + + wrapper.sendRequest(requestChannel, context); + + assertThat(ContextManager.isActive(), is(true)); + assertThat(ContextManager.activeSpan() == callerSpan, is(true)); + assertThat(sentRequest.get(), notNullValue()); + assertThat(sentRequest.get().getFirstHeader("sw8") != null, is(true)); + + exitSpan.finish(); + + ContextManager.stopSpan(callerSpan); + + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + + verify(producer).sendRequest(any(RequestChannel.class), any(HttpContext.class)); + verify(requestChannel).sendRequest(any(), any(), any()); + } + + @Test + public void interleavedRequestsDoNotShareCallerSpanStack() throws Exception { + AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); + + AsyncExitSpan firstRequest = createAsyncExitSpan("/first"); + AsyncExitSpan secondRequest = createAsyncExitSpan("/second"); + + Thread reactorThread = new Thread(() -> { + // Both requests are already detached from the caller's span stack. + // Finish them in reverse order to simulate response interleaving. + secondRequest.onResponse(500); + firstRequest.onResponse(200); + + secondRequest.finish(); + firstRequest.finish(); + + // No request span should have been pushed onto this reactor thread. + assertThat(ContextManager.isActive(), is(false)); + }); + + reactorThread.start(); + reactorThread.join(); + + // Finishing the async spans must not affect the caller's active span. + assertThat(ContextManager.isActive(), is(true)); + assertThat(ContextManager.activeSpan() == callerSpan, is(true)); + + ContextManager.stopSpan(callerSpan); + + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + + List spans = + SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); + + // One caller entry span + two independently finished request exit spans. + assertThat(spans.size(), is(3)); + } + + private AsyncExitSpan createAsyncExitSpan(String operationName) { + AsyncExitSpan exitSpan = new AsyncExitSpan( + new HttpHost("http", "127.0.0.1", 8080) + ); + + AbstractSpan requestSpan = ContextManager.createExitSpan( + operationName, + new ContextCarrier(), + "127.0.0.1:8080" + ); + + exitSpan.start(requestSpan); + requestSpan.prepareForAsync(); + ContextManager.stopSpan(requestSpan); + + return exitSpan; + } } \ No newline at end of file From d5dad34c9f7256342d3ce0e26432b7ec84cb31ef Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Tue, 22 Sep 2026 23:31:54 +0530 Subject: [PATCH 6/8] Add async response consumer lifecycle tests --- .../v5/AsyncResponseConsumerWrapperTest.java | 235 ++++++++++++++++++ 1 file changed, 235 insertions(+) create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncResponseConsumerWrapperTest.java diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncResponseConsumerWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncResponseConsumerWrapperTest.java new file mode 100644 index 0000000000..6924c197ba --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncResponseConsumerWrapperTest.java @@ -0,0 +1,235 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5; + +import java.util.List; +import org.apache.hc.core5.concurrent.FutureCallback; +import org.apache.hc.core5.http.EntityDetails; +import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpResponse; +import org.apache.hc.core5.http.nio.AsyncResponseConsumer; +import org.apache.hc.core5.http.protocol.BasicHttpContext; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.skywalking.apm.agent.core.boot.ServiceManager; +import org.apache.skywalking.apm.agent.core.context.ContextCarrier; +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractTracingSpan; +import org.apache.skywalking.apm.agent.test.helper.SegmentHelper; +import org.apache.skywalking.apm.agent.test.helper.SpanHelper; +import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; +import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; +import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; +import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; +import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncResponseConsumerWrapper; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@RunWith(TracingSegmentRunner.class) +public class AsyncResponseConsumerWrapperTest { + + @SegmentStoragePoint + private SegmentStorage segmentStorage; + + @Rule + public AgentServiceRule agentServiceRule = new AgentServiceRule(); + + @Rule + public MockitoRule rule = MockitoJUnit.rule(); + + @Mock + private AsyncResponseConsumer consumer; + + @Mock + private HttpResponse response; + + @Mock + private EntityDetails entityDetails; + + @Before + public void setUp() { + ServiceManager.INSTANCE.boot(); + } + + @Test + public void responseWithoutEntityFinishesSpan() throws Exception { + when(response.getCode()).thenReturn(200); + + AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); + AsyncExitSpan exitSpan = createAsyncExitSpan("/no-body"); + + AsyncResponseConsumerWrapper wrapper = + new AsyncResponseConsumerWrapper<>(consumer, exitSpan); + + HttpContext context = new BasicHttpContext(); + + wrapper.consumeResponse(response, null, context, (FutureCallback) null); + + assertThat(ContextManager.isActive(), is(true)); + assertThat(ContextManager.activeSpan() == callerSpan, is(true)); + + ContextManager.stopSpan(callerSpan); + + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + + List spans = + SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); + + assertThat(spans.size(), is(2)); + + AbstractTracingSpan responseSpan = findSpan(spans, "/no-body"); + assertThat(responseSpan, notNullValue()); + assertThat(SpanHelper.getErrorOccurred(responseSpan), is(false)); + + verify(consumer).consumeResponse(response, null, context, null); + } + + @Test + public void releaseBeforeFailureMarksSpanAsError() throws Exception { + when(response.getCode()).thenReturn(200); + + AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); + AsyncExitSpan exitSpan = createAsyncExitSpan("/body-failure"); + + AsyncResponseConsumerWrapper wrapper = + new AsyncResponseConsumerWrapper<>(consumer, exitSpan); + + HttpContext context = new BasicHttpContext(); + RuntimeException cause = new RuntimeException("body read failed"); + + // Response headers were received successfully, but the body will fail. + wrapper.consumeResponse( + response, + entityDetails, + context, + (FutureCallback) null + ); + + // releaseResources() can happen before failed(). The span must + // therefore be finished as an error rather than as a success. + wrapper.releaseResources(); + + // The later failure callback must not modify an already finished span. + wrapper.failed(cause); + + assertThat(ContextManager.isActive(), is(true)); + assertThat(ContextManager.activeSpan() == callerSpan, is(true)); + + ContextManager.stopSpan(callerSpan); + + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + + List spans = + SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); + + assertThat(spans.size(), is(2)); + + AbstractTracingSpan responseSpan = findSpan(spans, "/body-failure"); + assertThat(responseSpan, notNullValue()); + assertThat(SpanHelper.getErrorOccurred(responseSpan), is(true)); + + verify(consumer).consumeResponse( + response, + entityDetails, + context, + null + ); + verify(consumer).releaseResources(); + verify(consumer).failed(cause); + } + + @Test + public void streamEndFinishesResponseSpan() throws Exception { + when(response.getCode()).thenReturn(200); + + AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); + AsyncExitSpan exitSpan = createAsyncExitSpan("/body"); + + AsyncResponseConsumerWrapper wrapper = + new AsyncResponseConsumerWrapper<>(consumer, exitSpan); + + HttpContext context = new BasicHttpContext(); + + wrapper.consumeResponse( + response, + entityDetails, + context, + (FutureCallback) null + ); + + wrapper.streamEnd(null); + + assertThat(ContextManager.isActive(), is(true)); + assertThat(ContextManager.activeSpan() == callerSpan, is(true)); + + ContextManager.stopSpan(callerSpan); + + assertThat(segmentStorage.getTraceSegments().size(), is(1)); + + List spans = + SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); + + assertThat(spans.size(), is(2)); + + AbstractTracingSpan responseSpan = findSpan(spans, "/body"); + assertThat(responseSpan, notNullValue()); + assertThat(SpanHelper.getErrorOccurred(responseSpan), is(false)); + + verify(consumer).streamEnd(null); + } + + private AsyncExitSpan createAsyncExitSpan(String operationName) { + AsyncExitSpan exitSpan = new AsyncExitSpan( + new HttpHost("http", "127.0.0.1", 8080) + ); + + AbstractSpan requestSpan = ContextManager.createExitSpan( + operationName, + new ContextCarrier(), + "127.0.0.1:8080" + ); + + exitSpan.start(requestSpan); + requestSpan.prepareForAsync(); + ContextManager.stopSpan(requestSpan); + + return exitSpan; + } + + private AbstractTracingSpan findSpan( + List spans, + String operationName) { + for (AbstractTracingSpan span : spans) { + if (operationName.equals(span.getOperationName())) { + return span; + } + } + return null; + } +} \ No newline at end of file From 4989a1008991e88ab148df2f59100960bbac51f7 Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Fri, 25 Sep 2026 07:55:14 +0000 Subject: [PATCH 7/8] Refine --- .../plugin/httpclient/v5/AsyncExitSpan.java | 91 ------- .../httpclient/v5/AsyncRequestSpans.java | 140 +++++++++++ .../HttpAsyncClientDoExecuteInterceptor.java | 76 ++++-- .../define/IOSessionImplInstrumentation.java | 68 ----- .../wrapper/AsyncRequestProducerWrapper.java | 99 ++++---- .../wrapper/AsyncResponseConsumerWrapper.java | 76 +++--- .../v5/wrapper/FutureCallbackWrapper.java | 43 ++-- .../v5/AsyncRequestProducerWrapperTest.java | 177 ------------- .../httpclient/v5/AsyncRequestSpansTest.java | 162 ++++++++++++ .../v5/AsyncResponseConsumerWrapperTest.java | 235 ------------------ .../v5/FutureCallbackWrapperTest.java | 128 ---------- .../AsyncRequestProducerWrapperTest.java | 218 ++++++++++++++++ .../AsyncResponseConsumerWrapperTest.java | 199 +++++++++++++++ .../v5/wrapper/FutureCallbackWrapperTest.java | 129 ++++++++++ 14 files changed, 1026 insertions(+), 815 deletions(-) delete mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncExitSpan.java create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestSpans.java delete mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/define/IOSessionImplInstrumentation.java delete mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestSpansTest.java delete mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncResponseConsumerWrapperTest.java delete mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapperTest.java create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapperTest.java create mode 100644 apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapperTest.java diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncExitSpan.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncExitSpan.java deleted file mode 100644 index 2565b2579c..0000000000 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncExitSpan.java +++ /dev/null @@ -1,91 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package org.apache.skywalking.apm.plugin.httpclient.v5; - -import org.apache.hc.core5.http.HttpHost; -import org.apache.skywalking.apm.agent.core.context.tag.Tags; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; - -public class AsyncExitSpan { - private final HttpHost target; - private volatile Thread creator = Thread.currentThread(); - private AbstractSpan span; - - public AsyncExitSpan(HttpHost target) { - this.target = target; - } - - public HttpHost getTarget() { - return target; - } - - public boolean claimCreation() { - if (creator != Thread.currentThread()) { - return false; - } - creator = null; - return true; - } - - public void callerReturned() { - creator = null; - } - - public synchronized void start(AbstractSpan span) { - this.span = span; - } - - public synchronized void onResponse(int statusCode) { - if (span != null) { - Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode); - if (statusCode >= 400) { - span.errorOccurred(); - } - } - } - - public synchronized void finish() { - end(false, null); - } - - public synchronized void fail(Throwable cause) { - end(true, cause); - } - - public synchronized void abort() { - end(true, null); - } - - private void end(boolean error, Throwable cause) { - if (span == null) { - return; - } - - if (error) { - span.errorOccurred(); - } - - if (cause != null) { - span.log(cause); - } - - span.asyncFinish(); - span = null; - } -} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestSpans.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestSpans.java new file mode 100644 index 0000000000..dfb24da289 --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestSpans.java @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5; + +import java.util.concurrent.atomic.AtomicReference; +import org.apache.hc.core5.http.HttpHost; +import org.apache.skywalking.apm.agent.core.context.tag.Tags; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; + +/** + * Per-request async exit span, owned by the request itself rather than by whatever thread happens to be running + * when a callback fires. + * + *

The span is created once, on the caller thread inside {@code doExecute}, while the caller's tracing context is + * still active. It is then immediately detached via {@link AbstractSpan#prepareForAsync()} + + * {@code ContextManager.stopSpan(span)} so it never sits on any thread's active-span stack while the request is in + * flight. From that point on it is finished exactly once, by reference, from whichever lifecycle callback gets + * there first (I/O thread response consumer, or the future callback on the caller/business thread) — never by a + * parameterless {@code ContextManager.stopSpan()} that would blindly pop whatever span is currently active on that + * thread. + * + *

All mutating operations are synchronized: {@link #onResponse(int)} (tagging, typically the I/O thread) can + * otherwise race with {@link #finish()} / {@link #fail(Throwable)} (typically the response-consumer or callback + * thread) finishing and clearing the span in the same window. An {@link AtomicReference} alone would prevent a + * double-finish but not a tag-write racing a finish. + */ +public class AsyncRequestSpans { + + private final HttpHost target; + + /** + * Only true, and only once, on the thread that is still inside {@code doExecute} when the request producer + * hands the concrete request to the channel. Any other thread (a custom {@code AsyncRequestProducer} that + * defers sending) has no relationship to the caller's context, so it must not create a span. + */ + private final AtomicReference creator = new AtomicReference<>(Thread.currentThread()); + + private AbstractSpan span; + private boolean finished; + + public AsyncRequestSpans(HttpHost target) { + this.target = target; + } + + public HttpHost getTarget() { + return target; + } + + /** + * Claims the right to create the span. Returns {@code true} at most once, and only for the thread that + * constructed this holder (the {@code doExecute} caller thread). + */ + public boolean claimCreation() { + Thread current = Thread.currentThread(); + return creator.compareAndSet(current, null); + } + + /** + * Called at the end of {@code doExecute} (success or failure) so a late/duplicate send from the same thread + * cannot still claim creation after the caller has moved on. + */ + public void callerReturned() { + creator.set(null); + } + + /** + * Stores the span. Must be called only after the span has already been detached with + * {@code prepareForAsync()} + {@code ContextManager.stopSpan(span)} — this class never touches the active-span + * stack itself. + */ + public synchronized void start(AbstractSpan span) { + this.span = span; + } + + public synchronized void onResponse(int statusCode) { + if (span == null || finished) { + return; + } + + Tags.HTTP_RESPONSE_STATUS_CODE.set(span, statusCode); + + if (statusCode >= 400) { + span.errorOccurred(); + } + } + + /** The whole response completed successfully. */ + public synchronized void finish() { + end(false, null); + } + + /** The exchange failed with an exception. */ + public synchronized void fail(Throwable cause) { + end(true, cause); + } + + /** + * Cancelled, or resources released before the response ever completed (e.g. a redirect exec that declines to + * resend a non-repeatable entity and never invokes {@code completed()}). Only takes effect if the span is + * still open — the normal-completion paths already finished it earlier, so this is then a no-op. + */ + public synchronized void abort() { + end(true, null); + } + + private void end(boolean error, Throwable cause) { + if (span == null || finished) { + return; + } + + finished = true; + + if (error) { + span.errorOccurred(); + } + + if (cause != null) { + span.log(cause); + } + + span.asyncFinish(); + span = null; + } +} \ No newline at end of file diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java index 71e0b1ca2d..8e8fb92fc8 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/HttpAsyncClientDoExecuteInterceptor.java @@ -31,40 +31,80 @@ import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncResponseConsumerWrapper; import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.FutureCallbackWrapper; +/** + * Intercepts the internal {@code doExecute(HttpHost, AsyncRequestProducer, AsyncResponseConsumer, ..., FutureCallback)} + * overload shared by every async client implementation (Internal*AsyncClient, Minimal*AsyncClient, and the + * classic-facade adapter), whose argument order/types are identical across HttpClient 5.0 through 5.6. + * + *

Unlike the previous implementation, this interceptor never stores anything in the {@code HttpContext} and + * never wraps a callback purely to call a parameterless {@code ContextManager.stopSpan()}. It only: + *

    + *
  1. creates a per-request {@link AsyncRequestSpans} holder, while the caller's context is still active;
    2. + *
  2. wraps the request producer so the exit span is created on the caller thread, synchronously, the moment the + * concrete {@code HttpRequest} becomes available;
    3. + *
  3. wraps the response consumer and future callback so the retained span is finished by reference.
    4. + *
+ * Because span creation no longer depends on the {@code HttpContext}, this also fixes HttpClient 5.4+, where the + * context argument passed by the classic facade and by {@code execute(SimpleHttpRequest, FutureCallback)} is + * {@code null}. + */ public class HttpAsyncClientDoExecuteInterceptor implements InstanceMethodsAroundInterceptor { + private static final int TARGET_INDEX = 0; + private static final int REQUEST_PRODUCER_INDEX = 1; + private static final int RESPONSE_CONSUMER_INDEX = 2; + private static final int CALLBACK_INDEX = 5; + @Override - public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, - MethodInterceptResult result) throws Throwable { + public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, + Class[] argumentsTypes, MethodInterceptResult result) throws Throwable { if (!ContextManager.isActive()) { return; } + if (!(allArguments[REQUEST_PRODUCER_INDEX] instanceof AsyncRequestProducer) + || !(allArguments[RESPONSE_CONSUMER_INDEX] instanceof AsyncResponseConsumer)) { + return; + } - AsyncExitSpan exitSpan = new AsyncExitSpan((HttpHost) allArguments[0]); - - AsyncRequestProducer producer = (AsyncRequestProducer) allArguments[1]; - AsyncResponseConsumer consumer = (AsyncResponseConsumer) allArguments[2]; - FutureCallback callback = (FutureCallback) allArguments[5]; + final HttpHost target = allArguments[TARGET_INDEX] instanceof HttpHost + ? (HttpHost) allArguments[TARGET_INDEX] : null; + final AsyncRequestSpans spans = new AsyncRequestSpans(target); - allArguments[1] = new AsyncRequestProducerWrapper(producer, exitSpan); - allArguments[2] = new AsyncResponseConsumerWrapper(consumer, exitSpan); - allArguments[5] = new FutureCallbackWrapper(callback, exitSpan); + allArguments[REQUEST_PRODUCER_INDEX] = new AsyncRequestProducerWrapper( + (AsyncRequestProducer) allArguments[REQUEST_PRODUCER_INDEX], spans); + allArguments[RESPONSE_CONSUMER_INDEX] = new AsyncResponseConsumerWrapper<>( + (AsyncResponseConsumer) allArguments[RESPONSE_CONSUMER_INDEX], spans); + // Wrap even when the caller passed null: it's the only lifecycle hook that sees cancellation and the + // synchronous-failure-before-consumer-runs path for callers who supplied no callback of their own. + allArguments[CALLBACK_INDEX] = new FutureCallbackWrapper<>( + (FutureCallback) allArguments[CALLBACK_INDEX], spans); } @Override - public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class[] argumentsTypes, - Object ret) throws Throwable { - if (allArguments[1] instanceof AsyncRequestProducerWrapper) { - ((AsyncRequestProducerWrapper) allArguments[1]).getExitSpan().callerReturned(); - } + public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, + Class[] argumentsTypes, Object ret) throws Throwable { + releaseCreationClaim(allArguments); return ret; } @Override public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, - Class[] argumentsTypes, Throwable t) { - if (allArguments[1] instanceof AsyncRequestProducerWrapper) { - ((AsyncRequestProducerWrapper) allArguments[1]).getExitSpan().fail(t); + Class[] argumentsTypes, Throwable t) { + if (allArguments[REQUEST_PRODUCER_INDEX] instanceof AsyncRequestProducerWrapper) { + AsyncRequestProducerWrapper wrapper = (AsyncRequestProducerWrapper) allArguments[REQUEST_PRODUCER_INDEX]; + wrapper.getSpans().fail(t); + } + releaseCreationClaim(allArguments); + } + + /** + * Once {@code doExecute} has returned (or thrown), no thread other than a genuinely deferred custom producer + * has any business claiming span creation — clearing this here keeps {@link AsyncRequestSpans#claimCreation()} + * honest even if the same thread somehow re-enters. + */ + private void releaseCreationClaim(Object[] allArguments) { + if (allArguments[REQUEST_PRODUCER_INDEX] instanceof AsyncRequestProducerWrapper) { + ((AsyncRequestProducerWrapper) allArguments[REQUEST_PRODUCER_INDEX]).getSpans().callerReturned(); } } } diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/define/IOSessionImplInstrumentation.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/define/IOSessionImplInstrumentation.java deleted file mode 100644 index 5841089101..0000000000 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/define/IOSessionImplInstrumentation.java +++ /dev/null @@ -1,68 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package org.apache.skywalking.apm.plugin.httpclient.v5.define; - -import net.bytebuddy.description.method.MethodDescription; -import net.bytebuddy.matcher.ElementMatcher; -import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint; -import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint; -import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine; -import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch; - -import static net.bytebuddy.matcher.ElementMatchers.named; -import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName; - -public class IOSessionImplInstrumentation extends ClassInstanceMethodsEnhancePluginDefine { - - private static final String ENHANCE_CLASS = "org.apache.hc.core5.reactor.IOSessionImpl"; - private static final String METHOD_NAME = "poll"; - private static final String INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.httpclient.v5.IOSessionImplPollInterceptor"; - - @Override - protected ClassMatch enhanceClass() { - return byName(ENHANCE_CLASS); - } - - @Override - public ConstructorInterceptPoint[] getConstructorsInterceptPoints() { - return null; - } - - @Override - public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() { - return new InstanceMethodsInterceptPoint[]{ - new InstanceMethodsInterceptPoint() { - @Override - public ElementMatcher getMethodsMatcher() { - return named(METHOD_NAME); - } - - @Override - public String getMethodsInterceptor() { - return INTERCEPT_CLASS; - } - - @Override - public boolean isOverrideArgs() { - return false; - } - } - }; - } -} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java index ba03990eae..f355180657 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapper.java @@ -13,6 +13,7 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. + * */ package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; @@ -36,76 +37,74 @@ import org.apache.skywalking.apm.agent.core.logging.api.ILog; import org.apache.skywalking.apm.agent.core.logging.api.LogManager; import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; -import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncExitSpan; - +import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncRequestSpans; + +/** + * Delegates every {@link AsyncRequestProducer} method unchanged, except {@link #sendRequest}, where it wraps the + * {@link RequestChannel} the underlying producer is handed. All standard producers (Internal/Minimal async + * clients, and the classic-facade adapter) call {@code channel.sendRequest(...)} synchronously, on the calling + * thread, from inside {@code doExecute} — so this is where the concrete {@link HttpRequest} first becomes + * available, while the caller's tracing context is still active. + */ public class AsyncRequestProducerWrapper implements AsyncRequestProducer { private static final ILog LOGGER = LogManager.getLogger(AsyncRequestProducerWrapper.class); private final AsyncRequestProducer producer; - private final AsyncExitSpan exitSpan; + private final AsyncRequestSpans spans; - public AsyncRequestProducerWrapper(AsyncRequestProducer producer, AsyncExitSpan exitSpan) { + public AsyncRequestProducerWrapper(AsyncRequestProducer producer, AsyncRequestSpans spans) { this.producer = producer; - this.exitSpan = exitSpan; + this.spans = spans; } - public AsyncExitSpan getExitSpan() { - return exitSpan; + public AsyncRequestSpans getSpans() { + return spans; } @Override - public void sendRequest(RequestChannel channel, HttpContext context) throws IOException, HttpException { - producer.sendRequest((request, entityDetails, requestContext) -> { - if (exitSpan.claimCreation()) { + public void sendRequest(RequestChannel channel, HttpContext context) throws HttpException, IOException { + producer.sendRequest((request, entityDetails, ctx) -> { + if (spans.claimCreation()) { try { startExitSpan(request); } catch (Throwable t) { - LOGGER.error("Failed to trace the async HTTP request.", t); + // Tracing must never break the user's actual HTTP request. + LOGGER.error(t, "Failed to trace the async HttpClient request."); } } - - channel.sendRequest(request, entityDetails, requestContext); + channel.sendRequest(request, entityDetails, ctx); }, context); } private void startExitSpan(HttpRequest request) throws URISyntaxException { URI uri = request.getUri(); - HttpHost target = exitSpan.getTarget(); - + HttpHost target = spans.getTarget(); + // Same precedence InternalAbstractHttpAsyncClient itself uses: an explicit target host wins over + // whatever authority happens to be on the request URI. String scheme = target != null ? target.getSchemeName() : uri.getScheme(); String host = target != null ? target.getHostName() : uri.getHost(); int port = target != null ? target.getPort() : uri.getPort(); - if (host == null) { return; } - if (scheme == null) { scheme = "http"; } - if (port < 0) { port = "https".equalsIgnoreCase(scheme) ? 443 : 80; } - String peer = host + ":" + port; + String path = uri.getPath() == null || uri.getPath().isEmpty() ? "/" : uri.getPath(); + String url = scheme + "://" + peer + path + (uri.getRawQuery() == null ? "" : "?" + uri.getRawQuery()); - String path = uri.getPath() == null || uri.getPath().isEmpty() - ? "/" - : uri.getPath(); - - String url = scheme + "://" + peer + path - + (uri.getRawQuery() == null ? "" : "?" + uri.getRawQuery()); - - /* - * Check whether an exit span was already active BEFORE creating - * this request's span. - */ - boolean nested = ContextManager.isActive() && ContextManager.activeSpan().isExit(); - - AbstractSpan span = ContextManager.createExitSpan(path, peer); + // If we're already inside another plugin's exit span, createExitSpan reuses that span (nested depth + 1) + // instead of creating a new one. We must not treat a reused outer span as ours to detach/finish + // asynchronously — that lifecycle belongs to whichever plugin created it. + boolean nested = ContextManager.activeSpan() != null && ContextManager.activeSpan().isExit(); + ContextCarrier carrier = new ContextCarrier(); + AbstractSpan span = ContextManager.createExitSpan(path, carrier, peer); try { if (!nested) { span.setComponent(ComponentsDefine.HTTP_ASYNC_CLIENT); @@ -113,10 +112,6 @@ private void startExitSpan(HttpRequest request) throws URISyntaxException { Tags.HTTP.METHOD.set(span, request.getMethod()); SpanLayer.asHttp(span); } - - ContextCarrier carrier = new ContextCarrier(); - ContextManager.inject(carrier); - CarrierItem next = carrier.items(); while (next.hasNext()) { next = next.next(); @@ -124,17 +119,22 @@ private void startExitSpan(HttpRequest request) throws URISyntaxException { } } finally { if (!nested) { + // Detach BEFORE returning control to the channel: the client can report a synchronous failure + // back to doExecute's own catch block on this very thread before sendRequest() returns. span.prepareForAsync(); - } - - ContextManager.stopSpan(span); - - if (!nested) { - exitSpan.start(span); + ContextManager.stopSpan(span); + spans.start(span); + } else { + ContextManager.stopSpan(span); } } } + @Override + public void failed(Exception cause) { + producer.failed(cause); + } + @Override public boolean isRepeatable() { return producer.isRepeatable(); @@ -150,13 +150,14 @@ public int available() { return producer.available(); } - @Override - public void failed(Exception cause) { - producer.failed(cause); - } - @Override public void releaseResources() { producer.releaseResources(); } -} \ No newline at end of file + + // NOTE FOR AYUSH: AsyncRequestProducer's exact method set has drifted slightly across httpcore5 minor + // versions (5.0 vs 5.3+). Let your IDE's "implement remaining interface methods" fill in anything missing + // here (there should be none beyond the above in 5.0-5.6, but verify against the version this module + // actually compiles against) — every one of them should be a plain one-line delegate to `producer`, same + // as above. The only method with real logic is sendRequest(). +} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java index cb5b0c361d..b8218f3ead 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapper.java @@ -13,82 +13,90 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. + * */ package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; import java.io.IOException; -import java.nio.ByteBuffer; -import java.util.List; import org.apache.hc.core5.http.EntityDetails; import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.nio.AsyncResponseConsumer; import org.apache.hc.core5.http.nio.CapacityChannel; import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.hc.core5.http.Header; -import org.apache.hc.core5.concurrent.FutureCallback; -import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncExitSpan; +import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncRequestSpans; +/** + * Runs entirely on the I/O thread (with the sole exception that {@code releaseResources()} can also be invoked + * from elsewhere during cleanup). Never touches {@code ContextManager}'s active-span stack — only ever tags or + * finishes {@link #spans} by reference, which is safe to do from any thread. + */ public class AsyncResponseConsumerWrapper implements AsyncResponseConsumer { private final AsyncResponseConsumer consumer; - private final AsyncExitSpan exitSpan; + private final AsyncRequestSpans spans; - public AsyncResponseConsumerWrapper( - AsyncResponseConsumer consumer, AsyncExitSpan exitSpan) { + public AsyncResponseConsumerWrapper(AsyncResponseConsumer consumer, AsyncRequestSpans spans) { this.consumer = consumer; - this.exitSpan = exitSpan; + this.spans = spans; } @Override - public void consumeResponse( - HttpResponse response, - EntityDetails entityDetails, - HttpContext context, - FutureCallback resultCallback) throws HttpException, IOException { - - exitSpan.onResponse(response.getCode()); - + public void consumeResponse(HttpResponse response, EntityDetails entityDetails, HttpContext context, + org.apache.hc.core5.concurrent.FutureCallback resultCallback) throws HttpException, IOException { + spans.onResponse(response.getCode()); if (entityDetails == null) { - exitSpan.finish(); + // No body means streamEnd() will never be called for this exchange. + spans.finish(); } - consumer.consumeResponse(response, entityDetails, context, resultCallback); } @Override - public void informationResponse( - HttpResponse response, - HttpContext context) throws HttpException, IOException { + public void informationResponse(HttpResponse response, HttpContext context) throws HttpException, IOException { + // 1xx is not the final response; the exit span's status must come from the final consumeResponse() call. consumer.informationResponse(response, context); } @Override - public void updateCapacity(CapacityChannel capacityChannel) throws IOException { - consumer.updateCapacity(capacityChannel); + public void streamEnd(java.util.List trailers) + throws HttpException, IOException { + spans.finish(); + consumer.streamEnd(trailers); } @Override - public void consume(ByteBuffer src) throws IOException { - consumer.consume(src); + public void failed(Exception cause) { + spans.fail(cause); + consumer.failed(cause); } @Override - public void streamEnd(List trailers) throws HttpException, IOException { - exitSpan.finish(); - consumer.streamEnd(trailers); + public void updateCapacity(CapacityChannel capacityChannel) throws IOException { + consumer.updateCapacity(capacityChannel); } @Override - public void failed(Exception cause) { - exitSpan.fail(cause); - consumer.failed(cause); + public void consume(java.nio.ByteBuffer src) throws IOException { + consumer.consume(src); } @Override public void releaseResources() { - exitSpan.abort(); + // Fallback finisher, not a success signal: HttpAsyncMainClientExec#failed calls releaseResources() + // *before* reporting the failure, and a suppressed-redirect-with-non-repeatable-entity exchange only + // ever calls completed() without a real response. abort() only takes effect if the span is still open — + // every normal-completion path above has already finished it by the time release runs, so this is then + // a no-op. If the span IS still open here, the exchange ended without a complete response, so it's + // correctly marked as an error rather than silently dropped. + spans.abort(); consumer.releaseResources(); } -} \ No newline at end of file + + // NOTE FOR AYUSH: same caveat as AsyncRequestProducerWrapper — let the IDE fill in any interface method not + // listed above (e.g. some httpcore5 versions' AsyncResponseConsumer exposes it slightly differently); every + // one you add should be a plain delegate to `consumer` with zero span logic. The five methods above + // (consumeResponse, informationResponse, streamEnd, failed, releaseResources) are the only ones that matter + // for span lifecycle. +} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java index eb63792c88..26ed80ac9e 100644 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapper.java @@ -13,47 +13,60 @@ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. + * */ package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; import org.apache.hc.core5.concurrent.FutureCallback; -import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncExitSpan; +import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncRequestSpans; +/** + * This is the class the original bug (#14097) lived in: the old implementation called the parameterless + * {@code ContextManager.stopSpan()} here, which pops whatever span is active on whatever thread happens to + * invoke this callback — and with {@code HttpAsyncClients.classic(...)}, that can be the caller/business + * thread, once it reads the response body to EOF. That thread's active span is the caller's own business span, + * not this HTTP request's span. + * + *

This version never touches the active-span stack. It only finishes {@link #spans} by reference, which is + * safe from any thread — the caller's own span is never at risk. + * + *

{@code completed}/{@code failed} are largely redundant with {@link AsyncResponseConsumerWrapper}'s own + * finish paths ({@link AsyncRequestSpans#finish()}/{@link AsyncRequestSpans#fail(Throwable)} are idempotent), but + * this callback still matters for {@link #cancelled()} — which the consumer never sees — and as a safety net for + * any exchange that completes without ever driving the consumer's normal lifecycle. + */ public class FutureCallbackWrapper implements FutureCallback { private final FutureCallback callback; - private final AsyncExitSpan exitSpan; + private final AsyncRequestSpans spans; - public FutureCallbackWrapper(FutureCallback callback, AsyncExitSpan exitSpan) { + public FutureCallbackWrapper(FutureCallback callback, AsyncRequestSpans spans) { this.callback = callback; - this.exitSpan = exitSpan; + this.spans = spans; } @Override - public void completed(T o) { - exitSpan.finish(); - + public void completed(T result) { + spans.finish(); if (callback != null) { - callback.completed(o); + callback.completed(result); } } @Override - public void failed(Exception e) { - exitSpan.fail(e); - + public void failed(Exception ex) { + spans.fail(ex); if (callback != null) { - callback.failed(e); + callback.failed(ex); } } @Override public void cancelled() { - exitSpan.abort(); - + spans.abort(); if (callback != null) { callback.cancelled(); } } -} \ No newline at end of file +} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java deleted file mode 100644 index 2cef86fdfe..0000000000 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestProducerWrapperTest.java +++ /dev/null @@ -1,177 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.skywalking.apm.plugin.httpclient.v5; - -import java.util.List; -import java.util.concurrent.atomic.AtomicReference; -import org.apache.hc.core5.http.HttpHost; -import org.apache.hc.core5.http.HttpRequest; -import org.apache.hc.core5.http.nio.AsyncRequestProducer; -import org.apache.hc.core5.http.nio.RequestChannel; -import org.apache.hc.core5.http.protocol.BasicHttpContext; -import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.hc.core5.http.message.BasicHttpRequest; -import org.apache.skywalking.apm.agent.core.boot.ServiceManager; -import org.apache.skywalking.apm.agent.core.context.ContextCarrier; -import org.apache.skywalking.apm.agent.core.context.ContextManager; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractTracingSpan; -import org.apache.skywalking.apm.agent.test.helper.SegmentHelper; -import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; -import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; -import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; -import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; -import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncRequestProducerWrapper; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnit; -import org.mockito.junit.MockitoRule; - -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.notNullValue; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.verify; - -@RunWith(TracingSegmentRunner.class) -public class AsyncRequestProducerWrapperTest { - - @SegmentStoragePoint - private SegmentStorage segmentStorage; - - @Rule - public AgentServiceRule agentServiceRule = new AgentServiceRule(); - - @Rule - public MockitoRule rule = MockitoJUnit.rule(); - - @Mock - private AsyncRequestProducer producer; - - @Mock - private RequestChannel requestChannel; - - @Before - public void setUp() { - ServiceManager.INSTANCE.boot(); - } - - @Test - public void createsExitSpanAndInjectsPropagationHeader() throws Exception { - AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); - - HttpHost target = new HttpHost("http", "127.0.0.1", 8080); - AsyncExitSpan exitSpan = new AsyncExitSpan(target); - AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper(producer, exitSpan); - - HttpRequest request = new BasicHttpRequest( - "GET", - "http://127.0.0.1:8080/hello" - ); - HttpContext context = new BasicHttpContext(); - AtomicReference sentRequest = new AtomicReference<>(); - - doAnswer(invocation -> { - RequestChannel wrappedChannel = invocation.getArgument(0); - - wrappedChannel.sendRequest(request, null, context); - return null; - }).when(producer).sendRequest(any(RequestChannel.class), any(HttpContext.class)); - - doAnswer(invocation -> { - sentRequest.set(invocation.getArgument(0)); - return null; - }).when(requestChannel).sendRequest(any(), any(), any()); - - wrapper.sendRequest(requestChannel, context); - - assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == callerSpan, is(true)); - assertThat(sentRequest.get(), notNullValue()); - assertThat(sentRequest.get().getFirstHeader("sw8") != null, is(true)); - - exitSpan.finish(); - - ContextManager.stopSpan(callerSpan); - - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - - verify(producer).sendRequest(any(RequestChannel.class), any(HttpContext.class)); - verify(requestChannel).sendRequest(any(), any(), any()); - } - - @Test - public void interleavedRequestsDoNotShareCallerSpanStack() throws Exception { - AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); - - AsyncExitSpan firstRequest = createAsyncExitSpan("/first"); - AsyncExitSpan secondRequest = createAsyncExitSpan("/second"); - - Thread reactorThread = new Thread(() -> { - // Both requests are already detached from the caller's span stack. - // Finish them in reverse order to simulate response interleaving. - secondRequest.onResponse(500); - firstRequest.onResponse(200); - - secondRequest.finish(); - firstRequest.finish(); - - // No request span should have been pushed onto this reactor thread. - assertThat(ContextManager.isActive(), is(false)); - }); - - reactorThread.start(); - reactorThread.join(); - - // Finishing the async spans must not affect the caller's active span. - assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == callerSpan, is(true)); - - ContextManager.stopSpan(callerSpan); - - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - - List spans = - SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); - - // One caller entry span + two independently finished request exit spans. - assertThat(spans.size(), is(3)); - } - - private AsyncExitSpan createAsyncExitSpan(String operationName) { - AsyncExitSpan exitSpan = new AsyncExitSpan( - new HttpHost("http", "127.0.0.1", 8080) - ); - - AbstractSpan requestSpan = ContextManager.createExitSpan( - operationName, - new ContextCarrier(), - "127.0.0.1:8080" - ); - - exitSpan.start(requestSpan); - requestSpan.prepareForAsync(); - ContextManager.stopSpan(requestSpan); - - return exitSpan; - } -} \ No newline at end of file diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestSpansTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestSpansTest.java new file mode 100644 index 0000000000..f1d6ba8aeb --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncRequestSpansTest.java @@ -0,0 +1,162 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; +import org.junit.Rule; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +/** + * These tests intentionally don't touch ContextManager/ByteBuddy at all — {@link AsyncRequestSpans} owns no + * thread-stack state, so its lifecycle guarantees (exactly-once finish, correct error propagation, claim + * exclusivity) can and should be verified directly, without a TracingSegmentRunner. Thread-stack correctness + * (nothing leaks onto the reactor thread, the caller's own span survives) belongs in the plugin scenario, not + * here — see test/plugin/scenarios/httpclient-5.x-scenario. + */ +public class AsyncRequestSpansTest { + + @Rule + public MockitoRule mockitoRule = MockitoJUnit.rule(); + + @Mock + private AbstractSpan span; + + private AsyncRequestSpans spans; + + @Before + public void setUp() { + spans = new AsyncRequestSpans(null); + spans.start(span); + } + + @Test + public void finishIsAppliedExactlyOnce() { + spans.finish(); + spans.finish(); + spans.fail(new RuntimeException("late failure after already finished")); + + verify(span, times(1)).asyncFinish(); + } + + @Test + public void streamEndThenReleaseResourcesDoesNotDoubleFinishOrMarkError() { + // consumeResponse (no error status) -> streamEnd -> releaseResources, the normal successful path. + spans.onResponse(200); + spans.finish(); + spans.abort(); // what releaseResources() calls; must be a no-op once already finished + + verify(span, times(1)).asyncFinish(); + verify(span, never()).errorOccurred(); + } + + @Test + public void releaseResourcesBeforeFailedStillEndsAsError() { + // HttpAsyncMainClientExec#failed calls releaseResources() BEFORE reporting the failure. If the span is + // still open when releaseResources() runs, the exchange never completed successfully, so it must be + // marked an error even though `fail()` with the real cause hasn't been called yet. + spans.onResponse(200); + spans.abort(); // releaseResources() fires first, response never fully arrived + + verify(span, times(1)).errorOccurred(); + verify(span, times(1)).asyncFinish(); + } + + @Test + public void noBodyFinishesAtConsumeResponse() { + spans.onResponse(204); // no entity -> caller calls finish() directly, streamEnd() never comes + spans.finish(); + + verify(span, times(1)).asyncFinish(); + } + + @Test + public void bodyFailureAfterSuccessfulHeadersIsAnError() { + spans.onResponse(200); + spans.fail(new RuntimeException("body read failed")); + + verify(span, times(1)).errorOccurred(); + verify(span, times(1)).log(org.mockito.ArgumentMatchers.any(Throwable.class)); + verify(span, times(1)).asyncFinish(); + } + + @Test + public void cancellationEndsAsError() { + spans.abort(); + + verify(span, times(1)).errorOccurred(); + verify(span, times(1)).asyncFinish(); + } + + @Test + public void errorStatusCodeMarksErrorWithoutFinishing() { + spans.onResponse(500); + + verify(span, times(1)).errorOccurred(); + verify(span, never()).asyncFinish(); + } + + @Test + public void informationResponseDoesNotFinishOrTagStatus() { + // 1xx must be a pure passthrough at the wrapper level; AsyncRequestSpans is simply never called for it. + // Nothing to assert here beyond "no interaction" — covered by not invoking onResponse/finish at all. + verify(span, never()).asyncFinish(); + } + + @Test + public void onlyTheCreatingThreadCanClaimCreation() throws InterruptedException { + AsyncRequestSpans fresh = new AsyncRequestSpans(null); + AtomicInteger claims = new AtomicInteger(); + CountDownLatch done = new CountDownLatch(1); + + // A different thread -- standing in for a custom AsyncRequestProducer that defers sending to another + // thread -- must NOT be able to claim creation. Only the constructing (doExecute) thread may. + new Thread(() -> { + if (fresh.claimCreation()) { + claims.incrementAndGet(); + } + done.countDown(); + }).start(); + assertTrue(done.await(5, TimeUnit.SECONDS)); + assertEquals(0, claims.get()); + + assertTrue(fresh.claimCreation()); + assertEquals(false, fresh.claimCreation()); // exactly once, even for the right thread + } + + @Test + public void callerReturnedRevokesClaimEvenIfUnused() { + AsyncRequestSpans fresh = new AsyncRequestSpans(null); + fresh.callerReturned(); + + assertEquals(false, fresh.claimCreation()); + } +} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncResponseConsumerWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncResponseConsumerWrapperTest.java deleted file mode 100644 index 6924c197ba..0000000000 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/AsyncResponseConsumerWrapperTest.java +++ /dev/null @@ -1,235 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.skywalking.apm.plugin.httpclient.v5; - -import java.util.List; -import org.apache.hc.core5.concurrent.FutureCallback; -import org.apache.hc.core5.http.EntityDetails; -import org.apache.hc.core5.http.HttpHost; -import org.apache.hc.core5.http.HttpResponse; -import org.apache.hc.core5.http.nio.AsyncResponseConsumer; -import org.apache.hc.core5.http.protocol.BasicHttpContext; -import org.apache.hc.core5.http.protocol.HttpContext; -import org.apache.skywalking.apm.agent.core.boot.ServiceManager; -import org.apache.skywalking.apm.agent.core.context.ContextCarrier; -import org.apache.skywalking.apm.agent.core.context.ContextManager; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractTracingSpan; -import org.apache.skywalking.apm.agent.test.helper.SegmentHelper; -import org.apache.skywalking.apm.agent.test.helper.SpanHelper; -import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; -import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; -import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; -import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; -import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.AsyncResponseConsumerWrapper; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnit; -import org.mockito.junit.MockitoRule; - -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.notNullValue; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -@RunWith(TracingSegmentRunner.class) -public class AsyncResponseConsumerWrapperTest { - - @SegmentStoragePoint - private SegmentStorage segmentStorage; - - @Rule - public AgentServiceRule agentServiceRule = new AgentServiceRule(); - - @Rule - public MockitoRule rule = MockitoJUnit.rule(); - - @Mock - private AsyncResponseConsumer consumer; - - @Mock - private HttpResponse response; - - @Mock - private EntityDetails entityDetails; - - @Before - public void setUp() { - ServiceManager.INSTANCE.boot(); - } - - @Test - public void responseWithoutEntityFinishesSpan() throws Exception { - when(response.getCode()).thenReturn(200); - - AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); - AsyncExitSpan exitSpan = createAsyncExitSpan("/no-body"); - - AsyncResponseConsumerWrapper wrapper = - new AsyncResponseConsumerWrapper<>(consumer, exitSpan); - - HttpContext context = new BasicHttpContext(); - - wrapper.consumeResponse(response, null, context, (FutureCallback) null); - - assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == callerSpan, is(true)); - - ContextManager.stopSpan(callerSpan); - - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - - List spans = - SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); - - assertThat(spans.size(), is(2)); - - AbstractTracingSpan responseSpan = findSpan(spans, "/no-body"); - assertThat(responseSpan, notNullValue()); - assertThat(SpanHelper.getErrorOccurred(responseSpan), is(false)); - - verify(consumer).consumeResponse(response, null, context, null); - } - - @Test - public void releaseBeforeFailureMarksSpanAsError() throws Exception { - when(response.getCode()).thenReturn(200); - - AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); - AsyncExitSpan exitSpan = createAsyncExitSpan("/body-failure"); - - AsyncResponseConsumerWrapper wrapper = - new AsyncResponseConsumerWrapper<>(consumer, exitSpan); - - HttpContext context = new BasicHttpContext(); - RuntimeException cause = new RuntimeException("body read failed"); - - // Response headers were received successfully, but the body will fail. - wrapper.consumeResponse( - response, - entityDetails, - context, - (FutureCallback) null - ); - - // releaseResources() can happen before failed(). The span must - // therefore be finished as an error rather than as a success. - wrapper.releaseResources(); - - // The later failure callback must not modify an already finished span. - wrapper.failed(cause); - - assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == callerSpan, is(true)); - - ContextManager.stopSpan(callerSpan); - - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - - List spans = - SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); - - assertThat(spans.size(), is(2)); - - AbstractTracingSpan responseSpan = findSpan(spans, "/body-failure"); - assertThat(responseSpan, notNullValue()); - assertThat(SpanHelper.getErrorOccurred(responseSpan), is(true)); - - verify(consumer).consumeResponse( - response, - entityDetails, - context, - null - ); - verify(consumer).releaseResources(); - verify(consumer).failed(cause); - } - - @Test - public void streamEndFinishesResponseSpan() throws Exception { - when(response.getCode()).thenReturn(200); - - AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); - AsyncExitSpan exitSpan = createAsyncExitSpan("/body"); - - AsyncResponseConsumerWrapper wrapper = - new AsyncResponseConsumerWrapper<>(consumer, exitSpan); - - HttpContext context = new BasicHttpContext(); - - wrapper.consumeResponse( - response, - entityDetails, - context, - (FutureCallback) null - ); - - wrapper.streamEnd(null); - - assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == callerSpan, is(true)); - - ContextManager.stopSpan(callerSpan); - - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - - List spans = - SegmentHelper.getSpans(segmentStorage.getTraceSegments().get(0)); - - assertThat(spans.size(), is(2)); - - AbstractTracingSpan responseSpan = findSpan(spans, "/body"); - assertThat(responseSpan, notNullValue()); - assertThat(SpanHelper.getErrorOccurred(responseSpan), is(false)); - - verify(consumer).streamEnd(null); - } - - private AsyncExitSpan createAsyncExitSpan(String operationName) { - AsyncExitSpan exitSpan = new AsyncExitSpan( - new HttpHost("http", "127.0.0.1", 8080) - ); - - AbstractSpan requestSpan = ContextManager.createExitSpan( - operationName, - new ContextCarrier(), - "127.0.0.1:8080" - ); - - exitSpan.start(requestSpan); - requestSpan.prepareForAsync(); - ContextManager.stopSpan(requestSpan); - - return exitSpan; - } - - private AbstractTracingSpan findSpan( - List spans, - String operationName) { - for (AbstractTracingSpan span : spans) { - if (operationName.equals(span.getOperationName())) { - return span; - } - } - return null; - } -} \ No newline at end of file diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java deleted file mode 100644 index 386a3cf1f7..0000000000 --- a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/FutureCallbackWrapperTest.java +++ /dev/null @@ -1,128 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package org.apache.skywalking.apm.plugin.httpclient.v5; - -import org.apache.hc.core5.concurrent.FutureCallback; -import org.apache.hc.core5.http.HttpHost; -import org.apache.skywalking.apm.agent.core.boot.ServiceManager; -import org.apache.skywalking.apm.agent.core.context.ContextCarrier; -import org.apache.skywalking.apm.agent.core.context.ContextManager; -import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; -import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; -import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; -import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; -import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; -import org.apache.skywalking.apm.plugin.httpclient.v5.wrapper.FutureCallbackWrapper; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnit; -import org.mockito.junit.MockitoRule; - -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.mockito.Mockito.verify; - -@RunWith(TracingSegmentRunner.class) -public class FutureCallbackWrapperTest { - - @SegmentStoragePoint - private SegmentStorage segmentStorage; - - @Rule - public AgentServiceRule agentServiceRule = new AgentServiceRule(); - - @Rule - public MockitoRule rule = MockitoJUnit.rule(); - - @Mock - private FutureCallback delegate; - - @Before - public void setUp() { - ServiceManager.INSTANCE.boot(); - } - - private AsyncExitSpan createStartedExitSpan() { - AsyncExitSpan exitSpan = new AsyncExitSpan( - new HttpHost("http", "127.0.0.1", 8080)); - - AbstractSpan requestSpan = ContextManager.createExitSpan( - "/hello", - new ContextCarrier(), - "127.0.0.1:8080"); - - exitSpan.start(requestSpan); - requestSpan.prepareForAsync(); - ContextManager.stopSpan(requestSpan); - - return exitSpan; - } - - @Test - public void completedKeepsCallerSpanActive() { - AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); - AsyncExitSpan exitSpan = createStartedExitSpan(); - - new FutureCallbackWrapper<>(delegate, exitSpan).completed("ok"); - - assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == callerSpan, is(true)); - verify(delegate).completed("ok"); - - ContextManager.stopSpan(callerSpan); - - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - } - - @Test - public void failedKeepsCallerSpanActive() { - AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); - AsyncExitSpan exitSpan = createStartedExitSpan(); - Exception cause = new RuntimeException("boom"); - - new FutureCallbackWrapper<>(delegate, exitSpan).failed(cause); - - assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == callerSpan, is(true)); - verify(delegate).failed(cause); - - ContextManager.stopSpan(callerSpan); - - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - } - - @Test - public void cancelledKeepsCallerSpanActive() { - AbstractSpan callerSpan = ContextManager.createEntrySpan("/business", null); - AsyncExitSpan exitSpan = createStartedExitSpan(); - - new FutureCallbackWrapper<>(delegate, exitSpan).cancelled(); - - assertThat(ContextManager.isActive(), is(true)); - assertThat(ContextManager.activeSpan() == callerSpan, is(true)); - verify(delegate).cancelled(); - - ContextManager.stopSpan(callerSpan); - - assertThat(segmentStorage.getTraceSegments().size(), is(1)); - } -} \ No newline at end of file diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapperTest.java new file mode 100644 index 0000000000..f119df5193 --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncRequestProducerWrapperTest.java @@ -0,0 +1,218 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; + +import java.net.URI; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.message.BasicHttpRequest; +import org.apache.hc.core5.http.nio.AsyncRequestProducer; +import org.apache.hc.core5.http.nio.RequestChannel; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.skywalking.apm.agent.core.context.ContextCarrier; +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.agent.test.tools.AgentServiceRule; +import org.apache.skywalking.apm.agent.test.tools.SegmentStorage; +import org.apache.skywalking.apm.agent.test.tools.SegmentStoragePoint; +import org.apache.skywalking.apm.agent.test.tools.TracingSegmentRunner; +import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncRequestSpans; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Exercises {@link AsyncRequestProducerWrapper} against the real {@link ContextManager}, via + * {@link TracingSegmentRunner}. {@code startExitSpan()} calls {@code ContextManager.createExitSpan}, + * {@code AbstractSpan.prepareForAsync()} and {@code ContextManager.stopSpan()} directly — mocking those out + * would only prove a mock was invoked, not that the caller's own active-span stack is left correctly balanced, + * which is the entire point of this class (and of issue #14097). + * + *

{@code AsyncResponseConsumerWrapperTest} and {@code FutureCallbackWrapperTest} don't need this harness: + * neither ever touches {@code ContextManager} — only the {@link AsyncRequestSpans} reference they're handed. + * + *

Known gap, deliberate: there is no assertion here that the exit span's peer is built from the + * explicit target host rather than the request URI's authority. That would require reading a completed span + * back out of the archived {@code TraceSegment} (e.g. a peer accessor), and I don't have confirmed access to + * that accessor in this codebase — guessing it once already produced a compile failure, so I'm not guessing + * again. The target/URI precedence logic in {@code startExitSpan()} is a short, branch-free block that's easy + * to verify by reading it directly; if you tell me the actual read-side accessor (on whatever class + * {@code TraceSegment}/the span type actually exposes it), I'll add that assertion in a follow-up. + */ +@RunWith(TracingSegmentRunner.class) +public class AsyncRequestProducerWrapperTest { + + @SegmentStoragePoint + private SegmentStorage segmentStorage; + + @Rule + public AgentServiceRule agentServiceRule = new AgentServiceRule(); + + private static final HttpHost TARGET = new HttpHost("http", "example.org", 8080); + + /** + * Stands in for every real {@code AsyncRequestProducer} (Internal/Minimal async clients, classic-facade + * adapter): calls the {@link RequestChannel} it's handed synchronously, on the calling thread, with a + * concrete request — exactly what {@link AsyncRequestProducerWrapper#sendRequest} depends on. + */ + private AsyncRequestProducer syncDelegate(HttpRequest request) throws Exception { + AsyncRequestProducer delegate = mock(AsyncRequestProducer.class); + doAnswer(invocation -> { + RequestChannel channel = invocation.getArgument(0); + HttpContext context = invocation.getArgument(1); + channel.sendRequest(request, null, context); + return null; + }).when(delegate).sendRequest(any(RequestChannel.class), any(HttpContext.class)); + return delegate; + } + + private HttpRequest requestTo(String uri) throws Exception { + return new BasicHttpRequest("GET", new URI(uri)); + } + + @Test + public void callerSpanRemainsActiveImmediatelyAfterHandoff() throws Exception { + AbstractSpan caller = ContextManager.createLocalSpan("caller"); + AsyncRequestSpans spans = new AsyncRequestSpans(TARGET); + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper( + syncDelegate(requestTo("http://example.org/hello")), spans); + + wrapper.sendRequest(mock(RequestChannel.class), mock(HttpContext.class)); + + assertSame(caller, ContextManager.activeSpan()); + + ContextManager.stopSpan(caller); + spans.finish(); + } + + @Test + public void exitSpanIsDetachedAndNotArchivedUntilAsyncFinish() throws Exception { + AbstractSpan outer = ContextManager.createLocalSpan("outer"); + AsyncRequestSpans spans = new AsyncRequestSpans(TARGET); + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper( + syncDelegate(requestTo("http://example.org/hello")), spans); + + wrapper.sendRequest(mock(RequestChannel.class), mock(HttpContext.class)); + ContextManager.stopSpan(outer); + + assertEquals(0, segmentStorage.getTraceSegments().size()); + + spans.finish(); + + assertEquals(1, segmentStorage.getTraceSegments().size()); + } + + @Test + public void headersAreInjectedIntoTheConcreteRequest() throws Exception { + AbstractSpan outer = ContextManager.createLocalSpan("outer"); + HttpRequest request = requestTo("http://example.org/hello"); + AsyncRequestSpans spans = new AsyncRequestSpans(TARGET); + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper(syncDelegate(request), spans); + + wrapper.sendRequest(mock(RequestChannel.class), mock(HttpContext.class)); + + assertTrue("sw8 propagation header must be injected", request.containsHeader("sw8")); + + ContextManager.stopSpan(outer); + spans.finish(); + } + + @Test + public void nestedInsideAnotherExitSpanDoesNotCreateASeparateAsyncSpan() throws Exception { + AbstractSpan outerExit = ContextManager.createExitSpan("outer-exit", new ContextCarrier(), "outer-peer:1"); + + AsyncRequestSpans spans = mock(AsyncRequestSpans.class); + when(spans.getTarget()).thenReturn(TARGET); + when(spans.claimCreation()).thenReturn(true); + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper( + syncDelegate(requestTo("http://example.org/hello")), spans); + + wrapper.sendRequest(mock(RequestChannel.class), mock(HttpContext.class)); + + verify(spans, never()).start(any(AbstractSpan.class)); + assertSame(outerExit, ContextManager.activeSpan()); + + ContextManager.stopSpan(outerExit); + } + + @Test + public void sendRequestFromAnotherThreadNeverCreatesASpan() throws Exception { + AbstractSpan outer = ContextManager.createLocalSpan("outer"); + AsyncRequestSpans spans = mock(AsyncRequestSpans.class); + when(spans.getTarget()).thenReturn(TARGET); + when(spans.claimCreation()).thenReturn(false); + + HttpRequest request = requestTo("http://example.org/hello"); + AsyncRequestProducer deferredDelegate = mock(AsyncRequestProducer.class); + CountDownLatch done = new CountDownLatch(1); + doAnswer(invocation -> { + RequestChannel channel = invocation.getArgument(0); + HttpContext context = invocation.getArgument(1); + Thread t = new Thread(() -> { + try { + channel.sendRequest(request, null, context); + } catch (Exception ignored) { + // test-only best effort + } finally { + done.countDown(); + } + }); + t.start(); + return null; + }).when(deferredDelegate).sendRequest(any(RequestChannel.class), any(HttpContext.class)); + + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper(deferredDelegate, spans); + wrapper.sendRequest(mock(RequestChannel.class), mock(HttpContext.class)); + + assertTrue(done.await(5, TimeUnit.SECONDS)); + verify(spans, never()).start(any(AbstractSpan.class)); + assertSame(outer, ContextManager.activeSpan()); + + ContextManager.stopSpan(outer); + } + + @Test + public void tracingFailureInsideStartExitSpanNeverBreaksTheRealRequest() throws Exception { + AbstractSpan outer = ContextManager.createLocalSpan("outer"); + HttpRequest badRequest = mock(HttpRequest.class); + when(badRequest.getUri()).thenThrow(new java.net.URISyntaxException("x", "bad")); + + RequestChannel realChannel = mock(RequestChannel.class); + AsyncRequestSpans spans = new AsyncRequestSpans(TARGET); + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper(syncDelegate(badRequest), spans); + + wrapper.sendRequest(realChannel, mock(HttpContext.class)); + verify(realChannel).sendRequest(eq(badRequest), any(), any(HttpContext.class)); + + ContextManager.stopSpan(outer); + } +} \ No newline at end of file diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapperTest.java new file mode 100644 index 0000000000..9b19816e2d --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/AsyncResponseConsumerWrapperTest.java @@ -0,0 +1,199 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; + +import java.util.Collections; +import org.apache.hc.core5.concurrent.FutureCallback; +import org.apache.hc.core5.http.EntityDetails; +import org.apache.hc.core5.http.HttpResponse; +import org.apache.hc.core5.http.nio.AsyncResponseConsumer; +import org.apache.hc.core5.http.nio.CapacityChannel; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.protocol.HttpCoreContext; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncRequestSpans; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * {@link AsyncResponseConsumerWrapper} never touches {@code ContextManager} — it only tags/finishes the + * {@link AsyncRequestSpans} reference it's given, so these tests wire a REAL {@code AsyncRequestSpans} to a + * MOCKED {@link AbstractSpan}, the same technique {@code AsyncRequestSpansTest} uses. That gives two things at + * once per test: proof the wrapped consumer's original behavior is still invoked unchanged (delegation), and + * proof of the actual span-lifecycle side effect through the real holder (not just "a mock was called"). + */ +@RunWith(MockitoJUnitRunner.class) +public class AsyncResponseConsumerWrapperTest { + + @Mock + private AbstractSpan span; + + @Mock + private AsyncResponseConsumer delegate; + + @Mock + private FutureCallback resultCallback; + + private AsyncRequestSpans spans; + private AsyncResponseConsumerWrapper wrapper; + private HttpContext context; + + @Before + public void setUp() { + spans = new AsyncRequestSpans(null); + spans.start(span); + wrapper = new AsyncResponseConsumerWrapper<>(delegate, spans); + context = new HttpCoreContext(); + } + + private HttpResponse response(int status) { + HttpResponse r = mock(HttpResponse.class); + when(r.getCode()).thenReturn(status); + return r; + } + + @Test + public void consumeResponseWithEntityTagsStatusButDoesNotFinishYet() throws Exception { + EntityDetails entity = mock(EntityDetails.class); + HttpResponse response = response(200); + + wrapper.consumeResponse(response, entity, context, resultCallback); + + verify(span, never()).asyncFinish(); + verify(delegate).consumeResponse(response, entity, context, resultCallback); + } + + @Test + public void consumeResponseWithoutEntityFinishesImmediately() throws Exception { + // e.g. a 204 with no body: streamEnd() will never be called for this exchange, so consumeResponse() + // itself must finish the span. + HttpResponse response = response(204); + + wrapper.consumeResponse(response, null, context, resultCallback); + + verify(span, times(1)).asyncFinish(); + verify(delegate).consumeResponse(response, null, context, resultCallback); + } + + @Test + public void errorStatusMarksErrorWithoutFinishing() throws Exception { + EntityDetails entity = mock(EntityDetails.class); + HttpResponse response = response(500); + + wrapper.consumeResponse(response, entity, context, resultCallback); + + verify(span, times(1)).errorOccurred(); + verify(span, never()).asyncFinish(); + } + + @Test + public void informationResponseNeverTouchesTheSpan() throws Exception { + HttpResponse response = response(100); + + wrapper.informationResponse(response, context); + + verify(span, never()).asyncFinish(); + verify(span, never()).errorOccurred(); + verify(delegate).informationResponse(response, context); + } + + @Test + public void streamEndFinishesTheSpanExactlyOnce() throws Exception { + wrapper.streamEnd(Collections.emptyList()); + + verify(span, times(1)).asyncFinish(); + verify(delegate).streamEnd(Collections.emptyList()); + } + + @Test + public void consumeResponseThenStreamEndFinishesExactlyOnce() throws Exception { + EntityDetails entity = mock(EntityDetails.class); + wrapper.consumeResponse(response(200), entity, context, resultCallback); + wrapper.streamEnd(Collections.emptyList()); + + verify(span, times(1)).asyncFinish(); + } + + @Test + public void failedMarksErrorAndFinishesExactlyOnce() { + RuntimeException cause = new RuntimeException("boom"); + + wrapper.failed(cause); + + verify(span, times(1)).errorOccurred(); + verify(span, times(1)).log(cause); + verify(span, times(1)).asyncFinish(); + verify(delegate).failed(cause); + } + + @Test + public void releaseResourcesAfterNormalCompletionIsANoOp() throws Exception { + wrapper.streamEnd(Collections.emptyList()); + wrapper.releaseResources(); + + // finish() already ran at streamEnd(); releaseResources()'s abort() must not run it a second time nor + // retroactively mark a successful exchange as an error. + verify(span, times(1)).asyncFinish(); + verify(span, never()).errorOccurred(); + verify(delegate).releaseResources(); + } + + @Test + public void releaseResourcesBeforeFailedStillEndsAsErrorExactlyOnce() { + // HttpAsyncMainClientExec#failed calls releaseResources() BEFORE reporting the real failure. + wrapper.releaseResources(); + wrapper.failed(new RuntimeException("real cause, arrives after release")); + + verify(span, times(1)).errorOccurred(); + verify(span, times(1)).asyncFinish(); + } + + @Test + public void releaseResourcesWithoutAnyResponseEndsAsError() { + // A suppressed redirect with a non-repeatable entity: only releaseResources() ever runs, failed()/ + // completed() never do. The span must still end, and must end as an error (the exchange never actually + // completed), not silently disappear. + wrapper.releaseResources(); + + verify(span, times(1)).errorOccurred(); + verify(span, times(1)).asyncFinish(); + } + + @Test + public void updateCapacityAndConsumeAreTransparentPassthroughs() throws Exception { + CapacityChannel capacityChannel = mock(CapacityChannel.class); + wrapper.updateCapacity(capacityChannel); + verify(delegate).updateCapacity(capacityChannel); + verify(span, never()).asyncFinish(); + + java.nio.ByteBuffer buf = java.nio.ByteBuffer.allocate(0); + wrapper.consume(buf); + verify(delegate).consume(buf); + verify(span, never()).asyncFinish(); + } +} diff --git a/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapperTest.java b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapperTest.java new file mode 100644 index 0000000000..70c0998072 --- /dev/null +++ b/apm-sniffer/apm-sdk-plugin/httpclient-5.x-plugin/src/test/java/org/apache/skywalking/apm/plugin/httpclient/v5/wrapper/FutureCallbackWrapperTest.java @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; + +import org.apache.hc.core5.concurrent.FutureCallback; +import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan; +import org.apache.skywalking.apm.plugin.httpclient.v5.AsyncRequestSpans; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +/** + * This is the class the original bug (#14097) lived in. The old behavior called the parameterless + * {@code ContextManager.stopSpan()} here, which is exactly what these tests exist to guard against ever + * regressing to: none of them touch {@code ContextManager} at all, they only assert the {@link AsyncRequestSpans} + * reference is finished by reference — which is what actually makes it safe to run on the caller/business + * thread, as {@code HttpAsyncClients.classic(...)} does. + */ +@RunWith(MockitoJUnitRunner.class) +public class FutureCallbackWrapperTest { + + @Mock + private AbstractSpan span; + + @Mock + private FutureCallback delegate; + + private AsyncRequestSpans spans; + + @Before + public void setUp() { + spans = new AsyncRequestSpans(null); + spans.start(span); + } + + @Test + public void completedFinishesSpanExactlyOnceAndDelegates() { + FutureCallbackWrapper wrapper = new FutureCallbackWrapper<>(delegate, spans); + + wrapper.completed("result"); + + verify(span, times(1)).asyncFinish(); + verify(span, never()).errorOccurred(); + verify(delegate).completed("result"); + } + + @Test + public void failedMarksErrorAndFinishesExactlyOnceAndDelegates() { + FutureCallbackWrapper wrapper = new FutureCallbackWrapper<>(delegate, spans); + RuntimeException cause = new RuntimeException("boom"); + + wrapper.failed(cause); + + verify(span, times(1)).errorOccurred(); + verify(span, times(1)).log(cause); + verify(span, times(1)).asyncFinish(); + verify(delegate).failed(cause); + } + + @Test + public void cancelledMarksErrorAndFinishesExactlyOnceAndDelegates() { + FutureCallbackWrapper wrapper = new FutureCallbackWrapper<>(delegate, spans); + + wrapper.cancelled(); + + verify(span, times(1)).errorOccurred(); + verify(span, times(1)).asyncFinish(); + verify(delegate).cancelled(); + } + + @Test + public void toleratesANullDelegateCallback() { + // doExecute is always wrapped even when the caller passed no callback of their own — this is the only + // lifecycle hook that observes cancellation, so it must not NPE on a null delegate. + FutureCallbackWrapper wrapper = new FutureCallbackWrapper<>(null, spans); + + wrapper.completed("result"); // must not throw + + verify(span, times(1)).asyncFinish(); + } + + @Test + public void completedAfterConsumerAlreadyFinishedDoesNotDoubleFinish() { + // Simulates AsyncResponseConsumerWrapper already having finished the span (streamEnd/consumeResponse) + // before the future callback also fires — FutureCallbackWrapper's own paths are largely redundant + // safety nets, and AsyncRequestSpans' idempotency is what makes that redundancy safe. + spans.finish(); // as if AsyncResponseConsumerWrapper already ran + FutureCallbackWrapper wrapper = new FutureCallbackWrapper<>(delegate, spans); + + wrapper.completed("result"); + + verify(span, times(1)).asyncFinish(); + verify(delegate).completed("result"); + } + + @Test + public void cancelledAfterAlreadyFailedDoesNotOverwriteOrDoubleFinish() { + FutureCallbackWrapper wrapper = new FutureCallbackWrapper<>(delegate, spans); + wrapper.failed(new RuntimeException("first")); + + wrapper.cancelled(); + + verify(span, times(1)).errorOccurred(); + verify(span, times(1)).asyncFinish(); + verify(delegate).cancelled(); + } +} From 72925fbf4c45c62a524db46199ea17e6c4cbb3e1 Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Fri, 25 Sep 2026 14:25:47 +0530 Subject: [PATCH 8/8] Fix HttpClient 5.x async span lifecycle --- .../config/expectedData.yaml | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/test/plugin/scenarios/httpclient-5.x-scenario/config/expectedData.yaml b/test/plugin/scenarios/httpclient-5.x-scenario/config/expectedData.yaml index d1d6e1d04f..f9801999e8 100644 --- a/test/plugin/scenarios/httpclient-5.x-scenario/config/expectedData.yaml +++ b/test/plugin/scenarios/httpclient-5.x-scenario/config/expectedData.yaml @@ -19,7 +19,7 @@ segmentItems: segments: - segmentId: not null spans: - - operationName: GET:/httpclient-5.x/back + - operationName: HEAD:/httpclient-5.x/case/healthcheck parentSpanId: -1 spanId: 0 spanLayer: Http @@ -30,44 +30,29 @@ segmentItems: spanType: Entry peer: '' tags: - - {key: url, value: 'http://127.0.0.1:8080/httpclient-5.x/back'} - - {key: http.method, value: GET} + - {key: url, value: 'http://127.0.0.1:8080/httpclient-5.x/case/healthcheck'} + - {key: http.method, value: HEAD} - {key: http.status_code, value: '200'} - refs: - - {parentEndpoint: httpasyncclient/local, networkAddress: '127.0.0.1:8080', - refType: CrossProcess, parentSpanId: 1, parentTraceSegmentId: not null, parentServiceInstance: not - null, parentService: httpclient-5.x-scenario, traceId: not null} skipAnalysis: 'false' - segmentId: not null spans: - - operationName: /httpclient-5.x/back - parentSpanId: 0 - spanId: 1 + - operationName: GET:/httpclient-5.x/back + parentSpanId: -1 + spanId: 0 spanLayer: Http startTime: nq 0 endTime: nq 0 - componentId: 26 + componentId: 1 isError: false - spanType: Exit - peer: 127.0.0.1:8080 + spanType: Entry + peer: '' tags: - {key: url, value: 'http://127.0.0.1:8080/httpclient-5.x/back'} - {key: http.method, value: GET} - {key: http.status_code, value: '200'} - skipAnalysis: 'false' - - operationName: httpasyncclient/local - parentSpanId: -1 - spanId: 0 - spanLayer: Http - startTime: nq 0 - endTime: nq 0 - componentId: 26 - isError: false - spanType: Local - peer: '' refs: - - {parentEndpoint: GET:/httpclient-5.x/case/asyncGet, networkAddress: '', - refType: CrossThread, parentSpanId: 0, parentTraceSegmentId: not null, parentServiceInstance: not + - {parentEndpoint: /httpclient-5.x/back, networkAddress: '127.0.0.1:8080', + refType: CrossProcess, parentSpanId: 1, parentTraceSegmentId: not null, parentServiceInstance: not null, parentService: httpclient-5.x-scenario, traceId: not null} skipAnalysis: 'false' - segmentId: not null @@ -91,6 +76,21 @@ segmentItems: refType: CrossProcess, parentSpanId: 1, parentTraceSegmentId: not null, parentServiceInstance: not null, parentService: httpclient-5.x-scenario, traceId: not null} skipAnalysis: 'false' + - operationName: /httpclient-5.x/back + parentSpanId: 0 + spanId: 1 + spanLayer: Http + startTime: nq 0 + endTime: nq 0 + componentId: 26 + isError: false + spanType: Exit + peer: 127.0.0.1:8080 + tags: + - {key: url, value: 'http://127.0.0.1:8080/httpclient-5.x/back'} + - {key: http.method, value: GET} + - {key: http.status_code, value: '200'} + skipAnalysis: 'false' - segmentId: not null spans: - operationName: /httpclient-5.x/case/asyncGet