From 74627483d7bdcae3130b2ba173e08ec5f2658377 Mon Sep 17 00:00:00 2001
From: Scott Mitchell <scott_mitchell@apple.com>
Date: Mon, 4 May 2015 15:15:44 -0700
Subject: [PATCH] [#3724] HTTP/2 Headers END_STREAM results in RST_STREAM

Motivation:
If headers are sent on a stream that does not yet exist and the END_STREAM flag is set we will send a RST_STREAM frame. We should send the HEADERS frame and no RST_STREAM.

Modifications:
DefaultHttp2RemoteFlowController should allow frames to be sent if stream is created in the 'half closed (local)' state.

Result:
We can send HEADERS frame with the END_STREAM flag sent without sending a RST_STREAM frame.
---
 .../codec/http2/DefaultHttp2Connection.java   |  6 ----
 .../http2/DefaultHttp2ConnectionDecoder.java  |  2 +-
 .../DefaultHttp2RemoteFlowController.java     |  3 +-
 .../handler/codec/http2/Http2Connection.java  |  6 ++--
 .../codec/http2/DataCompressionHttp2Test.java |  7 ++++
 .../http2/Http2ConnectionRoundtripTest.java   | 34 +++++++++++++++++--
 6 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java
index 5c6bb7a406..71a574f6cc 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java
@@ -434,9 +434,6 @@ public class DefaultHttp2Connection implements Http2Connection {
         public Http2Stream open(boolean halfClosed) throws Http2Exception {
             state = activeState(id, state, isLocal(), halfClosed);
             activate();
-            if (halfClosed) {
-                notifyHalfClosed(this);
-            }
             return this;
         }
 
@@ -900,9 +897,6 @@ public class DefaultHttp2Connection implements Http2Connection {
         public DefaultStream createStream(int streamId, boolean halfClosed) throws Http2Exception {
             DefaultStream stream = createStream(streamId, activeState(streamId, IDLE, isLocal(), halfClosed));
             stream.activate();
-            if (halfClosed) {
-                notifyHalfClosed(stream);
-            }
             return stream;
         }
 
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
index 656229fdf7..d477ea889e 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
@@ -529,7 +529,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
 
         /**
          * Helper method for determining whether or not to ignore inbound frames. A stream is considered to be created
-         * after a go away is sent if the following conditions hold:
+         * after a {@code GOAWAY} is sent if the following conditions hold:
          * <p/>
          * <ul>
          *     <li>A {@code GOAWAY} must have been sent by the local endpoint</li>
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java
index bc4d7d6e2c..047361506c 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java
@@ -605,8 +605,7 @@ public class DefaultHttp2RemoteFlowController implements Http2RemoteFlowControll
                     frame.writeComplete();
                 }
             } catch (Throwable t) {
-                // Mark the state as cancelled, we'll clear the pending queue
-                // via cancel() below.
+                // Mark the state as cancelled, we'll clear the pending queue via cancel() below.
                 cancelled = true;
                 cause = t;
             } finally {
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Connection.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Connection.java
index 5ffd9437af..178a501c97 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Connection.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Connection.java
@@ -43,8 +43,10 @@ public interface Http2Connection {
         void onStreamActive(Http2Stream stream);
 
         /**
-         * Notifies the listener that the given stream is now {@code HALF CLOSED}. The stream can be
-         * inspected to determine which side is {@code CLOSED}.
+         * Notifies the listener that the given stream has transitioned from {@code OPEN} to {@code HALF CLOSED}.
+         * This method will <strong>not</strong> be called until a state transition occurs from when
+         * {@link #onStreamActive(Http2Stream)} was called.
+         * The stream can be inspected to determine which side is {@code HALF CLOSED}.
          * <p>
          * If a {@link RuntimeException} is thrown it will be logged and <strong>not propagated</strong>.
          * Throwing from this method is not supported and is considered a programming error.
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DataCompressionHttp2Test.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DataCompressionHttp2Test.java
index 43b917e8c3..bdec97fd17 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DataCompressionHttp2Test.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DataCompressionHttp2Test.java
@@ -42,6 +42,7 @@ import io.netty.channel.socket.nio.NioServerSocketChannel;
 import io.netty.channel.socket.nio.NioSocketChannel;
 import io.netty.handler.codec.http.HttpHeaderNames;
 import io.netty.handler.codec.http.HttpHeaderValues;
+import io.netty.handler.codec.http2.Http2Stream.State;
 import io.netty.handler.codec.http2.Http2TestUtil.FrameAdapter;
 import io.netty.handler.codec.http2.Http2TestUtil.FrameCountDown;
 import io.netty.handler.codec.http2.Http2TestUtil.Http2Runnable;
@@ -270,6 +271,12 @@ public class DataCompressionHttp2Test {
         clientConnection = new DefaultHttp2Connection(false);
 
         serverConnection.addListener(new Http2ConnectionAdapter() {
+            @Override
+            public void onStreamActive(Http2Stream stream) {
+                if (stream.state() == State.HALF_CLOSED_LOCAL || stream.state() == State.HALF_CLOSED_REMOTE) {
+                    serverLatch.countDown();
+                }
+            }
             @Override
             public void onStreamHalfClosed(Http2Stream stream) {
                 serverLatch.countDown();
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java
index 9e85856137..ed19e2ac03 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java
@@ -30,6 +30,7 @@ import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyBoolean;
 import static org.mockito.Matchers.anyInt;
+import static org.mockito.Matchers.anyLong;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doThrow;
@@ -110,9 +111,38 @@ public class Http2ConnectionRoundtripTest {
         clientGroup.sync();
     }
 
+    @Test
+    public void headersWithEndStreamShouldNotSendError() throws Exception {
+        bootstrapEnv(1, 1, 2, 1);
+
+        // Create a single stream by sending a HEADERS frame to the server.
+        final short weight = 16;
+        final Http2Headers headers = dummyHeaders();
+        runInChannel(clientChannel, new Http2Runnable() {
+            @Override
+            public void run() {
+                http2Client.encoder().writeHeaders(ctx(), 3, headers, 0, weight, false, 0, true,
+                        newPromise());
+                ctx().flush();
+            }
+        });
+
+        assertTrue(requestLatch.await(5, SECONDS));
+        verify(serverListener).onHeadersRead(any(ChannelHandlerContext.class), eq(3), eq(headers),
+                eq(0), eq(weight), eq(false), eq(0), eq(true));
+        // Wait for some time to see if a go_away or reset frame will be received.
+        Thread.sleep(1000);
+
+        // Verify that no errors have been received.
+        verify(serverListener, never()).onGoAwayRead(any(ChannelHandlerContext.class), anyInt(),
+                anyLong(), any(ByteBuf.class));
+        verify(serverListener, never()).onRstStreamRead(any(ChannelHandlerContext.class), anyInt(),
+                anyLong());
+    }
+
     @Test
     public void http2ExceptionInPipelineShouldCloseConnection() throws Exception {
-        bootstrapEnv(1, 1, 1, 1);
+        bootstrapEnv(1, 1, 2, 1);
 
         // Create a latch to track when the close occurs.
         final CountDownLatch closeLatch = new CountDownLatch(1);
@@ -190,7 +220,7 @@ public class Http2ConnectionRoundtripTest {
 
     @Test
     public void nonHttp2ExceptionInPipelineShouldNotCloseConnection() throws Exception {
-        bootstrapEnv(1, 1, 1, 1);
+        bootstrapEnv(1, 1, 2, 1);
 
         // Create a latch to track when the close occurs.
         final CountDownLatch closeLatch = new CountDownLatch(1);