Repository: netty/netty
Issue: Http2MultiplexCodec in client mode can trigger a GOAWAY if it creates a stream after getting a GOAWAY

### Expected behavior

When a client attempts to utilize a new stream channel using Http2MultiplexCodec _after_ receiving a GOAWAY, the client will gracefully fail the stream channel with an exception that signifies the stream wasn't processed, perhaps such a RST_STREAM(REFUSED_STREAM).

### Actual behavior

When a client attempts to create a new stream channel using Http2MultiplexCodec _after_ receiving a GOAWAY, the client will throw it's own PROTOCOL_EXCEPTION that gets sent to the server and the connection gets torn down. It's also debatable as to whether the server should be actively tearing down the connection due to that message since it is still processing client streams.

### Steps to reproduce

Establish a client connection using Http2MultiplexCodec, have the server send a GOAWAY with one outstanding client stream, and attempt to create another client stream. You can observe a client-sent GOAWAY(last processed stream: 0) with a message similar to: 
```
Cannot create stream 5 since this endpoint has received a GOAWAY frame with last stream id 3.
```

### Minimal yet complete reproducer code (or URL to code)

### Netty version
4.1.26.Final-SNAPSHOT (as of 6/20/2018)

Issue comments:
A simple solution might be to add something like
```java
} else if (connection().goAwayReceived()) {
    ReferenceCountUtil.release(frame);
    promise.setFailure(Http2Exception.streamError(-1, Http2Error.REFUSED_STREAM, "Received GOAWAY before stream was initialized.")
    );
    return;
}
```
[here](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodec.java#L1102) in the write method of the `Http2MultiplexCodec.DefaultHttp2StreamChannel.Http2ChannelUnsafe`.
And I'm happy to work on this once we have a consensus on direction.
> PROTOCOL_EXCEPTION that gets sent to the server and the connection gets torn down.

Can you clarify exactly what is being sent to the server? If the connection is being hard-shutdown that sounds like a problem, but sending a GO_AWAY and initiating local graceful close isn't necessarily a problem IIUC (this just prevents the server from pushing any streams). The rational accompanied with the GO_AWAY is a bit awkward from the perspective of the server and is somewhat leaking client state ... but the rational is only meant to be informative anyways. Can you clarify if this is causing a problem or breaking something?

If we no longer want to send a GO_AWAY and initiate graceful closure from the local end point in this scenario I would propose the following patch (or something similar):

```diff
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 4251317457..1b6f15bc44 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
@@ -864,9 +864,9 @@ public class DefaultHttp2Connection implements Http2Connection {
         private void checkNewStreamAllowed(int streamId, State state) throws Http2Exception {
             assert state != IDLE;
             if (goAwayReceived() && streamId > localEndpoint.lastStreamKnownByPeer()) {
-                throw connectionError(PROTOCOL_ERROR, "Cannot create stream %d since this endpoint has received a " +
-                                                      "GOAWAY frame with last stream id %d.", streamId,
-                                                      localEndpoint.lastStreamKnownByPeer());
+                throw streamError(streamId, REFUSED_STREAM,
+                    "Cannot create stream %d since this endpoint has received a GOAWAY frame with last stream id %d.",
+                        streamId, localEndpoint.lastStreamKnownByPeer());
             }
             if (!isValidStreamId(streamId)) {
                 if (streamId < 0) {
```

The rational being the actual H2 logic resides in the lower level API and we should ensure consistent behavior between the child channel API and lower level API.
This is causing an issue as the client sends a GOAWAY and closes the connection on flush. The exception is generated in the [`DefaultHttp2ConnectionEncoder`](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java#L166) which passes it to the lifecycle manager [here](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java#L234), which passes it to the [`Http2ConnectionHandler`](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L617), and ultimately the `onConnectionError` method and the hard shutdown pathway [here](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L654).

I think the patch you're proposing will result in a stream exception which is fine from the side of the client, but I think it will result in sending a RST_STREAM to the peer for a stream in the idle state which is a connection level protocol error. We could try to intercept it for the case of 'trying to initiate a stream' but I suspect it will start to get complex. Alternatively, maybe we can just add the graceful shutdown hint to the connection error for this case, though threading that info through might also be pretty complex. If things are working right, that should put us in the 'not a problem' behavior you mentioned above where the client sends a GOAWAY but it's a draining GOAWAY. This is still pretty awkward since it's triggered by attempting to initiate a new stream as opposed to being a response to a received GOAWAY, and it signals draining of inbound streams in response to an outbound stream concern, and the client didn't signal that it wasn't interested in pushed streams anymore just because the server won't accept new streams.

What is the pattern that the users of the non-multiplexcodec API should use to ensure they don't start a new stream after receiving a GOAWAY? In our usage of the old-style API we handle GOAWAY frames manually and stop accepting new streams at that point [here](https://github.com/twitter/finagle/blob/develop/finagle-http2/src/main/scala/com/twitter/finagle/http2/transport/StreamTransportFactory.scala#L125). Attempts to initiate new dispatches check this state and fail if we've gotten a GOAWAY. This section of code has more or less the same job as the stream channels of Http2MultiplexCodec so that might be why it feels natural to do the state checking there.
@Scottmitch, do you have any more suggestions/opinions? I'd like to get this into 4.1.26 if at all possible, which I believe is knocking at the door. That said, I think it's more desirable to be correct than released earlier. 😄
I went ahead and opened PR https://github.com/netty/netty/pull/8057 with what feels like the right thing to do based on our usage of the old Http2 API, namely that we had to handle ensuring a GOAWAY hadn't been issued already ourselves. I put it up eagerly so that it has a chance of getting into Netty 4.1.26 if it's the right thing to do, but I'm happy to throw that away if we can come to a decision that something else should be done.
Sorry was AFK for the past few days ...

> I think it will result in sending a RST_STREAM to the peer for a stream in the idle state 

Good point. We would need to avoid violating the protocol. Failing the promise should be sufficient.

> the client didn't signal that it wasn't interested in pushed streams anymore just because the server won't accept new streams.

Another good point. I agree that forcing a GO_AWAY doesn't make much sense in this scenario.

In general I think we should make the fix in the interface API which is responsible for the core H2 logic instead of fragmenting the logic in the child channel API.  Everything above (e.g. child channel API) would then inherit the fix.
> In general I think we should make the fix in the interface API which is responsible for the core H2 logic instead of fragmenting the logic in the child channel API. Everything above (e.g. child channel API) would then inherit the fix.

Failing the promise on such a failed write should be sufficient to get the behavior I want and will work for all APIs. We can fail the promise for both API's either in the `Http2FrameCodec` or in the `DefaultHttp2ConnectionEncoder`. If we modify the `Endpoint.incrementAndGetNextStreamId()` method to consider received GOAWAY's it will fail the promise in `Http2FrameCodec` which feels pretty good.

I'd prefer the change to `incrementAndGetNextStreamId()` unless we don't consider it part of the `Endpoint`s job to validate that possibility. It seems like it should be.
See PR https://github.com/netty/netty/pull/8069 for my proposal. I think the change in `Http2ConnectionHandler` is useful either way bcz as you pointed out we may end up inadvertently sending a RST on a stream that never existed which would kill the connection.

> I'd prefer the change to incrementAndGetNextStreamId()

Can you clarify this suggestion. It isn't clear how `incrementAndGetNextStreamId` would be changed (e.g. would it throw, would it know about the promise, etc...).
@Scottmitch, see https://github.com/netty/netty/pull/8067 for what I mean by changing `incrementAndGetNextStreamId()`. It already has the semantics of returning negative values to signal that a stream id couldn't be assigned and this is extended to cover the case that a GOAWAY has been issued by the peer.

A quick skim of your PR looks like it would probably accomplish the same goal. It would have the result in burning reserved stream id's that would go unused, but I don't think that's of any real consequence. I don't see a reason that both changes couldn't coexist in case someone wants to use the `Http2ConnectionHandler` and not the `Http2FrameCodec`.
This is fixed now, though we've unfortunately hit the situation where the client will send a RST_STREAM(cancel) on an idle stream. I have a couple PRs up, any of which should fix it https://github.com/netty/netty/pull/8067, https://github.com/netty/netty/pull/8086, though the fallout is different for each.
https://github.com/netty/netty/pull/8086 has been merged, so I think this can be closed.

Pull request review comments:
Would it be cleaner to instead set `nextReservationStreamId = -1` (or Integer.MIN_VALUE) in `goAwayReceived()`?
Perhaps. The connection between `nextReservationStreamId` and `nextStreamIdToCreate` is somewhat subtle, but it looks safe to me. I'm happy to change it if you like.
I wonder if we should instead change `Http2FrameCodec` from this....

```java
final int streamId = connection.local().incrementAndGetNextStreamId();
if (streamId < 0) {
  promise.setFailure(new Http2NoMoreStreamIdsException());
  return;
}
stream.id = streamId;
```

to this...

```java
final Http2Stream oldStream;
try {
  oldStream = connection.local().createStream(connection.local().incrementAndGetNextStreamId(), false);
} catch (Throwable cause) {
  promise.setFailure(cause);
  return;
}
stream.id = oldStream.id();
```

this way the `Http2FrameCodec` doesn't have to make assumptions about `Http2NoMoreStreamIdsException` or create any new failure cases.
Right now the stream creation is driven almost entirely in the `Connection[Encoder|Decoder]`s (with the exception being h2c upgrade pathways and a stream error handling pathway for decoding headers that were too large). It makes a fair amount of sense to have it in the Encoder/Decoder due to the requirement of monotonically increasing stream-id's: having stream creation in the Encoder/Decoder helps ensure that we put HEADERS frames on the socket in the right order.

That said, it would probably work. I also don't think it's a good idea to be dolling out stream id's that look valid but aren't, but that's somewhat orthogonal other than combining both changes would result in throwing a [`Http2NoMoreStreamIdsException`](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java#L873) instead of the synthetic RST_STREAM you have in https://github.com/netty/netty/pull/8069. That is arguably a good thing since, as @ejona86 noted, we don't want to trigger retries against the same session.
humm seems like my suggestion doesn't play nicely with the `Http2FrameCodecBuilder.forServer().encoderEnforceMaxConcurrentStreams(true)` case which delays the stream creation because it relies upon lower layers to create the streams.

Patch that resolved the issue:
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 42513174572..4204945c323 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
@@ -708,7 +708,8 @@ private final class DefaultEndpoint<F extends Http2FlowController> implements En
 
         @Override
         public int incrementAndGetNextStreamId() {
-            return nextReservationStreamId >= 0 ? nextReservationStreamId += 2 : nextReservationStreamId;
+            return goAwayReceived() ? -1 :
+                   nextReservationStreamId >= 0 ? nextReservationStreamId += 2 : nextReservationStreamId;
         }
 
         private void incrementExpectedStreamId(int streamId) {


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
index 21239860472..55d81bdbb98 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
@@ -44,6 +44,7 @@
 import io.netty.util.concurrent.GlobalEventExecutor;
 import io.netty.util.concurrent.Promise;
 import io.netty.util.internal.ReflectionUtil;
+import org.hamcrest.Matchers;
 import org.junit.After;
 import org.junit.Assume;
 import org.junit.Before;
@@ -58,13 +59,7 @@
 
 import static io.netty.handler.codec.http2.Http2CodecUtil.isStreamIdValid;
 import static org.hamcrest.Matchers.instanceOf;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertThat;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.*;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyBoolean;
 import static org.mockito.Mockito.anyInt;
@@ -402,6 +397,20 @@ public void goAwayLastStreamIdOverflowed() throws Exception {
         assertTrue(channel.isActive());
     }
 
+    @Test
+    public void outboundStreamAfterGoAwayFails() throws Exception {
+        final Http2FrameStream stream = frameCodec.newStream();
+        frameListener.onGoAwayRead(http2HandlerCtx, 0, Http2Error.NO_ERROR.code(), Unpooled.EMPTY_BUFFER);
+
+        assertTrue(frameCodec.connection().goAwayReceived());
+        assertTrue(channel.isOpen());
+        ChannelFuture f = channel.writeAndFlush(
+                new DefaultHttp2HeadersFrame(new DefaultHttp2Headers(), false).stream(stream));
+
+        assertFalse(f.isSuccess());
+        assertThat(f.cause(), instanceOf(Http2NoMoreStreamIdsException.class));
+    }
+
     @Test
     public void streamErrorShouldFireExceptionForInbound() throws Exception {
         frameListener.onHeadersRead(http2HandlerCtx, 3, request, 31, false);