Skip to content

Commit

Permalink
Fix http pipelining test on Netty 4.1 (#8412)
Browse files Browse the repository at this point in the history
  • Loading branch information
laurit authored May 5, 2023
1 parent 83a4054 commit 0ecd146
Show file tree
Hide file tree
Showing 7 changed files with 18 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ class FinatraServerLatestTest extends AbstractHttpServerTest[HttpServer] {
override def test(endpoint: ServerEndpoint): Boolean =
endpoint != ServerEndpoint.NOT_FOUND
})
options.setTestHttpPipelining(false)
}

override protected def assertHandlerSpan(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ class FinatraServerTest extends AbstractHttpServerTest[HttpServer] {
override def test(endpoint: ServerEndpoint): Boolean =
endpoint != ServerEndpoint.NOT_FOUND
})
options.setTestHttpPipelining(false)
}

override protected def assertHandlerSpan(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import static io.opentelemetry.instrumentation.netty.v4_1.internal.server.HttpServerRequestTracingHandler.HTTP_SERVER_REQUEST;

import io.netty.channel.Channel;
import io.netty.channel.ChannelFuture;
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelOutboundHandlerAdapter;
Expand Down Expand Up @@ -69,18 +68,18 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) thr
if (msg instanceof FullHttpResponse) {
// Headers and body all sent together, we have the response information in the msg.
beforeCommitHandler.handle(context, (HttpResponse) msg);
contextAttr.set(null);
HttpRequestAndChannel request = ctx.channel().attr(HTTP_SERVER_REQUEST).getAndSet(null);
writePromise.addListener(
future -> end(ctx.channel(), (FullHttpResponse) msg, writePromise));
future -> end(context, request, (FullHttpResponse) msg, writePromise));
} else {
// Body sent after headers. We stored the response information in the context when
// encountering HttpResponse (which was not FullHttpResponse since it's not
// LastHttpContent).
writePromise.addListener(
future ->
end(
ctx.channel(),
ctx.channel().attr(HTTP_SERVER_RESPONSE).getAndSet(null),
writePromise));
contextAttr.set(null);
HttpRequestAndChannel request = ctx.channel().attr(HTTP_SERVER_REQUEST).getAndSet(null);
HttpResponse response = ctx.channel().attr(HTTP_SERVER_RESPONSE).getAndSet(null);
writePromise.addListener(future -> end(context, request, response, writePromise));
}
} else {
writePromise = prm;
Expand All @@ -94,20 +93,24 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise prm) thr
try (Scope ignored = context.makeCurrent()) {
super.write(ctx, msg, writePromise);
} catch (Throwable throwable) {
end(ctx.channel(), null, throwable);
contextAttr.set(null);
HttpRequestAndChannel request = ctx.channel().attr(HTTP_SERVER_REQUEST).getAndSet(null);
end(context, request, null, throwable);
throw throwable;
}
}

private void end(Channel channel, HttpResponse response, ChannelFuture future) {
private void end(
Context context, HttpRequestAndChannel request, HttpResponse response, ChannelFuture future) {
Throwable error = future.isSuccess() ? null : future.cause();
end(channel, response, error);
end(context, request, response, error);
}

// make sure to remove the server context on end() call
private void end(Channel channel, @Nullable HttpResponse response, @Nullable Throwable error) {
Context context = channel.attr(AttributeKeys.SERVER_CONTEXT).getAndSet(null);
HttpRequestAndChannel request = channel.attr(HTTP_SERVER_REQUEST).getAndSet(null);
private void end(
Context context,
HttpRequestAndChannel request,
@Nullable HttpResponse response,
@Nullable Throwable error) {
error = NettyErrorHolder.getOrDefault(context, error);
instrumenter.end(context, request, response, error);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ protected void configure(HttpServerTestOptions options) {
unused ->
Sets.difference(
DEFAULT_HTTP_ATTRIBUTES, Collections.singleton(SemanticAttributes.HTTP_ROUTE)));
options.setTestHttpPipelining(false);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,6 @@ class VertxRxHttpServerTest extends HttpServerTest<Vertx> implements AgentTestTr
return false
}

@Override
boolean testHttpPipelining() {
false
}

protected Class<AbstractVerticle> verticle() {
return VertxReactiveWebServer
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,6 @@ class VertxRxHttpServerTest extends HttpServerTest<Vertx> implements AgentTestTr
return true
}

@Override
boolean testHttpPipelining() {
false
}

@Override
boolean verifyServerSpanEndTime() {
// server spans are ended inside of the controller spans
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,4 @@ class VertxLatestHttpServerTest extends AbstractVertxHttpServerTest {
protected Class<? extends AbstractVerticle> verticle() {
return VertxLatestWebServer
}

@Override
boolean testHttpPipelining() {
false
}
}

0 comments on commit 0ecd146

Please sign in to comment.