From 1a2fc90f0f0d3876cfba62bdb3f534430984a48a Mon Sep 17 00:00:00 2001 From: nmittler Date: Sun, 3 May 2015 08:49:49 -0700 Subject: [PATCH] Allowing inbound HTTP/2 frames after sending GOAWAY Motivation: If the client closes, a GOWAY is sent with a lastKnownStream of zero (since the remote side never created a stream). If there is still an exchange in progress, inbound frames for streams created by the client will be ignored because our ignore logic doesn't check to see if the stream was created by the remote endpoint. Frames for streams created by the local endpoint should continue to come through after sending GOAWAY. Modifications: Changed the decoder's streamCreatedAfterGoAwaySent logic to properly ensure that the stream was created remotely. Result: We now propertly process frames received after sending GOAWAY. --- .../codec/http2/DefaultHttp2Connection.java | 22 ++--- .../http2/DefaultHttp2ConnectionDecoder.java | 18 +++- .../handler/codec/http2/Http2Connection.java | 4 +- .../codec/http2/Http2ConnectionHandler.java | 4 +- .../DefaultHttp2ConnectionDecoderTest.java | 82 ++++++++++++++++--- .../DefaultHttp2ConnectionEncoderTest.java | 8 +- .../http2/DefaultHttp2ConnectionTest.java | 8 +- .../http2/Http2ConnectionHandlerTest.java | 2 +- 8 files changed, 108 insertions(+), 40 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 aadb93fe04..5c6bb7a406 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 @@ -147,12 +147,12 @@ public class DefaultHttp2Connection implements Http2Connection { @Override public boolean goAwayReceived() { - return localEndpoint.lastKnownStream >= 0; + return localEndpoint.lastStreamKnownByPeer >= 0; } @Override public void goAwayReceived(final int lastKnownStream, long errorCode, ByteBuf debugData) { - localEndpoint.lastKnownStream(lastKnownStream); + localEndpoint.lastStreamKnownByPeer(lastKnownStream); for (int i = 0; i < listeners.size(); ++i) { try { listeners.get(i).onGoAwayReceived(lastKnownStream, errorCode, debugData); @@ -178,12 +178,12 @@ public class DefaultHttp2Connection implements Http2Connection { @Override public boolean goAwaySent() { - return remoteEndpoint.lastKnownStream >= 0; + return remoteEndpoint.lastStreamKnownByPeer >= 0; } @Override public void goAwaySent(final int lastKnownStream, long errorCode, ByteBuf debugData) { - remoteEndpoint.lastKnownStream(lastKnownStream); + remoteEndpoint.lastStreamKnownByPeer(lastKnownStream); for (int i = 0; i < listeners.size(); ++i) { try { listeners.get(i).onGoAwaySent(lastKnownStream, errorCode, debugData); @@ -834,7 +834,7 @@ public class DefaultHttp2Connection implements Http2Connection { private final boolean server; private int nextStreamId; private int lastStreamCreated; - private int lastKnownStream = -1; + private int lastStreamKnownByPeer = -1; private boolean pushToAllowed = true; private F flowController; private int maxActiveStreams; @@ -989,12 +989,12 @@ public class DefaultHttp2Connection implements Http2Connection { } @Override - public int lastKnownStream() { - return lastKnownStream; + public int lastStreamKnownByPeer() { + return lastStreamKnownByPeer; } - private void lastKnownStream(int lastKnownStream) { - this.lastKnownStream = lastKnownStream; + private void lastStreamKnownByPeer(int lastKnownStream) { + this.lastStreamKnownByPeer = lastKnownStream; } @Override @@ -1013,10 +1013,10 @@ public class DefaultHttp2Connection implements Http2Connection { } private void checkNewStreamAllowed(int streamId) throws Http2Exception { - if (goAwayReceived() && streamId > localEndpoint.lastKnownStream()) { + 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.lastKnownStream()); + localEndpoint.lastStreamKnownByPeer()); } if (streamId < 0) { throw new Http2NoMoreStreamIdsException(); 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 0bf68ef679..656229fdf7 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 @@ -527,10 +527,22 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder { onUnknownFrame0(ctx, frameType, streamId, flags, payload); } + /** + * 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: + *

+ *

+ *

+ */ private boolean streamCreatedAfterGoAwaySent(int streamId) { - // Ignore inbound frames after a GOAWAY was sent and the stream id is greater than - // the last stream id set in the GOAWAY frame. - return connection.goAwaySent() && streamId > connection.remote().lastKnownStream(); + Http2Connection.Endpoint remote = connection.remote(); + return connection.goAwaySent() && remote.isValidStreamId(streamId) && + streamId > remote.lastStreamKnownByPeer(); } private void verifyStreamMayHaveExisted(int streamId) throws Http2Exception { 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 0acd1146a5..5ffd9437af 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 @@ -188,7 +188,7 @@ public interface Http2Connection { *

  • The connection is marked as going away.
    • * *

    - * This method differs from {@link #createdStreamId(int)} because the initial state of the stream will be + * This method differs from {@link #createIdleStream(int)} because the initial state of the stream will be * Immediately set before notifying {@link Listener}s. The state transition is sensitive to {@code halfClosed} * and is defined by {@link Http2Stream#open(boolean)}. * @param streamId The ID of the stream @@ -260,7 +260,7 @@ public interface Http2Connection { * If a GOAWAY was received for this endpoint, this will be the last stream ID from the * GOAWAY frame. Otherwise, this will be {@code -1}. */ - int lastKnownStream(); + int lastStreamKnownByPeer(); /** * Gets the flow controller for this endpoint. 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 44d5bc7467..c0a8b291c5 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 @@ -517,10 +517,10 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http final ByteBuf debugData, ChannelPromise promise) { try { final Http2Connection connection = connection(); - if (connection.goAwaySent() && connection.remote().lastKnownStream() < lastStreamId) { + if (connection.goAwaySent() && lastStreamId > connection.remote().lastStreamKnownByPeer()) { throw connectionError(PROTOCOL_ERROR, "Last stream identifier must not increase between " + "sending multiple GOAWAY frames (was '%d', is '%d').", - connection.remote().lastKnownStream(), + connection.remote().lastStreamKnownByPeer(), lastStreamId); } connection.goAwaySent(lastStreamId, errorCode, debugData); 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 648b022645..a56d644024 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 @@ -35,7 +35,6 @@ import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.never; -import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -65,9 +64,9 @@ import java.util.concurrent.atomic.AtomicInteger; * Tests for {@link DefaultHttp2ConnectionDecoder}. */ public class DefaultHttp2ConnectionDecoderTest { - private static final int STREAM_ID = 1; + private static final int STREAM_ID = 3; private static final int PUSH_STREAM_ID = 2; - private static final int STREAM_DEPENDENCY_ID = 3; + private static final int STREAM_DEPENDENCY_ID = 5; private Http2ConnectionDecoder decoder; @@ -169,8 +168,9 @@ public class DefaultHttp2ConnectionDecoderTest { } @Test - public void dataReadAfterGoAwayShouldApplyFlowControl() throws Exception { - when(connection.goAwaySent()).thenReturn(true); + public void dataReadAfterGoAwaySentShouldApplyFlowControl() throws Exception { + mockGoAwaySent(); + final ByteBuf data = dummyData(); int padding = 10; int processedBytes = data.readableBytes() + padding; @@ -187,6 +187,26 @@ public class DefaultHttp2ConnectionDecoderTest { } } + @Test + public void dataReadAfterGoAwaySentShouldAllowFramesForStreamCreatedByLocalEndpoint() throws Exception { + mockGoAwaySentShouldAllowFramesForStreamCreatedByLocalEndpoint(); + + final ByteBuf data = dummyData(); + int padding = 10; + int processedBytes = data.readableBytes() + padding; + mockFlowControl(processedBytes); + try { + decode().onDataRead(ctx, STREAM_ID, data, padding, true); + verify(localFlow).receiveFlowControlledFrame(eq(ctx), eq(stream), eq(data), eq(padding), eq(true)); + verify(localFlow).consumeBytes(eq(ctx), eq(stream), eq(processedBytes)); + + // Verify that the event was absorbed and not propagated to the observer. + verify(listener).onDataRead(eq(ctx), anyInt(), any(ByteBuf.class), anyInt(), anyBoolean()); + } finally { + data.release(); + } + } + @Test(expected = Http2Exception.class) public void dataReadForUnknownStreamShouldApplyFlowControlAndFail() throws Exception { when(connection.streamMayHaveExisted(STREAM_ID)).thenReturn(false); @@ -262,10 +282,10 @@ public class DefaultHttp2ConnectionDecoderTest { } @Test - public void dataReadAfterGoAwayForStreamInInvalidStateShouldIgnore() throws Exception { + public void dataReadAfterGoAwaySentForStreamInInvalidStateShouldIgnore() throws Exception { // Throw an exception when checking stream state. when(stream.state()).thenReturn(Http2Stream.State.CLOSED); - when(connection.goAwaySent()).thenReturn(true); + mockGoAwaySent(); final ByteBuf data = dummyData(); try { decode().onDataRead(ctx, STREAM_ID, data, 10, true); @@ -459,13 +479,21 @@ public class DefaultHttp2ConnectionDecoderTest { } @Test - public void pushPromiseReadAfterGoAwayShouldBeIgnored() throws Exception { - when(connection.goAwaySent()).thenReturn(true); + public void pushPromiseReadAfterGoAwaySentShouldBeIgnored() throws Exception { + mockGoAwaySent(); 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()); } + @Test + public void pushPromiseReadAfterGoAwayShouldAllowFramesForStreamCreatedByLocalEndpoint() throws Exception { + mockGoAwaySentShouldAllowFramesForStreamCreatedByLocalEndpoint(); + decode().onPushPromiseRead(ctx, STREAM_ID, PUSH_STREAM_ID, EmptyHttp2Headers.INSTANCE, 0); + verify(remote).reservePushStream(anyInt(), any(Http2Stream.class)); + verify(listener).onPushPromiseRead(eq(ctx), anyInt(), anyInt(), any(Http2Headers.class), anyInt()); + } + @Test(expected = Http2Exception.class) public void pushPromiseReadForUnknownStreamShouldThrow() throws Exception { when(connection.stream(STREAM_ID)).thenReturn(null); @@ -481,13 +509,21 @@ public class DefaultHttp2ConnectionDecoderTest { } @Test - public void priorityReadAfterGoAwayShouldBeIgnored() throws Exception { - when(connection.goAwaySent()).thenReturn(true); + public void priorityReadAfterGoAwaySentShouldBeIgnored() throws Exception { + mockGoAwaySent(); 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()); } + @Test + public void priorityReadAfterGoAwaySentShouldAllowFramesForStreamCreatedByLocalEndpoint() throws Exception { + mockGoAwaySentShouldAllowFramesForStreamCreatedByLocalEndpoint(); + decode().onPriorityRead(ctx, STREAM_ID, 0, (short) 255, true); + verify(stream).setPriority(anyInt(), anyShort(), anyBoolean()); + verify(listener).onPriorityRead(eq(ctx), anyInt(), anyInt(), anyShort(), anyBoolean()); + } + @Test public void priorityReadForUnknownStreamShouldBeIgnored() throws Exception { when(connection.stream(STREAM_ID)).thenReturn(null); @@ -521,13 +557,21 @@ public class DefaultHttp2ConnectionDecoderTest { } @Test - public void windowUpdateReadAfterGoAwayShouldBeIgnored() throws Exception { - when(connection.goAwaySent()).thenReturn(true); + public void windowUpdateReadAfterGoAwaySentShouldBeIgnored() throws Exception { + mockGoAwaySent(); decode().onWindowUpdateRead(ctx, STREAM_ID, 10); verify(remoteFlow, never()).incrementWindowSize(eq(ctx), any(Http2Stream.class), anyInt()); verify(listener, never()).onWindowUpdateRead(eq(ctx), anyInt(), anyInt()); } + @Test + public void windowUpdateReadAfterGoAwaySentShouldAllowFramesForStreamCreatedByLocalEndpoint() throws Exception { + mockGoAwaySentShouldAllowFramesForStreamCreatedByLocalEndpoint(); + decode().onWindowUpdateRead(ctx, STREAM_ID, 10); + verify(remoteFlow).incrementWindowSize(eq(ctx), any(Http2Stream.class), anyInt()); + verify(listener).onWindowUpdateRead(eq(ctx), anyInt(), anyInt()); + } + @Test(expected = Http2Exception.class) public void windowUpdateReadForUnknownStreamShouldThrow() throws Exception { when(connection.streamMayHaveExisted(STREAM_ID)).thenReturn(false); @@ -651,4 +695,16 @@ public class DefaultHttp2ConnectionDecoderTest { }).when(listener).onDataRead(any(ChannelHandlerContext.class), anyInt(), any(ByteBuf.class), anyInt(), anyBoolean()); } + + private void mockGoAwaySent() { + when(connection.goAwaySent()).thenReturn(true); + when(remote.isValidStreamId(STREAM_ID)).thenReturn(true); + when(remote.lastStreamKnownByPeer()).thenReturn(0); + } + + private void mockGoAwaySentShouldAllowFramesForStreamCreatedByLocalEndpoint() { + when(connection.goAwaySent()).thenReturn(true); + when(remote.isValidStreamId(STREAM_ID)).thenReturn(false); + when(remote.lastStreamKnownByPeer()).thenReturn(0); + } } 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 index bf198a4b49..b19ff56ac8 100644 --- 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 @@ -517,7 +517,7 @@ public class DefaultHttp2ConnectionEncoderTest { @Test public void canWriteDataFrameAfterGoAwaySent() { when(connection.goAwaySent()).thenReturn(true); - when(remote.lastKnownStream()).thenReturn(0); + when(remote.lastStreamKnownByPeer()).thenReturn(0); ByteBuf data = mock(ByteBuf.class); encoder.writeData(ctx, STREAM_ID, data, 0, false, promise); verify(remoteFlow).sendFlowControlled(eq(ctx), eq(stream), any(FlowControlled.class)); @@ -526,7 +526,7 @@ public class DefaultHttp2ConnectionEncoderTest { @Test public void canWriteHeaderFrameAfterGoAwaySent() { when(connection.goAwaySent()).thenReturn(true); - when(remote.lastKnownStream()).thenReturn(0); + when(remote.lastStreamKnownByPeer()).thenReturn(0); encoder.writeHeaders(ctx, STREAM_ID, EmptyHttp2Headers.INSTANCE, 0, false, promise); verify(remoteFlow).sendFlowControlled(eq(ctx), eq(stream), any(FlowControlled.class)); } @@ -534,7 +534,7 @@ public class DefaultHttp2ConnectionEncoderTest { @Test public void canWriteDataFrameAfterGoAwayReceived() { when(connection.goAwayReceived()).thenReturn(true); - when(local.lastKnownStream()).thenReturn(STREAM_ID); + when(local.lastStreamKnownByPeer()).thenReturn(STREAM_ID); ByteBuf data = mock(ByteBuf.class); encoder.writeData(ctx, STREAM_ID, data, 0, false, promise); verify(remoteFlow).sendFlowControlled(eq(ctx), eq(stream), any(FlowControlled.class)); @@ -543,7 +543,7 @@ public class DefaultHttp2ConnectionEncoderTest { @Test public void canWriteHeaderFrameAfterGoAwayReceived() { when(connection.goAwayReceived()).thenReturn(true); - when(local.lastKnownStream()).thenReturn(STREAM_ID); + when(local.lastStreamKnownByPeer()).thenReturn(STREAM_ID); encoder.writeHeaders(ctx, STREAM_ID, EmptyHttp2Headers.INSTANCE, 0, false, promise); verify(remoteFlow).sendFlowControlled(eq(ctx), eq(stream), any(FlowControlled.class)); } 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 4b2933f99d..91be041130 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 @@ -93,10 +93,10 @@ public class DefaultHttp2ConnectionTest { assertEquals(State.OPEN, stream1.state()); assertEquals(State.CLOSED, stream2.state()); assertEquals(State.OPEN, remoteStream.state()); - assertEquals(3, client.local().lastKnownStream()); + assertEquals(3, client.local().lastStreamKnownByPeer()); assertEquals(5, client.local().lastStreamCreated()); // The remote endpoint must not be affected by a received GOAWAY frame. - assertEquals(-1, client.remote().lastKnownStream()); + assertEquals(-1, client.remote().lastStreamKnownByPeer()); assertEquals(State.OPEN, remoteStream.state()); } @@ -111,10 +111,10 @@ public class DefaultHttp2ConnectionTest { assertEquals(State.OPEN, stream1.state()); assertEquals(State.CLOSED, stream2.state()); - assertEquals(3, server.remote().lastKnownStream()); + assertEquals(3, server.remote().lastStreamKnownByPeer()); assertEquals(5, server.remote().lastStreamCreated()); // The local endpoint must not be affected by a sent GOAWAY frame. - assertEquals(-1, server.local().lastKnownStream()); + assertEquals(-1, server.local().lastStreamKnownByPeer()); assertEquals(State.OPEN, localStream.state()); } 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 bb54701006..b7cda56f3e 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 @@ -351,7 +351,7 @@ public class Http2ConnectionHandlerTest { assertFalse(promise.isDone()); when(connection.goAwaySent()).thenReturn(true); - when(remote.lastKnownStream()).thenReturn(STREAM_ID); + when(remote.lastStreamKnownByPeer()).thenReturn(STREAM_ID); handler.goAway(ctx, STREAM_ID + 2, errorCode, data, promise); assertTrue(promise.isDone()); assertFalse(promise.isSuccess());