Repository: netty/netty
Issue: HTTP/2 send goaway with error condition must close tcp connection
Labels: defect

[section 5.4.1](https://tools.ietf.org/html/draft-ietf-httpbis-http2-17#section-5.4.1):

> After sending the GOAWAY frame for an error condition,
>    the endpoint MUST close the TCP connection.

We should update the life cycle manager's `goAway` documentation such that upon successful go away the channel must be closed.  The `Htto2ConnectionHandler` should also be updated to implement this.


Issue comments:
@ejona86 - Thanks for brining my attention to this!

@nmittler - FYI.

@nmittler - I think this means we can get away without keeping history for https://github.com/netty/netty/issues/3557.  We can just send a connection error in these "bad" conditions and it is OK if state is "invalid" because we are closing the connection. WDTY?

@Scottmitch I'm not sure this gets us out of the woods.  Consider this scenario:
1. Client sends HEADERS
2. Client sends RST_STREAM
3. Server receives HEADERS
4. Server sends response HEADERS
5. Client receives response HEADERS and throws a connection error since the stream no longer exists.

This doesn't involve GO_AWAY at all.

Yes there will be legitimate race conditions to deal with however I would like to explore if the following code (or something similar) is "sufficient" to ignore a frame due to these sequencing / race condition scenarios:

``` java
private boolean streamMayHaveExisted(int streamId) {
    return connection.remote().createdStreamId(streamId) && streamId < connection.remote().nextStreamId();
}
```

If we feel that we should be returning errors after some "time" then we should keep some form of history.  Possibly just in a limited capacity as suggested by @buchgr https://github.com/netty/netty/issues/3557#issuecomment-94029067.

@Scottmitch wouldn't you need to check both endpoints, not just remote?

@Scottmitch +1

@nmittler - That is a good point.  I will think through the details but I'll submit a PR with this type of "simple" checks...and also explore "simple history" for problem streams.

@Scottmitch since you seem to have looked into the details of the spec regarding this issue, could you please summarize in a short post what's the required behavior for every case and what's no required (just in like 5 bullet points). I have kinda lost track of when we have to send a RST_STREAM frame, when we have to close the connection and when we have to ignore a frame and such ... thanks!

@Scottmitch @ejona86 @buchgr I was thinking how we might add the concept of `history` to the current design.  It seems like `Http2LifecycleManager` might be a good landing place for this.  It could have a `history` method that returns an interface that can be queried:

``` java
interface ConnectionHistory {
    enum StreamHistory {
        CLOSED_NORMALLY,
        RESET_SENT,
        RESET_RECEIVED
    }

    // Not sure if we still need this if we're maintaining closeState.
    boolean streamMayHaveExisted(int streamId);
    StreamHistory streamHistory(int streamId);
}
```

The implementation will be in the `Http2Connection` which can use @ejona86's scheme for swapping maps, described in https://github.com/netty/netty/issues/3557#issuecomment-93847587.

This should help us resolve the issues in the encoder and decoder WRT closed streams.

## Stream History and Flow Control ... :/

The only problem that still remains is to figure out what flow controllers should do for absent streams.  

Right now the flow controller interfaces require a `Http2Stream`, which makes sense so that we don't have to perform multiple map lookups on the same stream. I think the only case we need to consider is [receiving an inbound DATA frame for a closed stream](https://github.com/netty/netty/blob/19c864505d017a52fdbf1ef600715d42d36d36fa/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java#L193), which would mean that the only methods that need to handle closed streams are `receiveFlowControlledFrame`, `consumeBytes`, and `unconsumedBytes`.

To simplify things, we could make `StreamHistory` implement `Http2Stream` but be backed by very little data (e.g. streamId + close state). This would get us past the interface of the flow controller, but there would be no properties (i.e. flow state).  We could change the interface spec of the `Http2LocalFlowController`, requiring that it support closed streams with no flow state attached.  This all seems a little hacky, however ... likely needs more thought.

@nmittler that all sounds good to me. No allocations yay :-). don't have a good solution for the flow controller atm either. all a bit hacky :\

@Scottmitch I'd could try throwing together a PR for this to see what it looks like ... WDYT?

@nmittler - Sorry for the delayed response.  I'm still wondering if we need the history.  I'm going to switch gears back into this topic shortly but I think keeping history for every stream may be overkill. Is the primary motivator for keeping history to be able to know when receiving a frame associated with a stream we don't have an object allocated for an error or not?  If so I would still like to explore the approach described in https://github.com/netty/netty/issues/3653#issuecomment-94050151 (ignore frames for streams that "may" have existed).  If we can tolerate "ignoring" frames for streams that "may have existed" this may reduce the amount of work we have to do for the majority of the use cases.  WDYT?

I have a PR pending for this...


Pull request review comments:
What is the difference between `ctx.close()` and `ctx.channel().close()`?

The [AbstractChannelHandlerContext.close(..)](https://github.com/netty/netty/blob/master/transport/src/main/java/io/netty/channel/AbstractChannelHandlerContext.java#L440) does a bit of extra work related to the `invoker` to ensure you are in the proper event loop and also notifies the [DefaultChannelHandlerInvoker](https://github.com/netty/netty/blob/master/transport/src/main/java/io/netty/channel/DefaultChannelHandlerInvoker.java#L276) -> [ChannelOutBoundHandler](https://github.com/netty/netty/blob/master/transport/src/main/java/io/netty/channel/ChannelHandlerInvokerUtil.java#L133).  [AbstractChannel](https://github.com/netty/netty/blob/master/transport/src/main/java/io/netty/channel/AbstractChannel.java#L199) just closes the pipeline...which then closes the [AbstractChannelHandlerContext](https://github.com/netty/netty/blob/master/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java#L996). @normanmaurer - I think they boil down to the same thing in this case but....anything else?

@nmittler @Scottmitch when you call ctx.close() it starts to flow through the ChannelPipeline starting from this ChannelHandler... Channel.close() will start at the tail of the ChannelPipeline and so go through all handlers. Almost 99% of the time you want to use ChannelHandlerContext.close()

@Scottmitch wrap this in an if statement as we not need to do the format stuff if error logger is not enabled:

``` java
if (logger.isErrorEnabled()) {
    String msg = format(....);
    logger.error(msg);
}
ctx.close();
....
```

Done.


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 668e4e798b0..0d996be0120 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
@@ -562,11 +562,17 @@ public ChannelFuture goAway(final ChannelHandlerContext ctx, final int lastStrea
             future.addListener(new GenericFutureListener<ChannelFuture>() {
                 @Override
                 public void operationComplete(ChannelFuture future) throws Exception {
-                    if (!future.isSuccess()) {
-                        String msg = format("Sending GOAWAY failed: lastStreamId '%d', errorCode '%d', " +
-                                            "debugData '%s'.", lastStreamId, errorCode, debugData);
-                        logger.error(msg, future.cause());
-                        ctx.channel().close();
+                    if (future.isSuccess()) {
+                        if (errorCode != NO_ERROR.code()) {
+                            ctx.close();
+                        }
+                    } else {
+                        if (logger.isErrorEnabled()) {
+                            logger.error(
+                                   format("Sending GOAWAY failed: lastStreamId '%d', errorCode '%d', debugData '%s'.",
+                                           lastStreamId, errorCode, debugData), future.cause());
+                        }
+                        ctx.close();
                     }
                 }
             });
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2LifecycleManager.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2LifecycleManager.java
index 03904e82516..5efd94a8f03 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2LifecycleManager.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2LifecycleManager.java
@@ -66,11 +66,12 @@ ChannelFuture resetStream(ChannelHandlerContext ctx, int streamId, long errorCod
             ChannelPromise promise);
 
     /**
-     * Close the connection and prevent the peer from creating streams. After this call the peer
-     * is not allowed to create any new streams and the local endpoint will be limited to creating streams with
-     * {@code stream identifier <= lastStreamId}. This may result in sending a {@code GO_AWAY} frame (assuming we
-     * have not already sent one with {@code Last-Stream-ID <= lastStreamId}), or may just return success if a
-     * {@code GO_AWAY} has previously been sent.
+     * Prevents the peer from creating streams and close the connection if {@code errorCode} is not
+     * {@link Http2Error#NO_ERROR}. After this call the peer is not allowed to create any new streams and the local
+     * endpoint will be limited to creating streams with {@code stream identifier <= lastStreamId}. This may result in
+     * sending a {@code GO_AWAY} frame (assuming we have not already sent one with
+     * {@code Last-Stream-ID <= lastStreamId}), or may just return success if a {@code GO_AWAY} has previously been
+     * sent.
      * @param ctx The context used for communication and buffer allocation if necessary.
      * @param lastStreamId The last stream that the local endpoint is claiming it will accept.
      * @param errorCode The rational as to why the connection is being closed. See {@link Http2Error}.


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 899af813d4d..832ff66f401 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
@@ -301,14 +301,27 @@ public Http2Stream answer(InvocationOnMock in) throws Throwable {
         verify(ctx, times(1)).close(any(ChannelPromise.class));
     }
 
+    @SuppressWarnings("unchecked")
+    @Test
     public void canSendGoAwayFrame() throws Exception {
-        handler = newHandler();
         ByteBuf data = mock(ByteBuf.class);
         long errorCode = Http2Error.INTERNAL_ERROR.code();
+        when(future.isDone()).thenReturn(true);
+        when(future.isSuccess()).thenReturn(true);
+        when(frameWriter.writeGoAway(eq(ctx), eq(STREAM_ID), eq(errorCode), eq(data), eq(promise))).thenReturn(future);
+        doAnswer(new Answer<Void>() {
+            @Override
+            public Void answer(InvocationOnMock invocation) throws Throwable {
+                invocation.getArgumentAt(0, GenericFutureListener.class).operationComplete(future);
+                return null;
+            }
+        }).when(future).addListener(any(GenericFutureListener.class));
+        handler = newHandler();
         handler.goAway(ctx, STREAM_ID, errorCode, data, promise);
 
         verify(connection).goAwaySent(eq(STREAM_ID), eq(errorCode), eq(data));
         verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID), eq(errorCode), eq(data), eq(promise));
+        verify(ctx).close();
     }
 
     @Test