Skip to content

Commit

Permalink
Discard new writes without closing connection on AbortWritesEvent (#…
Browse files Browse the repository at this point in the history
…3102)

Motivation:

Behavior of `SslHandler` in Netty 4.1.115 changed. Now it can wrap data and flush them to the network even if flush was not requested. In result, it affected our test `ConnectionCloseHeaderHandlingTest.PipelinedRequestsTest.serverCloseSecondPipelinedRequestWriteAborted` that we had to disable in #3097 to proceed with the upgrade.
Debugging showed that when we receive `AbortWritesEvent`, the following behavior of `WriteStreamSubscriber` depends on how many `activeWrites` we have. With 4.1.115 netty will flush data, there will be 0 `activeWrites` and it will close (with reset) the outbound. In result, we lose data of the previous response in the pipeline that we haven't read yet.

Modifications:
- Use `listenerDiscard` instead of `channelClosed` that will make sure new writes will be discarded without prematurely affecting the connection state regardless of the `activeWrites` count.
- Unskip the test.

Result:

Graceful handling of `connection: close` header does not depend on flushing behavior.
  • Loading branch information
idelpivnitskiy authored Nov 14, 2024
1 parent 9bf91c4 commit 4187c05
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -390,8 +390,6 @@ void serverCloseTwoPipelinedRequestsSentBeforeFirstResponse(boolean useUds, bool
@MethodSource("io.servicetalk.http.netty.ConnectionCloseHeaderHandlingTest#pipelinedRequestsTestData")
void serverCloseSecondPipelinedRequestWriteAborted(boolean useUds, boolean viaProxy,
boolean awaitRequestPayload) throws Exception {
assumeFalse(viaProxy, "Let all other tests run with Netty 4.1.115");

setUp(useUds, viaProxy, awaitRequestPayload);
AtomicReference<Throwable> secondRequestError = new AtomicReference<>();
CountDownLatch secondResponseReceived = new CountDownLatch(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) {
} else if (evt == CloseHandler.OutboundDataEndEvent.INSTANCE) {
connection.channelOutboundListener.channelOutboundClosed();
} else if (evt == AbortWritesEvent.INSTANCE) {
connection.channelOutboundListener.channelClosed(StacklessClosedChannelException.newInstance(
connection.channelOutboundListener.listenerDiscard(StacklessClosedChannelException.newInstance(
DefaultNettyConnection.class, "userEventTriggered(AbortWritesEvent)"));
} else if (evt == ChannelOutputShutdownEvent.INSTANCE) {
connection.closeHandler.channelClosedOutbound(ctx);
Expand Down

0 comments on commit 4187c05

Please sign in to comment.