Repository: netty/netty
Issue: HTTP/2 stream id roll over behavior
Labels: defect

It does not look like there is any code to handle the case where stream IDs roll over the largest number.  What is the expected behavior in this condition?  The client may want to open a new connection in this case to continue issuing requests.

Should this condition be checked for incoming and outgoing frames?


Issue comments:
@nmittler - FYI.  There is some info in section [5.1.1](https://tools.ietf.org/html/draft-ietf-httpbis-http2-14#section-5.1.1) related to this condition.

For the writer portion of this it is also possible we put this responsibility on the users of the http2-codec?  The http translation layer does allow users to specify streams, but may need some work in the case that stream ids are not provided by the user.  WDYT?

@Scottmitch The way it works now, an Http2Exception will be thrown when you try to create a stream after you've reached MAX_INT: https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java#L725

An application with access to the Http2Connection can always do their own verification before they attempt to create a stream so that the Http2Exception isn't thrown.

@nmittler - Good point...missed that.  Should this be a protocol error or a GOAWAY as suggested in [5.1.1](https://tools.ietf.org/html/draft-ietf-httpbis-http2-14#section-5.1.1)?

I agree on the second point.

Throwing an Http2Exception (i.e. connection error) when reading an inbound frame will result in a GO_AWAY, since onConnectionError will be called: https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L287

@nmittler - Well then looks like you are a couple step ahead of me ;)  Sorry for the noise.

@Scottmitch no worries! Keep the bugs coming! :)

@nmittler - Wait just a second... I think there is an issue here.  The protocol error never gets triggered because of an IndexOutOfBoundsException which originates from here https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java#L126 when the stream id wraps around and becomes negative.  The root cause seems to be the `index` variable here is `-1` https://github.com/netty/netty/blob/d35ddf0648f06c726c7d6a2eb1ccd7e65ed5564c/common/src/main/java/io/netty/util/collection/IntObjectHashMap.java#L296 which seems to be because of the modulus operation here is not playing nice with negative values https://github.com/netty/netty/blob/d35ddf0648f06c726c7d6a2eb1ccd7e65ed5564c/common/src/main/java/io/netty/util/collection/IntObjectHashMap.java#L315.

@Scottmitch good catch!  We should add a unit test for IntObjectHashMap for this.  Do you want to fix or should I?

@nmittler - Unless you have objections I'll let you take this one ;)

@Scottmitch created #3028 to address this.

@Scottmitch anything else that needs to be done here?

@nmittler - O yah I may have been a bit optimistic/premature on the closing of this issue. I had to rebase/rebuild my 3 PRs that I am using.  I am about to try and see the behavior after the build completes.

@nmittler - Behavior looks good.  Protocol exception is now being hit.  May be a good idea to add a unit test for this. WDYT?

@Scottmitch good idea .... since you're already in there, would you mind taking a crack at it?

@nmittler sure

@nmittler - I think we can leave this closed.  Thanks for your help!

@nmittler - After doing some more testing (and paying attention a bit more) on the write side we are not sending the GOAWAY in this condition.  Should we be?  Thoughts?

@nmittler - The promise gets correctly set here: https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java#L242 but the exception never reaches `onConnectionError`.

@Scottmitch right, for encoding it's a different situtation ... we just fail the promise.  The exception is for decoding.  Is that not what you would expect?

@nmittler - Not sure..My current rational is the codec is already checking for this condition and is their any alternative outcome (from a http2 protocol point of view) other than sending the goaway?  I can easily detect outside the codec but what do you think?

@Scottmitch I think for this particular error, that it does make sense to send a GO_AWAY on the writing side, since no more streams can be created. Other protocol violations that occur during writing should probably just fail the promise, however.  WDYT?

I agree.  This is the only exception I can think of at the moment that may result in special action on the write side.

@nmittler - How do you want to handle this?  One way would be to make a subclass of `Http2Exception` and check for it in `DefaultHttp2ConnectionEncoder.writeHeaders` catch clause.

@Scottmitch that seems reasonable to me.

@nmittler - PR is forthcoming...

@nmittler PR https://github.com/netty/netty/issues/3034 was submitted to resolve this issue.

Cherry picked #3034 


Pull request review comments:
@nmittler - Can you verify this change...I think this was a bug.

@nmittler - Note that I had an argument capture here to free the debug buffer but it is already freed.

@Scottmitch oof yeah I think you're right.  Good catch!

I don't think we should catch it here.  We should catch it in the encoder/decoder and call the lifecycle manager to write the GO_AWAY.

I don't think this belongs here.

I think we should revert this ... lifecycle manager (i.e. connection handler) is in charge of this.

@Scottmitch I think that's fine.

@nmittler - I agree...I was initially looking to use the `lifecycleManager` but for some reason thought it was only available at the `Http2ConnectionHandler` level.

Can you clarify the following:

> We should catch it in the encoder/decoder

Agreed...miss on my part.

Agreed.


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 2775ed745fa..635d755dbc1 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
@@ -854,8 +854,8 @@ private void checkNewStreamAllowed(int streamId) throws Http2Exception {
         }
 
         private void verifyStreamId(int streamId) throws Http2Exception {
-            if (nextStreamId < 0) {
-                throw protocolError("No more streams can be created on this connection");
+            if (streamId < 0) {
+                throw new Http2NoMoreStreamIdsException();
             }
             if (streamId < nextStreamId) {
                 throw protocolError("Request stream %d is behind the next expected stream %d", streamId, nextStreamId);
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
index 05e710b7cbf..70f2d6592e7 100644
--- 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
@@ -239,6 +239,9 @@ public ChannelFuture writeHeaders(ChannelHandlerContext ctx, int streamId, Http2
                 }
             }
         } catch (Throwable e) {
+            if (e instanceof Http2NoMoreStreamIdsException) {
+                lifecycleManager.onException(ctx, e);
+            }
             return promise.setFailure(e);
         }
 
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 19dbd0ef701..d09d0d27542 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
@@ -329,7 +329,8 @@ public ChannelFuture writeRstStream(ChannelHandlerContext ctx, int streamId, lon
      */
     public ChannelFuture writeGoAway(ChannelHandlerContext ctx, int lastStreamId, long errorCode, ByteBuf debugData,
             ChannelPromise promise) {
-        if (connection().isGoAway()) {
+        Http2Connection connection = connection();
+        if (connection.isGoAway()) {
             debugData.release();
             return ctx.newSucceededFuture();
         }
@@ -337,7 +338,7 @@ public ChannelFuture writeGoAway(ChannelHandlerContext ctx, int lastStreamId, lo
         ChannelFuture future = frameWriter().writeGoAway(ctx, lastStreamId, errorCode, debugData, promise);
         ctx.flush();
 
-        connection().goAwaySent(lastStreamId);
+        connection.goAwaySent(lastStreamId);
         return future;
     }
 
@@ -345,15 +346,16 @@ public ChannelFuture writeGoAway(ChannelHandlerContext ctx, int lastStreamId, lo
      * Sends a {@code GO_AWAY} frame appropriate for the given exception.
      */
     private ChannelFuture writeGoAway(ChannelHandlerContext ctx, Http2Exception cause) {
-        if (connection().isGoAway()) {
+        Http2Connection connection = connection();
+        if (connection.isGoAway()) {
             return ctx.newSucceededFuture();
         }
 
         // The connection isn't alredy going away, send the GO_AWAY frame now to start
         // the process.
-        int errorCode = cause != null ? cause.error().code() : NO_ERROR.code();
+        long errorCode = cause != null ? cause.error().code() : NO_ERROR.code();
         ByteBuf debugData = Http2CodecUtil.toByteBuf(ctx, cause);
-        int lastKnownStream = connection().remote().lastStreamCreated();
+        int lastKnownStream = connection.remote().lastStreamCreated();
         return writeGoAway(ctx, lastKnownStream, errorCode, debugData, ctx.newPromise());
     }
 
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Error.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Error.java
index 5fa92d2403b..0584adc18c5 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Error.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Error.java
@@ -33,16 +33,16 @@ public enum Http2Error {
     ENHANCE_YOUR_CALM(0xB),
     INADEQUATE_SECURITY(0xC);
 
-    private final int code;
+    private final long code;
 
-    Http2Error(int code) {
+    Http2Error(long code) {
         this.code = code;
     }
 
     /**
      * Gets the code for this error used on the wire.
      */
-    public int code() {
+    public long code() {
         return code;
     }
 }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2NoMoreStreamIdsException.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2NoMoreStreamIdsException.java
new file mode 100644
index 00000000000..7a79777386f
--- /dev/null
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2NoMoreStreamIdsException.java
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2014 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License, version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package io.netty.handler.codec.http2;
+
+/**
+ * This exception is thrown when there are no more stream IDs available for the current connection
+ */
+public class Http2NoMoreStreamIdsException extends Http2Exception {
+    private static final long serialVersionUID = -7756236161274851110L;
+    private static final String ERROR_MESSAGE = "No more streams can be created on this connection";
+
+    public Http2NoMoreStreamIdsException() {
+        super(Http2Error.PROTOCOL_ERROR, ERROR_MESSAGE);
+    }
+
+    public Http2NoMoreStreamIdsException(Throwable cause) {
+        super(Http2Error.PROTOCOL_ERROR, ERROR_MESSAGE, cause);
+    }
+}


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java
index 6aaf1682938..6491ced9015 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionRoundtripTest.java
@@ -217,6 +217,28 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
         assertTrue(clientChannel.isOpen());
     }
 
+    @Test
+    public void noMoreStreamIdsShouldSendGoAway() throws Exception {
+        bootstrapEnv(1, 3);
+
+        // Create a single stream by sending a HEADERS frame to the server.
+        final Http2Headers headers = dummyHeaders();
+        runInChannel(clientChannel, new Http2Runnable() {
+            @Override
+            public void run() {
+                http2Client.encoder().writeHeaders(ctx(), 3, headers, 0, (short) 16, false, 0,
+                        true, newPromise());
+                http2Client.encoder().writeHeaders(ctx(), Integer.MAX_VALUE + 1, headers, 0, (short) 16, false, 0,
+                        true, newPromise());
+            }
+        });
+
+        // Wait for the server to create the stream.
+        assertTrue(requestLatch.await(5, TimeUnit.SECONDS));
+        verify(serverListener).onGoAwayRead(any(ChannelHandlerContext.class), eq(0),
+                eq(Http2Error.PROTOCOL_ERROR.code()), any(ByteBuf.class));
+    }
+
     @Test
     public void flowControlProperlyChunksLargeMessage() throws Exception {
         final Http2Headers headers = dummyHeaders();