Don't send a RST on close of the stream may not have existed (#8086)

Motivation:

When a Http2MultiplexCodec stream channel fails to write the first
HEADERS it will forcibly close, and that will trigger sending a
RST_STREAM, which is commonly a connection level protocol error. This is
because it has what looks like a valid stream id, but didn't check with
the connection as to whether the stream may have actually existed.

Modifications:

Instead of checking if the stream was just a valid looking id ( > 0) we
check with the connection as to whether it may have existed at all.

Result:

We no longer send a RST_STREAM frame from Http2MultiplexCodec for idle
streams.
This commit is contained in:
Bryce Anderson 2018-07-05 18:09:23 -06:00 committed by Scott Mitchell
parent fa8f967852
commit 7f95506132
2 changed files with 51 additions and 4 deletions

View File

@ -906,9 +906,10 @@ public class Http2MultiplexCodec extends Http2FrameCodec {
final boolean wasActive = isActive();
// Only ever send a reset frame if the connection is still alive as otherwise it makes no sense at
// all anyway.
if (parent().isActive() && !streamClosedWithoutError && isStreamIdValid(stream().id())) {
// Only ever send a reset frame if the connection is still alive and if the stream may have existed
// as otherwise we may send a RST on a stream in an invalid state and cause a connection error.
if (parent().isActive() && !streamClosedWithoutError &&
connection().streamMayHaveExisted(stream().id())) {
Http2StreamFrame resetFrame = new DefaultHttp2ResetFrame(Http2Error.CANCEL).stream(stream());
write(resetFrame, unsafe().voidPromise());
flush();

View File

@ -308,6 +308,43 @@ public class Http2MultiplexCodecTest {
assertEquals(Http2Error.CANCEL.code(), reset.errorCode());
}
@Test
public void outboundStreamShouldNotWriteResetFrameOnClose_IfStreamDidntExist() {
writer = new Writer() {
private boolean headersWritten;
@Override
void write(Object msg, ChannelPromise promise) {
// We want to fail to write the first headers frame. This is what happens if the connection
// refuses to allocate a new stream due to having received a GOAWAY.
if (!headersWritten && msg instanceof Http2HeadersFrame) {
headersWritten = true;
Http2HeadersFrame headersFrame = (Http2HeadersFrame) msg;
final TestableHttp2MultiplexCodec.Stream stream =
(TestableHttp2MultiplexCodec.Stream) headersFrame.stream();
stream.id = 1;
promise.setFailure(new Exception("boom"));
} else {
super.write(msg, promise);
}
}
};
childChannelInitializer.handler = new ChannelInboundHandlerAdapter() {
@Override
public void channelActive(ChannelHandlerContext ctx) throws Exception {
ctx.writeAndFlush(new DefaultHttp2HeadersFrame(new DefaultHttp2Headers()));
ctx.fireChannelActive();
}
};
Channel childChannel = newOutboundStream();
assertFalse(childChannel.isActive());
childChannel.close();
parentChannel.runPendingTasks();
assertTrue(parentChannel.outboundMessages().isEmpty());
}
@Test
public void inboundRstStreamFireChannelInactive() {
LastInboundHandler inboundHandler = streamActiveAndWriteHeaders(inboundStream);
@ -745,7 +782,16 @@ public class Http2MultiplexCodecTest {
assertNotNull(headersFrame);
assertNotNull(headersFrame.stream());
assertFalse(Http2CodecUtil.isStreamIdValid(headersFrame.stream().id()));
((TestableHttp2MultiplexCodec.Stream) headersFrame.stream()).id = outboundStream.id();
TestableHttp2MultiplexCodec.Stream frameStream = (TestableHttp2MultiplexCodec.Stream) headersFrame.stream();
frameStream.id = outboundStream.id();
// Create the stream in the Http2Connection.
try {
Http2Stream stream = codec.connection().local().createStream(
headersFrame.stream().id(), headersFrame.isEndStream());
frameStream.stream = stream;
} catch (Exception ex) {
throw new IllegalStateException("Failed to create a stream", ex);
}
// Now read it and complete the write promise.
assertSame(headersFrame, parentChannel.readOutbound());