Repository: netty/netty
Issue: Http2: writing valid PRIORITY frame fails with misleading error message

### Expected behavior
* Writing valid PRIORITY frame with `streamDependency=0` succeeds
*  PRIORITY frame validation message contains actual cause of error

### Actual behavior
Writing PRIORITY frame with `streamDependency=0`
```
http2Handler.encoder()
     .writePriority(ctx, streamId, /*dependency*/ 0, weight, /*exclusive*/ false, ctx.newPromise());
```

fails with misleading error message

```
java.lang.IllegalArgumentException: streamId: 0 (expected: > 0)
 at io.netty.util.internal.ObjectUtil.checkPositive(ObjectUtil.java:44)
 at io.netty.handler.codec.http2.DefaultHttp2FrameWriter.verifyStreamId(DefaultHttp2FrameWriter.java:605)
 at io.netty.handler.codec.http2.DefaultHttp2FrameWriter.writePriority(DefaultHttp2FrameWriter.java:277)
 at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder.writePriority(DefaultHttp2ConnectionEncoder.java:278)
 at io.netty.handler.codec.http2.DecoratingHttp2FrameWriter.writePriority(DecoratingHttp2FrameWriter.java:59)
```
caused by [DefaultHttp2FrameWriter](https://github.com/netty/netty/blob/28e1fbd366602d8f4b50921afb2c9d5e41b94a2d/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java#L277) that expects `streamDependency > 0`,  
but fails with message `java.lang.IllegalArgumentException: streamId: 0 (expected: > 0)`. 

### Steps to reproduce

### Minimal yet complete reproducer code (or URL to code)

### Netty version
4.1.51.Final
### JVM version (e.g. `java -version`)

### OS version (e.g. `uname -a`)


Issue comments:
@ejona86 can you have a look ?
I'll send out a PR.

The `verifyStreamId()` check for `streamDependency` should be a `verifyStreamOrConnectionId()`:
https://github.com/netty/netty/blob/f58223982ce8d9ed056c3d127972222966d0ebd1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java#L273-L277

The mislabled error seems to be a bug in the verify function, in that it ignores `argumentName`:
https://github.com/netty/netty/blob/f58223982ce8d9ed056c3d127972222966d0ebd1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java#L604-L610

It looks like the reading direction has the correct verification:
https://github.com/netty/netty/blob/f58223982ce8d9ed056c3d127972222966d0ebd1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java#L501-L511

It later re-verifies with (although note these are asserts, so they are internal checks and not for verifying remotes):
https://github.com/netty/netty/blob/f58223982ce8d9ed056c3d127972222966d0ebd1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java#L184-L186

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java
index 71a6653437c..00a4fe42255 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java
@@ -274,7 +274,7 @@ public ChannelFuture writePriority(ChannelHandlerContext ctx, int streamId,
             int streamDependency, short weight, boolean exclusive, ChannelPromise promise) {
         try {
             verifyStreamId(streamId, STREAM_ID);
-            verifyStreamId(streamDependency, STREAM_DEPENDENCY);
+            verifyStreamOrConnectionId(streamDependency, STREAM_DEPENDENCY);
             verifyWeight(weight);
 
             ByteBuf buf = ctx.alloc().buffer(PRIORITY_FRAME_LENGTH);
@@ -602,11 +602,11 @@ private static void writePaddingLength(ByteBuf buf, int padding) {
     }
 
     private static void verifyStreamId(int streamId, String argumentName) {
-        checkPositive(streamId, "streamId");
+        checkPositive(streamId, argumentName);
     }
 
     private static void verifyStreamOrConnectionId(int streamId, String argumentName) {
-        checkPositiveOrZero(streamId, "streamId");
+        checkPositiveOrZero(streamId, argumentName);
     }
 
     private static void verifyWeight(short weight) {


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriterTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriterTest.java
index 6d53c4087ea..7edb1eed516 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriterTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriterTest.java
@@ -280,6 +280,38 @@ public void writeFrameHasPayload() throws Exception {
         assertEquals(expectedOutbound, outbound);
     }
 
+    @Test
+    public void writePriority() {
+        frameWriter.writePriority(
+            ctx, /* streamId= */ 1, /* dependencyId= */ 2, /* weight= */ (short) 256, /* exclusive= */ true, promise);
+
+        expectedOutbound = Unpooled.copiedBuffer(new byte[] {
+                (byte) 0x00, (byte) 0x00, (byte) 0x05, // payload length = 5
+                (byte) 0x02, // payload type = 2
+                (byte) 0x00, // flags = 0x00
+                (byte) 0x00, (byte) 0x00, (byte) 0x00, (byte) 0x01, // stream id = 1
+                (byte) 0x80, (byte) 0x00, (byte) 0x00, (byte) 0x02, // dependency id = 2 | exclusive = 1 << 63
+                (byte) 0xFF, // weight = 255 (implicit +1)
+        });
+        assertEquals(expectedOutbound, outbound);
+    }
+
+    @Test
+    public void writePriorityDefaults() {
+        frameWriter.writePriority(
+            ctx, /* streamId= */ 1, /* dependencyId= */ 0, /* weight= */ (short) 16, /* exclusive= */ false, promise);
+
+        expectedOutbound = Unpooled.copiedBuffer(new byte[] {
+                (byte) 0x00, (byte) 0x00, (byte) 0x05, // payload length = 5
+                (byte) 0x02, // payload type = 2
+                (byte) 0x00, // flags = 0x00
+                (byte) 0x00, (byte) 0x00, (byte) 0x00, (byte) 0x01, // stream id = 1
+                (byte) 0x00, (byte) 0x00, (byte) 0x00, (byte) 0x00, // dependency id = 0 | exclusive = 0 << 63
+                (byte) 0x0F, // weight = 15 (implicit +1)
+        });
+        assertEquals(expectedOutbound, outbound);
+    }
+
     private byte[] headerPayload(int streamId, Http2Headers headers, byte padding) throws Http2Exception, IOException {
         if (padding == 0) {
             return headerPayload(streamId, headers);