diff --git a/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/BlockingResponseHandler.java b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/BlockingResponseHandler.java index fa73bcd2b99..8772b7d1403 100644 --- a/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/BlockingResponseHandler.java +++ b/dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/BlockingResponseHandler.java @@ -17,6 +17,7 @@ import io.netty.handler.codec.http.HttpResponseStatus; import io.netty.handler.codec.http.HttpUtil; import io.netty.handler.codec.http.HttpVersion; +import io.netty.handler.codec.http.websocketx.CloseWebSocketFrame; import io.netty.util.ReferenceCountUtil; import java.nio.channels.ClosedChannelException; import java.util.Map; @@ -247,8 +248,13 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) throws Exception { log.info("Ignored write of object {}", msg); ReferenceCountUtil.release(msg); - // Releasing the message does not complete the write; notify listeners that it failed. - promise.tryFailure(new ClosedChannelException()); + // Fail close frames so Netty's close listener runs. Complete other discarded writes + // successfully to avoid re-entering framework error handling (e.g. in Vert.x). + if (msg instanceof CloseWebSocketFrame) { + promise.tryFailure(new ClosedChannelException()); + } else { + promise.trySuccess(); + } } } } diff --git a/dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/server/BlockingResponseHandlerTest.java b/dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/server/BlockingResponseHandlerTest.java index 5ccba3de956..a95264bccc0 100644 --- a/dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/server/BlockingResponseHandlerTest.java +++ b/dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/server/BlockingResponseHandlerTest.java @@ -3,6 +3,7 @@ import static datadog.trace.instrumentation.netty41.server.BlockingResponseHandler.BEFORE_BLOCKING_HANDLER_NAME; import static datadog.trace.instrumentation.netty41.server.BlockingResponseHandler.HANDLER_NAME; import static io.netty.handler.codec.http.HttpMethod.GET; +import static io.netty.handler.codec.http.HttpResponseStatus.OK; import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1; import static java.util.Collections.emptyMap; import static java.util.concurrent.TimeUnit.SECONDS; @@ -18,6 +19,8 @@ import datadog.context.Context; import datadog.trace.instrumentation.netty41.ServerRequestContext; import datadog.trace.instrumentation.netty41.server.NettyHttpServerDecorator.NettyBlockResponseFunction; +import io.netty.buffer.Unpooled; +import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelInboundHandlerAdapter; import io.netty.channel.ChannelPromise; @@ -25,16 +28,66 @@ import io.netty.channel.embedded.EmbeddedChannel; import io.netty.channel.local.LocalChannel; import io.netty.handler.codec.http.DefaultFullHttpRequest; +import io.netty.handler.codec.http.DefaultFullHttpResponse; +import io.netty.handler.codec.http.DefaultLastHttpContent; import io.netty.handler.codec.http.FullHttpRequest; +import io.netty.handler.codec.http.HttpContent; import io.netty.handler.codec.http.websocketx.CloseWebSocketFrame; import io.netty.util.concurrent.Future; import java.nio.channels.ClosedChannelException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; class BlockingResponseHandlerTest { + @ParameterizedTest + @MethodSource("discardedHttpWritesArguments") + void discardedHttpWritesDoNotFireExceptions(HttpContent message) { + EmbeddedChannel channel = + new EmbeddedChannel(BlockingResponseHandler.IgnoreAllWritesHandler.INSTANCE); + try { + channel.pipeline().writeAndFlush(message, channel.voidPromise()); + + channel.checkException(); + assertEquals(0, message.refCnt()); + assertNull(channel.readOutbound()); + assertTrue(channel.isActive()); + } finally { + channel.finishAndReleaseAll(); + } + } + + @ParameterizedTest + @MethodSource("discardedHttpWritesArguments") + void discardedHttpWritesCompletePromiseAndNotifyListener(HttpContent message) { + EmbeddedChannel channel = + new EmbeddedChannel(BlockingResponseHandler.IgnoreAllWritesHandler.INSTANCE); + AtomicReference notifiedPromise = new AtomicReference<>(); + try { + ChannelFuture future = channel.pipeline().writeAndFlush(message); + future.addListener((ChannelFutureListener) notifiedPromise::set); + + assertTrue(future.isSuccess()); + assertSame(future, notifiedPromise.get()); + channel.checkException(); + assertEquals(0, message.refCnt()); + assertNull(channel.readOutbound()); + assertTrue(channel.isActive()); + } finally { + channel.finishAndReleaseAll(); + } + } + + static Stream discardedHttpWritesArguments() { + return Stream.of( + new DefaultFullHttpResponse(HTTP_1_1, OK, Unpooled.buffer().writeByte(1)), + new DefaultLastHttpContent(Unpooled.buffer().writeByte(1))); + } + @Test void discardedCloseFrameCompletesPromiseAndNotifiesCloseListener() { EmbeddedChannel channel =