HTTP/2 StreamByteDistributor improve parameter validation

Motivation:
Each StreamByteDistributor may allow for priority in different ways, but there are certain characteristics which are invalid regardless of the distribution algorithm. We should validate these invalid characteristics at the flow controller level.

Modifications:
- Disallow negative stream IDs from being used. These streams may be accepted by the WeightedFairQueueByteDistributor and cause state for other valid streams to be evicted.
- Improve unit tests to verify limits are enforced.

Result:
Boundary conditions related to the priority parameters are validated more strictly.

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java

@@ -25,6 +25,8 @@ import java.util.ArrayDeque;
 import java.util.Deque;
 
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_WINDOW_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_WEIGHT;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_WEIGHT;
 import static io.netty.handler.codec.http2.Http2Error.FLOW_CONTROL_ERROR;
 import static io.netty.handler.codec.http2.Http2Error.INTERNAL_ERROR;
 import static io.netty.handler.codec.http2.Http2Exception.streamError;
@@ -178,6 +180,11 @@ public class DefaultHttp2RemoteFlowController implements Http2RemoteFlowControll
 
     @Override
     public void updateDependencyTree(int childStreamId, int parentStreamId, short weight, boolean exclusive) {
+        // It is assumed there are all validated at a higher level. For example in the Http2FrameReader.
+        assert weight >= MIN_WEIGHT && weight <= MAX_WEIGHT : "Invalid weight";
+        assert childStreamId != parentStreamId : "A stream cannot depend on itself";
+        assert childStreamId > 0 && parentStreamId >= 0 : "childStreamId must be > 0. parentStreamId must be >= 0.";
+
         streamByteDistributor.updateDependencyTree(childStreamId, parentStreamId, weight, exclusive);
     }
 

codec-http2/src/main/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributor.java

@@ -33,8 +33,6 @@ import java.util.List;
 import static io.netty.handler.codec.http2.Http2CodecUtil.CONNECTION_STREAM_ID;
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_MIN_ALLOCATION_CHUNK;
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT;
-import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_WEIGHT;
-import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_WEIGHT;
 import static io.netty.handler.codec.http2.Http2CodecUtil.streamableBytes;
 import static io.netty.handler.codec.http2.Http2Error.INTERNAL_ERROR;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
@@ -197,14 +195,6 @@ public final class WeightedFairQueueByteDistributor implements StreamByteDistrib
 
     @Override
     public void updateDependencyTree(int childStreamId, int parentStreamId, short weight, boolean exclusive) {
-        if (weight < MIN_WEIGHT || weight > MAX_WEIGHT) {
-            throw new IllegalArgumentException(String.format(
-                    "Invalid weight: %d. Must be between %d and %d (inclusive).", weight, MIN_WEIGHT, MAX_WEIGHT));
-        }
-        if (childStreamId == parentStreamId) {
-            throw new IllegalArgumentException("A stream cannot depend on itself");
-        }
-
         State state = state(childStreamId);
         if (state == null) {
             // If there is no State object that means there is no Http2Stream object and we would have to keep the

codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowControllerTest.java

@@ -33,6 +33,8 @@ import java.util.concurrent.atomic.AtomicInteger;
 import static io.netty.handler.codec.http2.Http2CodecUtil.CONNECTION_STREAM_ID;
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT;
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_WINDOW_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_WEIGHT;
+import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_WEIGHT;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -906,6 +908,36 @@ public abstract class DefaultHttp2RemoteFlowControllerTest {
         dataA.assertFullyWritten();
     }
 
+    @Test(expected = AssertionError.class)
+    public void invalidParentStreamIdThrows() {
+        controller.updateDependencyTree(STREAM_D, -1, DEFAULT_PRIORITY_WEIGHT, true);
+    }
+
+    @Test(expected = AssertionError.class)
+    public void invalidChildStreamIdThrows() {
+        controller.updateDependencyTree(-1, STREAM_D, DEFAULT_PRIORITY_WEIGHT, true);
+    }
+
+    @Test(expected = AssertionError.class)
+    public void connectionChildStreamIdThrows() {
+        controller.updateDependencyTree(0, STREAM_D, DEFAULT_PRIORITY_WEIGHT, true);
+    }
+
+    @Test(expected = AssertionError.class)
+    public void invalidWeightTooSmallThrows() {
+        controller.updateDependencyTree(STREAM_A, STREAM_D, (short) (MIN_WEIGHT - 1), true);
+    }
+
+    @Test(expected = AssertionError.class)
+    public void invalidWeightTooBigThrows() {
+        controller.updateDependencyTree(STREAM_A, STREAM_D, (short) (MAX_WEIGHT + 1), true);
+    }
+
+    @Test(expected = AssertionError.class)
+    public void dependencyOnSelfThrows() {
+        controller.updateDependencyTree(STREAM_A, STREAM_A, DEFAULT_PRIORITY_WEIGHT, true);
+    }
+
     private void assertWritabilityChanged(int amt, boolean writable) {
         verify(listener, times(amt)).writabilityChanged(stream(STREAM_A));
         verify(listener, times(amt)).writabilityChanged(stream(STREAM_B));

codec-http2/src/test/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributorDependencyTreeTest.java

@@ -31,6 +31,17 @@ import static org.mockito.Mockito.doAnswer;
 
 public class WeightedFairQueueByteDistributorDependencyTreeTest extends
                                                 AbstractWeightedFairQueueByteDistributorDependencyTest {
+    private static final int leadersId = 3; // js, css
+    private static final int unblockedId = 5;
+    private static final int backgroundId = 7;
+    private static final int speculativeId = 9;
+    private static final int followersId = 11; // images
+    private static final short leadersWeight = 201;
+    private static final short unblockedWeight = 101;
+    private static final short backgroundWeight = 1;
+    private static final short speculativeWeight = 1;
+    private static final short followersWeight = 1;
+
     @Before
     public void setup() throws Http2Exception {
         MockitoAnnotations.initMocks(this);
@@ -162,17 +173,6 @@ public class WeightedFairQueueByteDistributorDependencyTreeTest extends
         assertEquals(0, distributor.numChildren(3));
     }
 
-    private static final int leadersId = 3; // js, css
-    private static final int unblockedId = 5;
-    private static final int backgroundId = 7;
-    private static final int speculativeId = 9;
-    private static final int followersId = 11; // images
-    private static final short leadersWeight = 201;
-    private static final short unblockedWeight = 101;
-    private static final short backgroundWeight = 1;
-    private static final short speculativeWeight = 1;
-    private static final short followersWeight = 1;
-
     @Test
     public void fireFoxQoSStreamsRemainAfterDataStreamsAreClosed() throws Http2Exception {
         // http://bitsup.blogspot.com/2015/01/http2-dependency-priorities-in-firefox.html

codec-http2/src/test/java/io/netty/handler/codec/http2/WeightedFairQueueByteDistributorTest.java

@@ -28,14 +28,14 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.anyInt;
-import static org.mockito.Mockito.eq;
-import static org.mockito.Mockito.same;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.atMost;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.same;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
@@ -51,13 +51,18 @@ public class WeightedFairQueueByteDistributorTest extends AbstractWeightedFairQu
     public void setup() throws Http2Exception {
         MockitoAnnotations.initMocks(this);
 
-        connection = new DefaultHttp2Connection(false);
-        distributor = new WeightedFairQueueByteDistributor(connection);
-        distributor.allocationQuantum(ALLOCATION_QUANTUM);
-
         // Assume we always write all the allocated bytes.
         doAnswer(writeAnswer(false)).when(writer).write(any(Http2Stream.class), anyInt());
 
+        setup(-1);
+    }
+
+    private void setup(int maxStateOnlySize) throws Http2Exception {
+        connection = new DefaultHttp2Connection(false);
+        distributor = maxStateOnlySize >= 0 ? new WeightedFairQueueByteDistributor(connection, maxStateOnlySize)
+                                            : new WeightedFairQueueByteDistributor(connection);
+        distributor.allocationQuantum(ALLOCATION_QUANTUM);
+
         connection.local().createStream(STREAM_A, false);
         connection.local().createStream(STREAM_B, false);
         Http2Stream streamC = connection.local().createStream(STREAM_C, false);
@@ -898,6 +903,16 @@ public class WeightedFairQueueByteDistributorTest extends AbstractWeightedFairQu
         assertEquals(700, captureWrites(STREAM_D));
     }
 
+    @Test
+    public void activeStreamDependentOnNewNonActiveStreamGetsQuantum() throws Http2Exception {
+        setup(0);
+        updateStream(STREAM_D, 700, true);
+        setPriority(STREAM_D, STREAM_E, DEFAULT_PRIORITY_WEIGHT, true);
+
+        assertFalse(write(700));
+        assertEquals(700, captureWrites(STREAM_D));
+    }
+
     private boolean write(int numBytes) throws Http2Exception {
         return distributor.distribute(numBytes, writer);
     }