Skip to content
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.

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

Use the correct error when reset a stream #13309

Merged
merged 1 commit into from
Mar 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,11 @@ void fireChildReadComplete() {
unsafe.notifyReadComplete(unsafe.recvBufAllocHandle(), false);
}

final void closeWithError(Http2Error error) {
assert eventLoop().inEventLoop();
unsafe.close(unsafe.voidPromise(), error);
}

private final class Http2ChannelUnsafe implements Unsafe {
private final VoidChannelPromise unsafeVoidPromise =
new VoidChannelPromise(AbstractHttp2StreamChannel.this, false);
Expand Down Expand Up @@ -648,6 +653,10 @@ public void disconnect(ChannelPromise promise) {

@Override
public void close(final ChannelPromise promise) {
close(promise, Http2Error.CANCEL);
}

void close(final ChannelPromise promise, Http2Error error) {
if (!promise.setUncancellable()) {
return;
}
Expand Down Expand Up @@ -678,7 +687,7 @@ public void operationComplete(ChannelFuture future) {
// Only ever send a reset frame if the connection is still alive and if the stream was created before
// as otherwise we may send a RST on a stream in an invalid state and cause a connection error.
if (parent().isActive() && !readEOS && isStreamIdValid(stream.id())) {
Http2StreamFrame resetFrame = new DefaultHttp2ResetFrame(Http2Error.CANCEL).stream(stream());
Http2StreamFrame resetFrame = new DefaultHttp2ResetFrame(error).stream(stream());
write(resetFrame, unsafe().voidPromise());
flush();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,9 @@ final void onHttp2FrameStreamException(ChannelHandlerContext ctx, Http2FrameStre
try {
channel.pipeline().fireExceptionCaught(cause.getCause());
} finally {
channel.unsafe().closeForcibly();
// Close with the correct error that causes this stream exception.
// See https://github.com/netty/netty/issues/13235#issuecomment-1441994672
channel.closeWithError(cause.error());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,9 @@ public void exceptionCaught(ChannelHandlerContext ctx, final Throwable cause) th
try {
childChannel.pipeline().fireExceptionCaught(cause.getCause());
} finally {
childChannel.unsafe().closeForcibly();
// Close with the correct error that causes this stream exception.
// See https://github.com/netty/netty/issues/13235#issuecomment-1441994672
childChannel.closeWithError(exception.error());
}
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,24 @@ public void execute() throws Throwable {
assertFalse(channel.isActive());
}

@Test
public void contentLengthNotMatchRstStreamWithProtocolError() {
final LastInboundHandler inboundHandler = new LastInboundHandler();
request.addLong(HttpHeaderNames.CONTENT_LENGTH, 10);
Http2StreamChannel channel = newInboundStream(3, false, inboundHandler);
frameInboundWriter.writeInboundData(3, bb(8), 0, true);
assertThrows(StreamException.class, new Executable() {
@Override
public void execute() throws Throwable {
inboundHandler.checkException();
}
});
assertNotNull(inboundHandler.readInbound());
assertFalse(channel.isActive());
verify(frameWriter).writeRstStream(eqCodecCtx(), eq(3),
eq(Http2Error.PROTOCOL_ERROR.code()), anyChannelPromise());
}

@Test
public void framesShouldBeMultiplexed() {
LastInboundHandler handler1 = new LastInboundHandler();
Expand Down