Repository: netty/netty
Issue: Cleanup HTTP/2 GO_AWAY methods
Labels: cleanup

Right now the determination of whether GO_AWAY was sent or received is a little confusing since each endpoint has a goAwayReceived() method.  This means that to determine if GO_AWAY was sent we would have to call connection.remote().goAwayReceived() which is a little clumsy and strange.

It would probably be better to just move back the GO_AWAY methods to 1st class citizens of the connection, so we'd have goAwaySent() and goAwayRecieved() ... this should make it much more clear going forward.


Issue comments:
@nmittler I think @normanmaurer cherry-picked and closed your PR.  Can this be closed?

I think so :)

> Am 14.10.2014 um 22:39 schrieb Scott Mitchell notifications@github.com:
> 
> @nmittler I think @normanmaurer cherry-picked and closed your PR. Can this be closed?
> 
> —
> Reply to this email directly or view it on GitHub.

@normanmaurer @Scottmitch sorry for the delay, I was OOO.  Thanks for cherry-picking!

@nmittler NP. Just wanted to make sure there wasn't any loose ends you intended on tying up.

@Scottmitch No I think it was good to go :)


Pull request review comments:

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 c69cc5bec21..043c2b8d9d1 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
@@ -149,7 +149,7 @@ public Endpoint remote() {
 
     @Override
     public boolean isGoAway() {
-        return localEndpoint.isGoAwayReceived() || remoteEndpoint.isGoAwayReceived();
+        return goAwaySent() || goAwayReceived();
     }
 
     @Override
@@ -162,6 +162,26 @@ public Http2Stream createRemoteStream(int streamId, boolean halfClosed) throws H
         return remote().createStream(streamId, halfClosed);
     }
 
+    @Override
+    public boolean goAwayReceived() {
+        return localEndpoint.lastKnownStream >= 0;
+    }
+
+    @Override
+    public void goAwayReceived(int lastKnownStream) {
+        localEndpoint.lastKnownStream(lastKnownStream);
+    }
+
+    @Override
+    public boolean goAwaySent() {
+        return remoteEndpoint.lastKnownStream >= 0;
+    }
+
+    @Override
+    public void goAwaySent(int lastKnownStream) {
+        remoteEndpoint.lastKnownStream(lastKnownStream);
+    }
+
     private void removeStream(DefaultStream stream) {
         // Notify the listeners of the event first.
         for (Listener listener : listeners) {
@@ -791,16 +811,10 @@ public int lastStreamCreated() {
 
         @Override
         public int lastKnownStream() {
-            return isGoAwayReceived() ? lastKnownStream : lastStreamCreated;
+            return lastKnownStream >= 0 ? lastKnownStream : lastStreamCreated;
         }
 
-        @Override
-        public boolean isGoAwayReceived() {
-            return lastKnownStream >= 0;
-        }
-
-        @Override
-        public void goAwayReceived(int lastKnownStream) {
+        private void lastKnownStream(int lastKnownStream) {
             boolean alreadyNotified = isGoAway();
             this.lastKnownStream = lastKnownStream;
             if (!alreadyNotified) {
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 ee4c0da8acd..d27fc6dc5fe 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
@@ -249,7 +249,7 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
             Http2Stream stream = connection.stream(streamId);
             verifyGoAwayNotReceived();
             verifyRstStreamNotReceived(stream);
-            if (connection.remote().isGoAwayReceived() || stream != null && shouldIgnoreFrame(stream)) {
+            if (connection.goAwaySent() || stream != null && shouldIgnoreFrame(stream)) {
                 // Ignore this frame.
                 return;
             }
@@ -426,7 +426,7 @@ public void onPushPromiseRead(ChannelHandlerContext ctx, int streamId, int promi
         public void onGoAwayRead(ChannelHandlerContext ctx, int lastStreamId, long errorCode, ByteBuf debugData)
                 throws Http2Exception {
             // Don't allow any more connections to be created.
-            connection.local().goAwayReceived(lastStreamId);
+            connection.goAwayReceived(lastStreamId);
 
             listener.onGoAwayRead(ctx, lastStreamId, errorCode, debugData);
         }
@@ -461,7 +461,7 @@ public void onUnknownFrame(ChannelHandlerContext ctx, byte frameType, int stream
          * stream/connection.
          */
         private boolean shouldIgnoreFrame(Http2Stream stream) {
-            if (connection.remote().isGoAwayReceived() && connection.remote().lastStreamCreated() <= stream.id()) {
+            if (connection.goAwaySent() && connection.remote().lastStreamCreated() <= stream.id()) {
                 // Frames from streams created after we sent a go-away should be ignored.
                 // Frames for the connection stream ID (i.e. 0) will always be allowed.
                 return true;
@@ -476,7 +476,7 @@ private boolean shouldIgnoreFrame(Http2Stream stream) {
          * exception.
          */
         private void verifyGoAwayNotReceived() throws Http2Exception {
-            if (connection.local().isGoAwayReceived()) {
+            if (connection.goAwayReceived()) {
                 throw protocolError("Received frames after receiving GO_AWAY");
             }
         }
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 8d77c3ad132..45985304db8 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
@@ -186,17 +186,6 @@ interface Endpoint {
          */
         int lastKnownStream();
 
-        /**
-         * Indicates whether or not a GOAWAY was received by this endpoint.
-         */
-        boolean isGoAwayReceived();
-
-        /**
-         * Indicates that a GOAWAY was received from the opposite endpoint and sets the last known stream
-         * created by this endpoint.
-         */
-        void goAwayReceived(int lastKnownStream);
-
         /**
          * Gets the {@link Endpoint} opposite this one.
          */
@@ -265,6 +254,26 @@ interface Endpoint {
      */
     Http2Stream createRemoteStream(int streamId, boolean halfClosed) throws Http2Exception;
 
+    /**
+     * Indicates whether or not a {@code GOAWAY} was received from the remote endpoint.
+     */
+    boolean goAwayReceived();
+
+    /**
+     * Indicates that a {@code GOAWAY} was received from the remote endpoint and sets the last known stream.
+     */
+    void goAwayReceived(int lastKnownStream);
+
+    /**
+     * Indicates whether or not a {@code GOAWAY} was sent to the remote endpoint.
+     */
+    boolean goAwaySent();
+
+    /**
+     * Indicates that a {@code GOAWAY} was sent to the remote endpoint and sets the last known stream.
+     */
+    void goAwaySent(int lastKnownStream);
+
     /**
      * Indicates whether or not either endpoint has received a GOAWAY.
      */
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 ce22ce2ffb2..3bfa9aa769b 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
@@ -343,7 +343,7 @@ public ChannelFuture writeGoAway(ChannelHandlerContext ctx, int lastStreamId, lo
         ChannelFuture future = frameWriter().writeGoAway(ctx, lastStreamId, errorCode, debugData, promise);
         ctx.flush();
 
-        connection().remote().goAwayReceived(lastStreamId);
+        connection().goAwaySent(lastStreamId);
         return future;
     }
 


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java
index 9de6880a0e7..b660542aadf 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoderTest.java
@@ -159,7 +159,7 @@ public Http2Stream answer(InvocationOnMock invocation) throws Throwable {
 
     @Test
     public void dataReadAfterGoAwayShouldApplyFlowControl() throws Exception {
-        when(remote.isGoAwayReceived()).thenReturn(true);
+        when(connection.goAwaySent()).thenReturn(true);
         final ByteBuf data = dummyData();
         try {
             decode().onDataRead(ctx, STREAM_ID, data, 10, true);
@@ -187,7 +187,7 @@ public void dataReadWithEndOfStreamShouldCloseRemoteSide() throws Exception {
 
     @Test
     public void headersReadAfterGoAwayShouldBeIgnored() throws Exception {
-        when(remote.isGoAwayReceived()).thenReturn(true);
+        when(connection.goAwaySent()).thenReturn(true);
         decode().onHeadersRead(ctx, STREAM_ID, EmptyHttp2Headers.INSTANCE, 0, false);
         verify(remote, never()).createStream(eq(STREAM_ID), eq(false));
 
@@ -235,7 +235,7 @@ public void headersReadForPromisedStreamShouldCloseStream() throws Exception {
 
     @Test
     public void pushPromiseReadAfterGoAwayShouldBeIgnored() throws Exception {
-        when(remote.isGoAwayReceived()).thenReturn(true);
+        when(connection.goAwaySent()).thenReturn(true);
         decode().onPushPromiseRead(ctx, STREAM_ID, PUSH_STREAM_ID, EmptyHttp2Headers.INSTANCE, 0);
         verify(remote, never()).reservePushStream(anyInt(), any(Http2Stream.class));
         verify(listener, never()).onPushPromiseRead(eq(ctx), anyInt(), anyInt(), any(Http2Headers.class), anyInt());
@@ -251,7 +251,7 @@ public void pushPromiseReadShouldSucceed() throws Exception {
 
     @Test
     public void priorityReadAfterGoAwayShouldBeIgnored() throws Exception {
-        when(remote.isGoAwayReceived()).thenReturn(true);
+        when(connection.goAwaySent()).thenReturn(true);
         decode().onPriorityRead(ctx, STREAM_ID, 0, (short) 255, true);
         verify(stream, never()).setPriority(anyInt(), anyShort(), anyBoolean());
         verify(listener, never()).onPriorityRead(eq(ctx), anyInt(), anyInt(), anyShort(), anyBoolean());
@@ -266,7 +266,7 @@ public void priorityReadShouldSucceed() throws Exception {
 
     @Test
     public void windowUpdateReadAfterGoAwayShouldBeIgnored() throws Exception {
-        when(remote.isGoAwayReceived()).thenReturn(true);
+        when(connection.goAwaySent()).thenReturn(true);
         decode().onWindowUpdateRead(ctx, STREAM_ID, 10);
         verify(encoder, never()).updateOutboundWindowSize(anyInt(), anyInt());
         verify(listener, never()).onWindowUpdateRead(eq(ctx), anyInt(), anyInt());
@@ -287,7 +287,7 @@ public void windowUpdateReadShouldSucceed() throws Exception {
 
     @Test
     public void rstStreamReadAfterGoAwayShouldSucceed() throws Exception {
-        when(remote.isGoAwayReceived()).thenReturn(true);
+        when(connection.goAwaySent()).thenReturn(true);
         decode().onRstStreamRead(ctx, STREAM_ID, PROTOCOL_ERROR.code());
         verify(lifecycleManager).closeStream(eq(stream), eq(future));
         verify(listener).onRstStreamRead(eq(ctx), anyInt(), anyLong());
@@ -342,7 +342,7 @@ public void settingsReadShouldSetValues() throws Exception {
     @Test
     public void goAwayShouldReadShouldUpdateConnectionState() throws Exception {
         decode().onGoAwayRead(ctx, 1, 2L, EMPTY_BUFFER);
-        verify(local).goAwayReceived(1);
+        verify(connection).goAwayReceived(1);
         verify(listener).onGoAwayRead(eq(ctx), eq(1), eq(2L), eq(EMPTY_BUFFER));
     }
 
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java
index 46596cd3f96..e64544591b1 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java
@@ -174,7 +174,7 @@ public void reserveWithPushDisallowedShouldThrow() throws Http2Exception {
 
     @Test(expected = Http2Exception.class)
     public void goAwayReceivedShouldDisallowCreation() throws Http2Exception {
-        server.local().goAwayReceived(0);
+        server.goAwayReceived(0);
         server.remote().createStream(3, true);
     }