Repository: netty/netty
Issue: HTTP/2 HEADERS stream dependency stream state
Labels: defect

The specification does not provide explicit direction as how to what state the `Stream Dependency` stream is expected to be in, and how implementations should react if it is not in the OPEN (or other "created" states).  The decoder may have to be updated by:
1. Move the `setPriority` above notification of the listener.
2. Potentially check the stream state of the `Stream Dependency` stream before accepting the frame.

https://github.com/http2/http2-spec/issues/739


Issue comments:
The behavior should be the same as the PRIORITY frame.  We should create the stream.  So we need to move the set priority above the notification.


Pull request review comments:

Patch that resolved the issue:
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 f839e6d0390..06eeced4aa1 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
@@ -307,11 +307,18 @@ public void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers
                 }
             }
 
+            try {
+                // This call will create a stream for streamDependency if necessary.
+                // For this reason it must be done before notifying the listener.
+                stream.setPriority(streamDependency, weight, exclusive);
+            } catch (ClosedStreamCreationException ignored) {
+                // It is possible that either the stream for this frame or the parent stream is closed.
+                // In this case we should ignore the exception and allow the frame to be sent.
+            }
+
             listener.onHeadersRead(ctx, streamId, headers,
                     streamDependency, weight, exclusive, padding, endOfStream);
 
-            stream.setPriority(streamDependency, weight, exclusive);
-
             // If the headers completes this stream, close it.
             if (endOfStream) {
                 lifecycleManager.closeRemoteSide(stream, ctx.newSucceededFuture());


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 82319597145..0591363d8d5 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
@@ -64,6 +64,7 @@
 public class DefaultHttp2ConnectionDecoderTest {
     private static final int STREAM_ID = 1;
     private static final int PUSH_STREAM_ID = 2;
+    private static final int STREAM_DEPENDENCY_ID = 3;
 
     private Http2ConnectionDecoder decoder;
 
@@ -344,6 +345,51 @@ public void headersReadForPromisedStreamShouldCloseStream() throws Exception {
                 eq(DEFAULT_PRIORITY_WEIGHT), eq(false), eq(0), eq(true));
     }
 
+    @Test
+    public void headersDependencyNotCreatedShouldCreateAndSucceed() throws Exception {
+        final short weight = 1;
+        decode().onHeadersRead(ctx, STREAM_ID, EmptyHttp2Headers.INSTANCE, STREAM_DEPENDENCY_ID,
+                weight, true, 0, true);
+        verify(listener).onHeadersRead(eq(ctx), eq(STREAM_ID), eq(EmptyHttp2Headers.INSTANCE), eq(STREAM_DEPENDENCY_ID),
+                eq(weight), eq(true), eq(0), eq(true));
+        verify(stream).setPriority(eq(STREAM_DEPENDENCY_ID), eq(weight), eq(true));
+        verify(lifecycleManager).closeRemoteSide(eq(stream), any(ChannelFuture.class));
+    }
+
+    @Test
+    public void headersDependencyPreviouslyCreatedStreamShouldSucceed() throws Exception {
+        final short weight = 1;
+        doAnswer(new Answer<Http2Stream>() {
+            @Override
+            public Http2Stream answer(InvocationOnMock in) throws Throwable {
+                throw new ClosedStreamCreationException(Http2Error.INTERNAL_ERROR);
+            }
+        }).when(stream).setPriority(eq(STREAM_DEPENDENCY_ID), eq(weight), eq(true));
+        decode().onHeadersRead(ctx, STREAM_ID, EmptyHttp2Headers.INSTANCE, STREAM_DEPENDENCY_ID,
+                weight, true, 0, true);
+        verify(listener).onHeadersRead(eq(ctx), eq(STREAM_ID), eq(EmptyHttp2Headers.INSTANCE), eq(STREAM_DEPENDENCY_ID),
+                eq(weight), eq(true), eq(0), eq(true));
+        verify(stream).setPriority(eq(STREAM_DEPENDENCY_ID), eq(weight), eq(true));
+        verify(lifecycleManager).closeRemoteSide(eq(stream), any(ChannelFuture.class));
+    }
+
+    @Test(expected = RuntimeException.class)
+    public void headersDependencyInvalidStreamShouldFail() throws Exception {
+        final short weight = 1;
+        doAnswer(new Answer<Http2Stream>() {
+            @Override
+            public Http2Stream answer(InvocationOnMock in) throws Throwable {
+                throw new RuntimeException("Fake Exception");
+            }
+        }).when(stream).setPriority(eq(STREAM_DEPENDENCY_ID), eq(weight), eq(true));
+        decode().onHeadersRead(ctx, STREAM_ID, EmptyHttp2Headers.INSTANCE, STREAM_DEPENDENCY_ID,
+                weight, true, 0, true);
+        verify(listener, never()).onHeadersRead(any(ChannelHandlerContext.class), anyInt(), any(Http2Headers.class),
+                anyInt(), anyShort(), anyBoolean(), anyInt(), anyBoolean());
+        verify(stream).setPriority(eq(STREAM_DEPENDENCY_ID), eq(weight), eq(true));
+        verify(lifecycleManager, never()).closeRemoteSide(eq(stream), any(ChannelFuture.class));
+    }
+
     @Test
     public void pushPromiseReadAfterGoAwayShouldBeIgnored() throws Exception {
         when(connection.goAwaySent()).thenReturn(true);
@@ -372,9 +418,9 @@ public void priorityReadAfterGoAwayShouldBeIgnored() throws Exception {
     public void priorityReadShouldSucceed() throws Exception {
         when(connection.stream(STREAM_ID)).thenReturn(null);
         when(connection.requireStream(STREAM_ID)).thenReturn(null);
-        decode().onPriorityRead(ctx, STREAM_ID, 0, (short) 255, true);
-        verify(stream).setPriority(eq(0), eq((short) 255), eq(true));
-        verify(listener).onPriorityRead(eq(ctx), eq(STREAM_ID), eq(0), eq((short) 255), eq(true));
+        decode().onPriorityRead(ctx, STREAM_ID, STREAM_DEPENDENCY_ID, (short) 255, true);
+        verify(stream).setPriority(eq(STREAM_DEPENDENCY_ID), eq((short) 255), eq(true));
+        verify(listener).onPriorityRead(eq(ctx), eq(STREAM_ID), eq(STREAM_DEPENDENCY_ID), eq((short) 255), eq(true));
         verify(remote).createStream(STREAM_ID);
         verify(stream, never()).open(anyBoolean());
     }
@@ -390,11 +436,11 @@ public Http2Stream answer(InvocationOnMock in) throws Throwable {
         when(connection.stream(STREAM_ID)).thenReturn(null);
         when(connection.requireStream(STREAM_ID)).thenReturn(null);
         // Just return the stream object as the connection stream to ensure the dependent stream "exists"
-        when(connection.stream(0)).thenReturn(stream);
-        when(connection.requireStream(0)).thenReturn(stream);
-        decode().onPriorityRead(ctx, STREAM_ID, 0, (short) 255, true);
+        when(connection.stream(STREAM_DEPENDENCY_ID)).thenReturn(stream);
+        when(connection.requireStream(STREAM_DEPENDENCY_ID)).thenReturn(stream);
+        decode().onPriorityRead(ctx, STREAM_ID, STREAM_DEPENDENCY_ID, (short) 255, true);
         verify(stream, never()).setPriority(anyInt(), anyShort(), anyBoolean());
-        verify(listener).onPriorityRead(eq(ctx), eq(STREAM_ID), eq(0), eq((short) 255), eq(true));
+        verify(listener).onPriorityRead(eq(ctx), eq(STREAM_ID), eq(STREAM_DEPENDENCY_ID), eq((short) 255), eq(true));
         verify(remote).createStream(STREAM_ID);
     }
 
@@ -405,12 +451,12 @@ public void priorityReadOnPreviouslyParentExistingStreamShouldSucceed() throws E
             public Http2Stream answer(InvocationOnMock in) throws Throwable {
                 throw new ClosedStreamCreationException(Http2Error.INTERNAL_ERROR);
             }
-        }).when(stream).setPriority(eq(0), eq((short) 255), eq(true));
+        }).when(stream).setPriority(eq(STREAM_DEPENDENCY_ID), eq((short) 255), eq(true));
         when(connection.stream(STREAM_ID)).thenReturn(stream);
         when(connection.requireStream(STREAM_ID)).thenReturn(stream);
-        decode().onPriorityRead(ctx, STREAM_ID, 0, (short) 255, true);
-        verify(stream).setPriority(eq(0), eq((short) 255), eq(true));
-        verify(listener).onPriorityRead(eq(ctx), eq(STREAM_ID), eq(0), eq((short) 255), eq(true));
+        decode().onPriorityRead(ctx, STREAM_ID, STREAM_DEPENDENCY_ID, (short) 255, true);
+        verify(stream).setPriority(eq(STREAM_DEPENDENCY_ID), eq((short) 255), eq(true));
+        verify(listener).onPriorityRead(eq(ctx), eq(STREAM_ID), eq(STREAM_DEPENDENCY_ID), eq((short) 255), eq(true));
     }
 
     @Test