Skip to content

Commit 663d96c

Browse files
amarzialiamarziali
authored
Fix continuation leakage on netty http2 client pipeline (#11595)
Fix continuation leakage on netty http2 client pipeline handle http2 connect failures rename Co-authored-by: andrea.marziali <andrea.marziali@datadoghq.com>
1 parent 65845b3 commit 663d96c

6 files changed

Lines changed: 141 additions & 11 deletions

File tree

‎dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/Http2ConnectContinuationListener.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package datadog.trace.instrumentation.netty41;
2+
3+
import static datadog.trace.instrumentation.netty41.AttributeKeys.CONNECT_PARENT_CONTINUATION_ATTRIBUTE_KEY;
4+
5+
import datadog.trace.bootstrap.instrumentation.api.AgentScope;
6+
import io.netty.channel.Channel;
7+
import io.netty.channel.ChannelFuture;
8+
import io.netty.channel.ChannelFutureListener;
9+
10+
public final class Http2ConnectContinuationListener implements ChannelFutureListener {
11+
public static final ChannelFutureListener INSTANCE = new Http2ConnectContinuationListener();
12+
13+
private Http2ConnectContinuationListener() {}
14+
15+
@Override
16+
public void operationComplete(final ChannelFuture future) {
17+
if (future.isSuccess() || future.isCancelled()) {
18+
cancel(future.channel());
19+
}
20+
// Failed connects are left for ChannelFutureListenerInstrumentation, which creates the
21+
// netty.connect error span under this continuation.
22+
}
23+
24+
public static void cancel(final Channel channel) {
25+
if (channel == null) {
26+
return;
27+
}
28+
final AgentScope.Continuation continuation =
29+
channel.attr(CONNECT_PARENT_CONTINUATION_ATTRIBUTE_KEY).getAndRemove();
30+
if (continuation != null) {
31+
continuation.cancel();
32+
}
33+
}
34+
}

‎dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyChannelPipelineInstrumentation.java‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.captureActiveSpan;
88
import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopContinuation;
99
import static datadog.trace.instrumentation.netty41.AttributeKeys.CONNECT_PARENT_CONTINUATION_ATTRIBUTE_KEY;
10+
import static datadog.trace.instrumentation.netty41.AttributeKeys.HTTP2_CONNECTION_CODEC_ATTRIBUTE_KEY;
1011
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
1112
import static net.bytebuddy.matcher.ElementMatchers.returns;
1213
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
@@ -29,6 +30,7 @@
2930
import datadog.trace.instrumentation.netty41.server.websocket.WebSocketServerInboundTracingHandler;
3031
import datadog.trace.instrumentation.netty41.server.websocket.WebSocketServerOutboundTracingHandler;
3132
import datadog.trace.instrumentation.netty41.server.websocket.WebSocketServerTracingHandler;
33+
import io.netty.channel.ChannelFuture;
3234
import io.netty.channel.ChannelHandler;
3335
import io.netty.channel.ChannelPipeline;
3436
import io.netty.handler.codec.http.HttpClientCodec;
@@ -90,6 +92,7 @@ public String[] helperClassNames() {
9092
packageName + ".server.websocket.WebSocketServerTracingHandler",
9193
packageName + ".server.websocket.WebSocketServerOutboundTracingHandler",
9294
packageName + ".server.websocket.WebSocketServerInboundTracingHandler",
95+
packageName + ".Http2ConnectContinuationListener",
9396
packageName + ".NettyHttp2Helper",
9497
packageName + ".NettyPipelineHelper",
9598
};
@@ -175,6 +178,9 @@ public static void addHandler(
175178
handler2 instanceof ChannelHandler ? (ChannelHandler) handler2 : handler3;
176179

177180
try {
181+
if (NettyHttp2Helper.isHttp2ConnectionCodec(handler)) {
182+
pipeline.channel().attr(HTTP2_CONNECTION_CODEC_ATTRIBUTE_KEY).set(Boolean.TRUE);
183+
}
178184
// Server pipeline handlers
179185
if (handler instanceof HttpServerCodec) {
180186
NettyPipelineHelper.addHandlerAfter(
@@ -249,14 +255,33 @@ else if (handler instanceof HttpClientCodec) {
249255

250256
public static class ConnectAdvice {
251257
@Advice.OnMethodEnter(suppress = Throwable.class)
252-
public static void addParentSpan(@Advice.This final ChannelPipeline pipeline) {
258+
public static boolean addParentSpan(@Advice.This final ChannelPipeline pipeline) {
253259
AgentScope.Continuation continuation = captureActiveSpan();
254260
if (continuation != noopContinuation()) {
255261
final Attribute<AgentScope.Continuation> attribute =
256262
pipeline.channel().attr(CONNECT_PARENT_CONTINUATION_ATTRIBUTE_KEY);
257263
if (!attribute.compareAndSet(null, continuation)) {
258264
continuation.cancel();
265+
return false;
259266
}
267+
return Boolean.TRUE.equals(
268+
pipeline.channel().attr(HTTP2_CONNECTION_CODEC_ATTRIBUTE_KEY).get());
269+
}
270+
return false;
271+
}
272+
273+
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
274+
public static void cleanupHttp2ConnectParentContinuation(
275+
@Advice.Enter final boolean cleanupHttp2Continuation,
276+
@Advice.This final ChannelPipeline pipeline,
277+
@Advice.Return final ChannelFuture future) {
278+
if (!cleanupHttp2Continuation) {
279+
return;
280+
}
281+
if (future == null) {
282+
Http2ConnectContinuationListener.cancel(pipeline.channel());
283+
} else {
284+
future.addListener(Http2ConnectContinuationListener.INSTANCE);
260285
}
261286
}
262287
}

‎dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyHttp2Helper.java‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,14 @@
99
import org.slf4j.LoggerFactory;
1010

1111
public class NettyHttp2Helper {
12-
private static final Class HTTP2_CODEC_CLS;
12+
private static final Class HTTP2_STREAM_FRAME_CODEC_CLS;
13+
private static final Class HTTP2_CONNECTION_CODEC_CLS;
1314
private static final MethodHandle IS_SERVER_FIELD;
1415
private static final Logger LOGGER = LoggerFactory.getLogger(NettyHttp2Helper.class);
1516

1617
static {
1718
Class codecClass;
19+
Class frameCodecClass;
1820
MethodHandle isServerField;
1921
try {
2022
codecClass =
@@ -38,12 +40,31 @@ public class NettyHttp2Helper {
3840
isServerField = null;
3941
LOGGER.debug("Unable to setup netty http2 instrumentation", t);
4042
}
41-
HTTP2_CODEC_CLS = codecClass;
43+
try {
44+
frameCodecClass =
45+
Class.forName(
46+
"io.netty.handler.codec.http2.Http2FrameCodec",
47+
false,
48+
NettyHttp2Helper.class.getClassLoader());
49+
} catch (final ClassNotFoundException cnfe) {
50+
// can be expected
51+
frameCodecClass = null;
52+
} catch (Throwable t) {
53+
// unexpected
54+
frameCodecClass = null;
55+
LOGGER.debug("Unable to setup netty http2 connection detection", t);
56+
}
57+
HTTP2_STREAM_FRAME_CODEC_CLS = codecClass;
58+
HTTP2_CONNECTION_CODEC_CLS = frameCodecClass;
4259
IS_SERVER_FIELD = isServerField;
4360
}
4461

4562
public static boolean isHttp2FrameCodec(final ChannelHandler handler) {
46-
return HTTP2_CODEC_CLS != null && HTTP2_CODEC_CLS.isInstance(handler);
63+
return HTTP2_STREAM_FRAME_CODEC_CLS != null && HTTP2_STREAM_FRAME_CODEC_CLS.isInstance(handler);
64+
}
65+
66+
public static boolean isHttp2ConnectionCodec(final ChannelHandler handler) {
67+
return HTTP2_CONNECTION_CODEC_CLS != null && HTTP2_CONNECTION_CODEC_CLS.isInstance(handler);
4768
}
4869

4970
public static boolean isServer(final ChannelHandler handler) {

‎dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/client/HttpClientRequestTracingHandler.java‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import datadog.trace.api.Config;
1919
import datadog.trace.bootstrap.instrumentation.api.AgentScope;
2020
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
21+
import io.netty.channel.Channel;
2122
import io.netty.channel.ChannelHandler;
2223
import io.netty.channel.ChannelHandlerContext;
2324
import io.netty.channel.ChannelOutboundHandlerAdapter;
@@ -54,8 +55,7 @@ public void write(final ChannelHandlerContext ctx, final Object msg, final Chann
5455
}
5556

5657
AgentScope parentScope = null;
57-
final AgentScope.Continuation continuation =
58-
ctx.channel().attr(CONNECT_PARENT_CONTINUATION_ATTRIBUTE_KEY).getAndRemove();
58+
final AgentScope.Continuation continuation = takeConnectParentContinuation(ctx);
5959
if (continuation != null) {
6060
parentScope = continuation.activate();
6161
}
@@ -111,4 +111,16 @@ public void write(final ChannelHandlerContext ctx, final Object msg, final Chann
111111
}
112112
}
113113
}
114+
115+
private static AgentScope.Continuation takeConnectParentContinuation(
116+
final ChannelHandlerContext ctx) {
117+
final Channel channel = ctx.channel();
118+
AgentScope.Continuation continuation =
119+
channel.attr(CONNECT_PARENT_CONTINUATION_ATTRIBUTE_KEY).getAndRemove();
120+
if (continuation == null && channel.parent() != null) {
121+
continuation =
122+
channel.parent().attr(CONNECT_PARENT_CONTINUATION_ATTRIBUTE_KEY).getAndRemove();
123+
}
124+
return continuation;
125+
}
114126
}

‎dd-java-agent/instrumentation/netty/netty-common/src/main/java/datadog/trace/instrumentation/netty41/AttributeKeys.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ public final class AttributeKeys {
2727
CONNECT_PARENT_CONTINUATION_ATTRIBUTE_KEY =
2828
attributeKey("datadog.connect.parent.continuation");
2929

30+
public static final AttributeKey<Boolean> HTTP2_CONNECTION_CODEC_ATTRIBUTE_KEY =
31+
attributeKey("datadog.http2.connection.codec");
32+
3033
public static final AttributeKey<Context> PARENT_CONTEXT_ATTRIBUTE_KEY =
3134
attributeKey("datadog.server.parent-context");
3235

‎dd-java-agent/instrumentation/reactor-netty-1.0/src/test/groovy/ReactorNettyHttp2ClientTest.groovy‎

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import reactor.netty.http.server.HttpServer
99
import spock.lang.IgnoreIf
1010
import spock.lang.Shared
1111

12+
import static datadog.trace.agent.test.utils.PortUtils.UNUSABLE_PORT
1213
import static datadog.trace.agent.test.utils.TraceUtils.basicSpan
1314
import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace
1415

@@ -24,16 +25,50 @@ class ReactorNettyHttp2ClientTest extends InstrumentationSpecification {
2425
.handle { req, res -> res.status(200).send() }
2526
.bindNow()
2627

27-
@Override
28-
boolean useStrictTraceWrites() {
29-
false
30-
}
31-
3228
@Override
3329
def cleanupSpec() {
3430
server?.disposeNow()
3531
}
3632

33+
def "test http2 prior knowledge failed connect creates connect error span"() {
34+
setup:
35+
HttpClient httpClient = HttpClient.create()
36+
.disableRetry(true)
37+
.protocol(HttpProtocol.H2C)
38+
39+
when:
40+
runUnderTrace("parent", {
41+
httpClient.baseUrl("http://127.0.0.1:${UNUSABLE_PORT}")
42+
.get()
43+
.uri("/")
44+
.response()
45+
.block()
46+
})
47+
48+
then:
49+
def ex = thrown(Exception)
50+
(ex instanceof ConnectException) || (ex.cause instanceof ConnectException)
51+
52+
and:
53+
assertTraces(1) {
54+
trace(2) {
55+
basicSpan(it, "parent", null, ex)
56+
57+
span {
58+
operationName "netty.connect"
59+
resourceName "netty.connect"
60+
childOf span(0)
61+
errored true
62+
tags {
63+
"$Tags.COMPONENT" "netty"
64+
errorTags Throwable, ~"Connection refused"
65+
defaultTags()
66+
}
67+
}
68+
}
69+
}
70+
}
71+
3772
def "test http2 client/server propagation"() {
3873
setup:
3974
HttpClient httpClient = HttpClient.create()

0 commit comments

Comments
 (0)