Skip to content

Only warn when writing to client before LastContent when a body is expected #2014

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 8, 2025
+57 −2
Merged

Only warn when writing to client before LastContent when a body is expected #2014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
996c5aa
Only warn when writing to client before LastContent when a body is ex…
gavinbunney Oct 8, 2025
a68adf3
fmt
gavinbunney Oct 8, 2025
953d18c
Dispose body in test
gavinbunney Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientResponseWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,10 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
channel.attr(ClientRequestReceiver.ATTR_ZUUL_RESP).set(zuulResponse);

if (channel.isActive()) {
// Track if this is happening.
// track if response is being written before receiving LastContent for requests with a body
if (!ClientRequestReceiver.isLastContentReceivedForChannel(channel)
&& !shouldAllowPreemptiveResponse(channel)) {
&& !shouldAllowPreemptiveResponse(channel)
&& zuulResponse.getInboundRequest().hasBody()) {
responseBeforeReceivedLastContentCounter.increment();
logger.warn(
Copy link
Contributor

@lalernehl lalernehl Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this warning useful overall?

Copy link
Collaborator Author

@gavinbunney gavinbunney Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be a signal of a misbehaving client or origin; will be interesting to see how it behaves in prod given the metric should reduce/provide insight into the cases when it does legitimately happen

"Writing response to client channel before have received the LastContent of request! {},"
Expand Down
54 changes: 54 additions & 0 deletions zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientResponseWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
import static org.assertj.core.api.Assertions.assertThat;

import com.netflix.netty.common.HttpLifecycleChannelHandler;
import com.netflix.spectator.api.Counter;
import com.netflix.spectator.api.DefaultRegistry;
import com.netflix.spectator.api.Registry;
import com.netflix.zuul.BasicRequestCompleteHandler;
import com.netflix.zuul.context.CommonContextKeys;
import com.netflix.zuul.context.SessionContext;
Expand All @@ -30,6 +33,7 @@
import com.netflix.zuul.stats.status.StatusCategory;
import com.netflix.zuul.stats.status.StatusCategoryUtils;
import com.netflix.zuul.stats.status.ZuulStatusCategory;
import io.netty.buffer.Unpooled;
import io.netty.channel.Channel;
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.ChannelOutboundHandlerAdapter;
Expand All @@ -43,6 +47,8 @@
import java.util.concurrent.atomic.AtomicReference;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
Expand Down Expand Up @@ -134,4 +140,52 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise)
HttpLifecycleChannelHandler.CompleteReason.SESSION_COMPLETE, null, nettyResp.get()));
assertThat(responseWriter.getZuulResponse()).isNull();
}

@ParameterizedTest
@ValueSource(booleans = {false, true})
Copy link
Collaborator

@jguerra jguerra Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

void warningOnlyForRequestsWithBody(boolean hasBodyChunk) {
Registry registry = new DefaultRegistry();
Counter warningCounter = registry.counter("server.http.requests.responseBeforeReceivedLastContent");
ClientResponseWriter responseWriter = new ClientResponseWriter(new BasicRequestCompleteHandler(), registry);
EmbeddedChannel channel = new EmbeddedChannel(responseWriter);

SessionContext ctx = new SessionContext();
HttpRequestMessage request = new HttpRequestBuilder(ctx).build();

if (hasBodyChunk) {
request.bufferBodyContents(new io.netty.handler.codec.http.DefaultHttpContent(
Unpooled.copiedBuffer("body", java.nio.charset.StandardCharsets.UTF_8)));
}
Copy link
Collaborator

@jguerra jguerra Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember if zuul-core tests have leak detection asserts or not, but can you release this if it isn't being released somewhere else already?

Copy link
Collaborator Author

@gavinbunney gavinbunney Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call - added a dispose call


request.storeInboundRequest();

HttpResponseMessageImpl response = new HttpResponseMessageImpl(ctx, request, 200);
response.setHeaders(new Headers());

channel.attr(ClientRequestReceiver.ATTR_ZUUL_REQ).set(request);
DefaultHttpRequest nettyRequest = new DefaultHttpRequest(
HttpVersion.HTTP_1_1,
hasBodyChunk ? HttpMethod.POST : HttpMethod.GET,
hasBodyChunk ? "/api" : "/favicon.ico");
ctx.set(CommonContextKeys.NETTY_HTTP_REQUEST, nettyRequest);

channel.pipeline().fireUserEventTriggered(new HttpLifecycleChannelHandler.StartEvent(nettyRequest));
channel.writeInbound(response);

assertThat(responseWriter.getZuulResponse()).isNotNull();
assertThat(request.hasBody()).isEqualTo(hasBodyChunk);

if (hasBodyChunk) {
assertThat(warningCounter.count())
.as("should warn as is body expected")
.isEqualTo(1);
} else {
assertThat(warningCounter.count())
.as("should not warn as no body expected")
.isEqualTo(0);
}

request.disposeBufferedBody();
channel.close();
}
}