Repository: netty/netty (Java)

Issue:
HTTP/2 MultiplexCodec sends a RST_STREAM if the HEADERS fail to write due to HPACK issue
### Expected behavior

If the stream fails to write the headers due to a HPACK issue such as being larger than the MAX_HEADER_LIST_SIZE it should not send a RST_STREAM frame.

### Actual behavior

Sends a RST_STREAM frame.

There is a test for this sort of behavior [here](https://github.com/netty/netty/blob/1cc692dd7de54bfffa05813a90b14670b4eb50d1/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2MultiplexCodecTest.java#L312) but it doesn't consider that the `Http2FrameCodec` will create a new stream [here](https://github.com/netty/netty/blob/1cc692dd7de54bfffa05813a90b14670b4eb50d1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java#L352) which is circumvented in the tests via the `Writer` interface.

The reset is sent [here](https://github.com/netty/netty/blob/1cc692dd7de54bfffa05813a90b14670b4eb50d1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java#L892).

Without having thought about it a ton, I _think_ it should be generally safe to not send a RST_STREAM if the first write fails. The first message must always be HEADERS of some form (either regular or a PUSH_PROMISE) and if it gets past validation to where it's being written to the socket at all it should write in its entirety unless there is a socket problem in which case sending a RST_STREAM is no longer meaningful.

### Netty version

4.1 (SHA 1cc692dd7de, Oct 24, 2018)



Hints:
@ejona86 @Scottmitch @carl-mastrangelo thoughts ?
While I agree that we shouldn't be sending RST_STREAM if the headers frame fails, I think "first frame" may be an over-simplification. At the very least, that would not be true for server-side. It took me hours to convince myself that https://github.com/netty/netty/pull/8332 was "correct." This feels like another time-sink.

My original plan for the new H2 API was that Netty internals would be intelligent, so it would end up throwing away the RST in cases like this. I'm not against doing that in the framer/connectionhandler/etc. Reasoning for this is that it is the sanest way to avoid sharing state and avoiding races. Technically, a h2childchan could write(Headers), write(RST), flush(). Any of the current "HPACK failed" behavior is based on HPACK encoding being synchronous, which is bizarre with Netty's async model.
@ejona86 good point
That's right, for a server, a RST is still necessary if the first response headers fail to write.

So what is a way forward on this? The `HpackEncoder` throws a synchronous `StreamException` which feels like the right channel to deal with this. The problem is that we can't throw it out of the `FrameWriter.writeHeaders` method and thus it must go in the promise. The `DefaultHttp2ConnectionEncoder` already attempts to work around this [here](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java#L200). Maybe the best long term solution would be to allow the `FrameWriter` to throw `Http2Exception` so we can know if it was synchronous or not and act accordingly.

If the goal is to swallow RST_STREAM messages I think it will be tricky and also prone to races but probably doable. In this case when the `Http2MultiplexCodec` writes its reset in the `close` method, the connection sees the `HttpStream` in a state of `isHeadersSent() == true` and `isHeadersReceived() == fase`so in this case the connection believes the headers have been sent. This can probably be changed by not opening the stream in the `HALF_CLOSED_LOCAL` state and instead transitioning it to that state after the headers have been successfully written. Unfortunately that would happen in that same section of `DefaultHttp2ConnectionEncoder` code that works around the lack of synchronous exceptions so I think the best would be that we do this in conjunction with the above and just require that we didn't get a synchronous exception. Alternatively, we would need to buffer writes until the flush of the HEADERS completed in order to manage the case of a `StreamChannel` sending an explicit RST_STREAM. Despite this strategies shortcomings, at that point we should swallow this RST_STREAM in the `ConnectionHandler.resetStream` method.

Patch:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java
@@ -163,7 +163,12 @@ public ChannelFuture writeHeaders(final ChannelHandlerContext ctx, final int str
             Http2Stream stream = connection.stream(streamId);
             if (stream == null) {
                 try {
-                    stream = connection.local().createStream(streamId, endOfStream);
+                    // We don't create the stream in a `halfClosed` state because if this is an initial
+                    // HEADERS frame we don't want the connection state to signify that the HEADERS have
+                    // been sent until after they have been encoded and placed in the outbound buffer.
+                    // Therefore, we let the `LifeCycleManager` will take care of transitioning the state
+                    // as appropriate.
+                    stream = connection.local().createStream(streamId, /*endOfStream*/ false);
                 } catch (Http2Exception cause) {
                     if (connection.remote().mayHaveCreatedStream(streamId)) {
                         promise.tryFailure(new IllegalStateException("Stream no longer exists: " + streamId, cause));


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoderTest.java
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoderTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoderTest.java
@@ -710,7 +710,7 @@ public void headersWriteShouldHalfClosePushStream() throws Exception {
     }
 
     @Test
-    public void headersWriteShouldHalfCloseAfterOnError() throws Exception {
+    public void headersWriteShouldHalfCloseAfterOnErrorForPreCreatedStream() throws Exception {
         final ChannelPromise promise = newPromise();
         final Throwable ex = new RuntimeException();
         // Fake an encoding error, like HPACK's HeaderListSizeException
@@ -725,11 +725,38 @@ public ChannelFuture answer(InvocationOnMock invocation) {
             });
 
         writeAllFlowControlledFrames();
-        createStream(STREAM_ID, false);
+        Http2Stream stream = createStream(STREAM_ID, false);
+        encoder.writeHeaders(ctx, STREAM_ID, EmptyHttp2Headers.INSTANCE, 0, true, promise);
+
+        assertTrue(promise.isDone());
+        assertFalse(promise.isSuccess());
+        assertFalse(stream.isHeadersSent());
+        InOrder inOrder = inOrder(lifecycleManager);
+        inOrder.verify(lifecycleManager).onError(eq(ctx), eq(true), eq(ex));
+        inOrder.verify(lifecycleManager).closeStreamLocal(eq(stream(STREAM_ID)), eq(promise));
+    }
+
+    @Test
+    public void headersWriteShouldHalfCloseAfterOnErrorForImplicitlyCreatedStream() throws Exception {
+        final ChannelPromise promise = newPromise();
+        final Throwable ex = new RuntimeException();
+        // Fake an encoding error, like HPACK's HeaderListSizeException
+        when(writer.writeHeaders(eq(ctx), eq(STREAM_ID), eq(EmptyHttp2Headers.INSTANCE), eq(0),
+            eq(DEFAULT_PRIORITY_WEIGHT), eq(false), eq(0), eq(true), eq(promise)))
+            .thenAnswer(new Answer<ChannelFuture>() {
+                @Override
+                public ChannelFuture answer(InvocationOnMock invocation) {
+                    promise.setFailure(ex);
+                    return promise;
+                }
+            });
+
+        writeAllFlowControlledFrames();
         encoder.writeHeaders(ctx, STREAM_ID, EmptyHttp2Headers.INSTANCE, 0, true, promise);
 
         assertTrue(promise.isDone());
         assertFalse(promise.isSuccess());
+        assertFalse(stream(STREAM_ID).isHeadersSent());
         InOrder inOrder = inOrder(lifecycleManager);
         inOrder.verify(lifecycleManager).onError(eq(ctx), eq(true), eq(ex));
         inOrder.verify(lifecycleManager).closeStreamLocal(eq(stream(STREAM_ID)), eq(promise));