Repository: netty/netty
Issue: http2: f990f99 broke sending RST_STREAM
Labels: defect

In f990f99 (for #4758), Http2ConnectionHandler was changed from:

``` java
public ChannelFuture resetStream(...) {
  ...
  if (stream.state() == IDLE || (connection().local().created(stream) && !stream.isHeaderSent())) {
    // The other endpoint doesn't know about the stream yet, so we can't actually send
    // the RST_STREAM frame. The HTTP/2 spec also disallows sending RST_STREAM for IDLE streams.
```

To:

``` java
public ChannelFuture resetStream(...) {
  ...
  if (stream.state() == IDLE || connection().local().created(stream)) {
    // The other endpoint doesn't know about the stream yet, so we can't actually send
    // the RST_STREAM frame. The HTTP/2 spec also disallows sending RST_STREAM for IDLE streams.
```

However, the logic is now invalid, in that if the stream is created the RST_STREAM _won't_ be sent. Instead, the second part of the condition should be removed entirely and only base on `stream.state() == IDLE`.

This makes it hard to update to CR2.

@Scottmitch 
@nmittler 


Issue comments:
@ejona86 - Thanks for reporting! See https://github.com/netty/netty/pull/4857


Pull request review comments:
Wait ... is this right? I think we still need something here to make sure that the other endpoint doesn't know about the stream (see https://github.com/netty/netty/pull/4502). Or can we not get into `IDLE` via a remote-initiated stream?

#4502 was necessary because we used to queue the initial headers frames and it was possible for the user to do a `writeHeaders()` (headers would be queued) and then `writeReset()` (reset would be sent, but remote doesn't know about the stream and responds with a protocol error and the connection is closed). Now that we don't queue initial headers this scenario should not be possible (stream is opened and the remote peer doesn't know it) and we are now just trying to enforce the following:

https://tools.ietf.org/html/rfc7540#section-6.4

> RST_STREAM frames MUST NOT be sent for a stream in the "idle" state.
>    If a RST_STREAM frame identifying an idle stream is received, the
>    recipient MUST treat this as a connection error (Section 5.4.1) of
>    type PROTOCOL_ERROR.

However this restriction seems strange if the peer has sent a PRIORITY frame which creates a stream in the idle state ... we are not allowed to reset it ... and the peer may think it can later send frames on this stream ... but I think we now send a stream error in this case in [DefaultHttp2ConnectionDecoder](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java#L562).

sgtm ... I knew you were working on not-queuing initial headers, but I wasn't sure it had gone in yet. Thanks!


Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
index 8e98c3cfcd9..e8f92c14b70 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
@@ -617,9 +617,8 @@ public ChannelFuture resetStream(final ChannelHandlerContext ctx, int streamId,
         }
 
         final ChannelFuture future;
-        if (stream.state() == IDLE || connection().local().created(stream)) {
-            // The other endpoint doesn't know about the stream yet, so we can't actually send
-            // the RST_STREAM frame. The HTTP/2 spec also disallows sending RST_STREAM for IDLE streams.
+        if (stream.state() == IDLE) {
+            // We cannot write RST_STREAM frames on IDLE streams https://tools.ietf.org/html/rfc7540#section-6.4.
             future = promise.setSuccess();
         } else {
             future = frameWriter().writeRstStream(ctx, streamId, errorCode, promise);
@@ -629,17 +628,16 @@ public ChannelFuture resetStream(final ChannelHandlerContext ctx, int streamId,
         // from resulting in multiple reset frames being sent.
         stream.resetSent();
 
-        future.addListener(new ChannelFutureListener() {
-            @Override
-            public void operationComplete(ChannelFuture future) throws Exception {
-                if (future.isSuccess()) {
-                    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);
+        if (future.isDone()) {
+            processRstStreamWriteResult(ctx, stream, future);
+        } else {
+            future.addListener(new ChannelFutureListener() {
+                @Override
+                public void operationComplete(ChannelFuture future) throws Exception {
+                    processRstStreamWriteResult(ctx, stream, future);
                 }
-            }
-        });
+            });
+        }
 
         return future;
     }
@@ -688,10 +686,10 @@ private void checkCloseConnection(ChannelFuture future) {
         // If this connection is closing and the graceful shutdown has completed, close the connection
         // once this operation completes.
         if (closeListener != null && isGracefulShutdownComplete()) {
-            ChannelFutureListener closeListener = Http2ConnectionHandler.this.closeListener;
+            ChannelFutureListener closeListener = this.closeListener;
             // This method could be called multiple times
             // and we don't want to notify the closeListener multiple times.
-            Http2ConnectionHandler.this.closeListener = null;
+            this.closeListener = null;
             try {
                 closeListener.operationComplete(future);
             } catch (Exception e) {
@@ -717,6 +715,15 @@ private ChannelFuture goAway(ChannelHandlerContext ctx, Http2Exception cause) {
         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}.
      */


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
index 5dcd0ccbc18..c17cc4d8819 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
@@ -43,6 +43,7 @@
 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.Http2Stream.State.CLOSED;
+import static io.netty.handler.codec.http2.Http2Stream.State.IDLE;
 import static io.netty.util.CharsetUtil.UTF_8;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -321,6 +322,15 @@ public void writeRstOnClosedStreamShouldSucceed() throws Exception {
         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")
     @Test
     public void closeListenerShouldBeNotifiedOnlyOneTime() throws Exception {