HTTP/2 RST_STREAM Regression f990f99

Motivation:
Commit f990f99 introduced a bug into the RST_STREAM processing that would prevent a RST_STREAM from being sent when it should have been. The promise would be marked as successful even though the RST_STREAM frame would never be sent.

Modifications:
- Fix conditional in Http2ConnectionHandler.resetStream to allow reset streams to be sent in all stream states besides IDLE.

Result:
RST_STREAM frames are now sent when they are supposed to be sent
Fixes https://github.com/netty/netty/issues/4856
This commit is contained in:
Scott Mitchell 2016-02-09 14:53:38 -08:00
parent 36aa11937d
commit 56e6e07b25
2 changed files with 32 additions and 15 deletions

View File

@ -617,9 +617,8 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
} }
final ChannelFuture future; final ChannelFuture future;
if (stream.state() == IDLE || connection().local().created(stream)) { if (stream.state() == IDLE) {
// The other endpoint doesn't know about the stream yet, so we can't actually send // We cannot write RST_STREAM frames on IDLE streams https://tools.ietf.org/html/rfc7540#section-6.4.
// the RST_STREAM frame. The HTTP/2 spec also disallows sending RST_STREAM for IDLE streams.
future = promise.setSuccess(); future = promise.setSuccess();
} else { } else {
future = frameWriter().writeRstStream(ctx, streamId, errorCode, promise); future = frameWriter().writeRstStream(ctx, streamId, errorCode, promise);
@ -629,17 +628,16 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
// from resulting in multiple reset frames being sent. // from resulting in multiple reset frames being sent.
stream.resetSent(); stream.resetSent();
if (future.isDone()) {
processRstStreamWriteResult(ctx, stream, future);
} else {
future.addListener(new ChannelFutureListener() { future.addListener(new ChannelFutureListener() {
@Override @Override
public void operationComplete(ChannelFuture future) throws Exception { public void operationComplete(ChannelFuture future) throws Exception {
if (future.isSuccess()) { processRstStreamWriteResult(ctx, stream, future);
closeStream(stream, promise);
} else {
// The connection will be closed and so no need to change the resetSent flag to false.
onConnectionError(ctx, future.cause(), null);
}
} }
}); });
}
return future; return future;
} }
@ -688,10 +686,10 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
// If this connection is closing and the graceful shutdown has completed, close the connection // If this connection is closing and the graceful shutdown has completed, close the connection
// once this operation completes. // once this operation completes.
if (closeListener != null && isGracefulShutdownComplete()) { if (closeListener != null && isGracefulShutdownComplete()) {
ChannelFutureListener closeListener = Http2ConnectionHandler.this.closeListener; ChannelFutureListener closeListener = this.closeListener;
// This method could be called multiple times // This method could be called multiple times
// and we don't want to notify the closeListener multiple times. // and we don't want to notify the closeListener multiple times.
Http2ConnectionHandler.this.closeListener = null; this.closeListener = null;
try { try {
closeListener.operationComplete(future); closeListener.operationComplete(future);
} catch (Exception e) { } catch (Exception e) {
@ -717,6 +715,15 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
return goAway(ctx, lastKnownStream, errorCode, debugData, ctx.newPromise()); return goAway(ctx, lastKnownStream, errorCode, debugData, ctx.newPromise());
} }
private void processRstStreamWriteResult(ChannelHandlerContext ctx, Http2Stream stream, ChannelFuture future) {
if (future.isSuccess()) {
closeStream(stream, future);
} else {
// The connection will be closed and so no need to change the resetSent flag to false.
onConnectionError(ctx, future.cause(), null);
}
}
/** /**
* Returns the client preface string if this is a client connection, otherwise returns {@code null}. * Returns the client preface string if this is a client connection, otherwise returns {@code null}.
*/ */

View File

@ -43,6 +43,7 @@ import static io.netty.handler.codec.http2.Http2CodecUtil.connectionPrefaceBuf;
import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
import static io.netty.handler.codec.http2.Http2Error.STREAM_CLOSED; import static io.netty.handler.codec.http2.Http2Error.STREAM_CLOSED;
import static io.netty.handler.codec.http2.Http2Stream.State.CLOSED; import static io.netty.handler.codec.http2.Http2Stream.State.CLOSED;
import static io.netty.handler.codec.http2.Http2Stream.State.IDLE;
import static io.netty.util.CharsetUtil.UTF_8; import static io.netty.util.CharsetUtil.UTF_8;
import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertFalse;
@ -321,6 +322,15 @@ public class Http2ConnectionHandlerTest {
verify(frameWriter).writeRstStream(eq(ctx), eq(STREAM_ID), anyLong(), any(ChannelPromise.class)); verify(frameWriter).writeRstStream(eq(ctx), eq(STREAM_ID), anyLong(), any(ChannelPromise.class));
} }
@Test
public void writeRstOnIdleStreamShouldNotWriteButStillSucceed() throws Exception {
handler = newHandler();
when(stream.state()).thenReturn(IDLE);
handler.resetStream(ctx, STREAM_ID, STREAM_CLOSED.code(), promise);
verify(frameWriter, never()).writeRstStream(eq(ctx), eq(STREAM_ID), anyLong(), any(ChannelPromise.class));
verify(stream).close();
}
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")
@Test @Test
public void closeListenerShouldBeNotifiedOnlyOneTime() throws Exception { public void closeListenerShouldBeNotifiedOnlyOneTime() throws Exception {