From 7ec97a0bcbf13aa698d6330dd86860dbe759276b Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Mon, 21 Sep 2026 16:35:38 +0530 Subject: [PATCH 1/9] 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/9] 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/9] 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/9] 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/9] 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/9] 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/9] 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/9] 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 From 33a457f98bfac0661d6b4358204d9f75168c442e Mon Sep 17 00:00:00 2001 From: Ayush0612005 Date: Sat, 26 Sep 2026 00:09:06 +0530 Subject: [PATCH 9/9] Fix httpclient-5.x scenario expectations and nested exit span handling The scenario failed because expectedData.yaml did not match the trace the producer-wrapper design produces. The /httpclient-5.x/back entry segment is referenced by the async exit span inside the asyncGet entry segment, so its parentEndpoint is GET:/httpclient-5.x/case/asyncGet, and the exit span is archived before the entry span in that segment. The hand-added healthcheck segment is dropped, since it is not part of what this scenario checks. Create the exit span without a carrier and inject afterwards. createExitSpan(op, carrier, peer) injects before returning, and injection throws for a reused outer exit span without a peer, which left the reused span's extra depth on the caller's stack so the caller's segment was never reported. Add a regression test for it and an assertion that the exit span lands in the caller's segment with the explicit target as its peer. Also remove leftover notes and tidy imports and comments in the wrappers. Verified locally: plugin unit tests 53/53, checkstyle clean, and httpclient-5.x-scenario passes for httpclient 5.0 and 5.1. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01JbpEb2HPqrhVcyhjjrJHPW --- .../httpclient/v5/AsyncRequestSpans.java | 3 +- .../HttpAsyncClientDoExecuteInterceptor.java | 4 +- .../wrapper/AsyncRequestProducerWrapper.java | 32 ++++---- .../wrapper/AsyncResponseConsumerWrapper.java | 33 ++++----- .../AsyncRequestProducerWrapperTest.java | 74 ++++++++++++++----- .../config/expectedData.yaml | 45 ++++------- 6 files changed, 101 insertions(+), 90 deletions(-) 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 index dfb24da289..35be85c242 100644 --- 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 @@ -67,8 +67,7 @@ public HttpHost getTarget() { * constructed this holder (the {@code doExecute} caller thread). */ public boolean claimCreation() { - Thread current = Thread.currentThread(); - return creator.compareAndSet(current, null); + return creator.compareAndSet(Thread.currentThread(), null); } /** 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 8e8fb92fc8..8736dbcd1c 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 @@ -36,8 +36,8 @@ * 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: + *

It does not store anything in the {@code HttpContext} and never touches the span stack of a thread other than + * the caller's. 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 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 f355180657..3d8c69fbff 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 @@ -98,13 +98,13 @@ private void startExitSpan(HttpRequest request) throws URISyntaxException { String path = uri.getPath() == null || uri.getPath().isEmpty() ? "/" : uri.getPath(); String url = scheme + "://" + peer + path + (uri.getRawQuery() == null ? "" : "?" + uri.getRawQuery()); - // 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); + // Inside another plugin's exit span, createExitSpan reuses that span (depth + 1) instead of creating one. + // That span belongs to the other plugin, so it must not be turned into an async span here: only propagate. + boolean nested = ContextManager.activeSpan().isExit(); + // Create the span without a carrier and inject afterwards. createExitSpan(op, carrier, peer) injects before + // returning, and injection throws for a reused outer exit span without a peer, which would leave the extra + // depth on the caller's stack with nothing to stop it. + AbstractSpan span = ContextManager.createExitSpan(path, peer); try { if (!nested) { span.setComponent(ComponentsDefine.HTTP_ASYNC_CLIENT); @@ -112,20 +112,22 @@ 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(); request.setHeader(next.getHeadKey(), next.getHeadValue()); } } finally { + // Detach before the request is forwarded: the client may report a failure on this thread before + // doExecute returns. prepareForAsync() requires the span to still be the active one. 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); + } + ContextManager.stopSpan(span); + if (!nested) { spans.start(span); - } else { - ContextManager.stopSpan(span); } } } @@ -154,10 +156,4 @@ public int available() { public void releaseResources() { producer.releaseResources(); } - - // 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 b8218f3ead..01854f9582 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 @@ -19,7 +19,11 @@ 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.concurrent.FutureCallback; 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; @@ -28,9 +32,9 @@ 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. + * Finishes the request's exit span through {@link AsyncRequestSpans}, never through the span stack of the current + * thread: the response is consumed on the I/O reactor thread, which serves many requests, and for + * {@code HttpAsyncClients.classic(...)} the body may be read to the end on the caller's thread. */ public class AsyncResponseConsumerWrapper implements AsyncResponseConsumer { @@ -44,7 +48,7 @@ public AsyncResponseConsumerWrapper(AsyncResponseConsumer consumer, AsyncRequ @Override public void consumeResponse(HttpResponse response, EntityDetails entityDetails, HttpContext context, - org.apache.hc.core5.concurrent.FutureCallback resultCallback) throws HttpException, IOException { + FutureCallback resultCallback) throws HttpException, IOException { spans.onResponse(response.getCode()); if (entityDetails == null) { // No body means streamEnd() will never be called for this exchange. @@ -60,8 +64,7 @@ public void informationResponse(HttpResponse response, HttpContext context) thro } @Override - public void streamEnd(java.util.List trailers) - throws HttpException, IOException { + public void streamEnd(List trailers) throws HttpException, IOException { spans.finish(); consumer.streamEnd(trailers); } @@ -78,25 +81,17 @@ public void updateCapacity(CapacityChannel capacityChannel) throws IOException { } @Override - public void consume(java.nio.ByteBuffer src) throws IOException { + public void consume(ByteBuffer src) throws IOException { consumer.consume(src); } @Override public void releaseResources() { - // 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. + // Fallback finisher, not a success signal. In the normal case the span was already finished at + // streamEnd/consumeResponse, so this does nothing. HttpAsyncMainClientExec#failed releases the consumer + // before it reports the failure, and a suppressed redirect with a non-repeatable entity (5.5.x) only + // releases it, so a span still open here ended without a complete response and is marked as an error. spans.abort(); consumer.releaseResources(); } - - // 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/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 index f119df5193..da1a8b148f 100644 --- 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 @@ -19,6 +19,7 @@ package org.apache.skywalking.apm.plugin.httpclient.v5.wrapper; import java.net.URI; +import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import org.apache.hc.core5.http.HttpHost; @@ -30,6 +31,10 @@ 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; @@ -40,6 +45,7 @@ import org.junit.runner.RunWith; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; @@ -51,22 +57,9 @@ 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. + * Runs {@link AsyncRequestProducerWrapper} against the real {@link ContextManager}: the exit span must be created in + * the caller's segment, detached from the caller's stack before the request is forwarded, and finished later by + * reference only. */ @RunWith(TracingSegmentRunner.class) public class AsyncRequestProducerWrapperTest { @@ -84,7 +77,7 @@ public class AsyncRequestProducerWrapperTest { * 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 { + private AsyncRequestProducer syncDelegate(HttpRequest request) throws Exception { AsyncRequestProducer delegate = mock(AsyncRequestProducer.class); doAnswer(invocation -> { RequestChannel channel = invocation.getArgument(0); @@ -215,4 +208,49 @@ public void tracingFailureInsideStartExitSpanNeverBreaksTheRealRequest() throws ContextManager.stopSpan(outer); } -} \ No newline at end of file + + @Test + public void exitSpanLandsInCallerSegmentWithExplicitTargetAsPeer() throws Exception { + AbstractSpan caller = ContextManager.createLocalSpan("caller"); + // The request's own authority differs from the explicit target, which must win, as in the client itself. + AsyncRequestSpans spans = new AsyncRequestSpans(TARGET); + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper( + syncDelegate(requestTo("http://other.invalid:9999/hello?a=b")), spans); + + wrapper.sendRequest(mock(RequestChannel.class), mock(HttpContext.class)); + spans.onResponse(200); + spans.finish(); + ContextManager.stopSpan(caller); + + assertEquals(1, segmentStorage.getTraceSegments().size()); + TraceSegment segment = segmentStorage.getTraceSegments().get(0); + List spanList = SegmentHelper.getSpans(segment); + assertEquals(2, spanList.size()); + AbstractTracingSpan exit = spanList.get(0); + assertTrue(exit.isExit()); + assertEquals("/hello", exit.getOperationName()); + assertEquals("example.org:8080", SpanHelper.getPeer(exit)); + assertEquals(caller.getSpanId(), SpanHelper.getParentSpanId(exit)); + assertFalse(SpanHelper.getErrorOccurred(exit)); + } + + /** + * An outer exit span without a peer: injection fails, but the reused span's extra depth must still be released, + * otherwise the outer span could never be stopped and the caller's segment would never be reported. + */ + @Test + public void nestedInsideExitSpanWithoutPeerLeavesTheStackBalanced() throws Exception { + AbstractSpan outerExit = ContextManager.createExitSpan("outer-exit", ""); + HttpRequest request = requestTo("http://example.org/hello"); + RequestChannel realChannel = mock(RequestChannel.class); + AsyncRequestSpans spans = new AsyncRequestSpans(TARGET); + AsyncRequestProducerWrapper wrapper = new AsyncRequestProducerWrapper(syncDelegate(request), spans); + + wrapper.sendRequest(realChannel, mock(HttpContext.class)); + + verify(realChannel).sendRequest(eq(request), any(), any(HttpContext.class)); + assertSame(outerExit, ContextManager.activeSpan()); + ContextManager.stopSpan(outerExit); + assertEquals(1, segmentStorage.getTraceSegments().size()); + } +} 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 f9801999e8..bb3c8d7a99 100644 --- a/test/plugin/scenarios/httpclient-5.x-scenario/config/expectedData.yaml +++ b/test/plugin/scenarios/httpclient-5.x-scenario/config/expectedData.yaml @@ -15,11 +15,11 @@ # limitations under the License. segmentItems: - serviceName: httpclient-5.x-scenario - segmentSize: ge 4 + segmentSize: ge 3 segments: - segmentId: not null spans: - - operationName: HEAD:/httpclient-5.x/case/healthcheck + - operationName: GET:/httpclient-5.x/back parentSpanId: -1 spanId: 0 spanLayer: Http @@ -30,33 +30,31 @@ segmentItems: spanType: Entry peer: '' tags: - - {key: url, value: 'http://127.0.0.1:8080/httpclient-5.x/case/healthcheck'} - - {key: http.method, value: HEAD} + - {key: url, value: 'http://127.0.0.1:8080/httpclient-5.x/back'} + - {key: http.method, value: GET} - {key: http.status_code, value: '200'} + refs: + - {parentEndpoint: 'GET:/httpclient-5.x/case/asyncGet', 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: GET:/httpclient-5.x/back - parentSpanId: -1 - spanId: 0 + - operationName: /httpclient-5.x/back + parentSpanId: 0 + spanId: 1 spanLayer: Http startTime: nq 0 endTime: nq 0 - componentId: 1 + componentId: 26 isError: false - spanType: Entry - peer: '' + 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'} - refs: - - {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 - spans: - operationName: GET:/httpclient-5.x/case/asyncGet parentSpanId: -1 spanId: 0 @@ -76,21 +74,6 @@ 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