Repository: netty/netty
Issue: HTTP/2 UniformStreamByteDistributor stream window ignored
Labels: defect

UniformStreamByteDistributor will write to a stream even if the stream's flow control window is negative. This is not allowed by the rfc.

https://tools.ietf.org/html/rfc7540#section-6.9.2

> A change to SETTINGS_INITIAL_WINDOW_SIZE can cause the available
>    space in a flow-control window to become negative.  A sender MUST
>    track the negative flow-control window and MUST NOT send new flow-
>    controlled frames until it receives WINDOW_UPDATE frames that cause
>    the flow-control window to become positive.

I guess the spec also means that you should not send new flow control frames until the flow-control window becomes non-negative? If the flow-control window is 0 and we have empty frames I don't think it should be a problem to send these.

While working on PR https://github.com/netty/netty/pull/4538 I developed the following test case to demonstrate the issue:

``` diff
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/UniformStreamByteDistributorTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/UniformStreamByteDistributorTest.java
index 44010e4..c52b678 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/UniformStreamByteDistributorTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/UniformStreamByteDistributorTest.java
@@ -185,11 +185,29 @@ public class UniformStreamByteDistributorTest {
         verifyNoMoreInteractions(writer);
     }

+    @Test
+    public void streamWindowExhaustedDoesNotWrite() throws Http2Exception {
+        updateStream(STREAM_A, 0, true, false);
+        updateStream(STREAM_B, 0, true);
+        updateStream(STREAM_C, 0, true);
+        updateStream(STREAM_D, 0, true, false);
+
+        assertFalse(write(10));
+        verifyWrite(STREAM_B, 0);
+        verifyWrite(STREAM_C, 0);
+        verifyNoMoreInteractions(writer);
+    }
+
     private Http2Stream stream(int streamId) {
         return connection.stream(streamId);
     }

     private void updateStream(final int streamId, final int streamableBytes, final boolean hasFrame) {
+        updateStream(streamId, streamableBytes, hasFrame, hasFrame);
+    }
+
+    private void updateStream(final int streamId, final int streamableBytes, final boolean hasFrame,
+            final boolean isWriteAllowed) {
         final Http2Stream stream = stream(streamId);
         distributor.updateStreamableBytes(new StreamByteDistributor.StreamState() {
             @Override
@@ -206,6 +224,11 @@ public class UniformStreamByteDistributorTest {
             public boolean hasFrame() {
                 return hasFrame;
             }
+
+            @Override
+            public boolean isWriteAllowed() {
+                return isWriteAllowed;
+            }
         });
     }
```


Issue comments:
@nmittler - FYI

@Scottmitch good catch! ... do you want to throw together a PR with the updated test?

@nmittler - Yes. I can handle this. However the new interface required to fix this is introduced and used in PR #4538. Would you mind if I fixed after this issue after PR #4538 is resolved?

@Scottmitch sgtm, thanks!

pr pending ...


Pull request review comments:
Maybe instead of using another variable `writeAllowed`, it might be more clear if we just use `windowSize` directly:

``` java
if (state.windowSize <= 0) {
    continue;
}
```

@nmittler  - We could do this, but boolean should be less memory, and this would be the only reason why we need `windowSize`. Would changing the variable name or adding javadocs on the variable be sufficient?

Also I think the condition would be `< 0` (instead of `<= 0`).

@Scottmitch ok, but can we name it to something more clear?  Maybe something like `windowCollapsed` or `hasWindow`?

@nmittler - `windowNegative` work for you?


Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/UniformStreamByteDistributor.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/UniformStreamByteDistributor.java
index 9b3cd2daded..565ddfaa260 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/UniformStreamByteDistributor.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/UniformStreamByteDistributor.java
@@ -97,10 +97,13 @@ public boolean distribute(int maxBytes, Writer writer) throws Http2Exception {
         State state = queue.pollFirst();
         do {
             state.enqueued = false;
-            if (state.streamableBytes > 0 && maxBytes == 0) {
-                // Stop at the first state that can't send. Add this state back to the head of
-                // the queue. Note that empty frames at the head of the queue will always be
-                // written.
+            if (state.windowNegative) {
+                continue;
+            }
+            if (maxBytes == 0 && state.streamableBytes > 0) {
+                // Stop at the first state that can't send. Add this state back to the head of the queue. Note
+                // that empty frames at the head of the queue will always be written, assuming the stream window
+                // is not negative.
                 queue.addFirst(state);
                 state.enqueued = true;
                 break;
@@ -134,6 +137,7 @@ int streamableBytes0(Http2Stream stream) {
     private final class State {
         final Http2Stream stream;
         int streamableBytes;
+        boolean windowNegative;
         boolean enqueued;
         boolean writing;
 
@@ -149,12 +153,15 @@ void updateStreamableBytes(int newStreamableBytes, boolean hasFrame, int windowS
                 streamableBytes = newStreamableBytes;
                 totalStreamableBytes += delta;
             }
-            // We should queue this state if there is a frame. We don't want to queue this frame if the window
-            // size is <= 0 and we are writing this state. The rational being we already gave this state the chance to
-            // write, and if there were empty frames the expectation is they would have been sent. At this point there
-            // must be a call to updateStreamableBytes for this state to be able to write again.
-            if (hasFrame && (!writing || windowSize > 0)) {
-                // It's not in the queue but has data to send, add it.
+            // In addition to only enqueuing state when they have frames we enforce the following restrictions:
+            // 1. If the window has gone negative. We never want to queue a state. However we also don't want to
+            //    Immediately remove the item if it is already queued because removal from dequeue is O(n). So
+            //    we allow it to stay queued and rely on the distribution loop to remove this state.
+            // 2. If the window is zero we only want to queue if we are not writing. If we are writing that means
+            //    we gave the state a chance to write zero length frames. We wait until updateStreamableBytes is
+            //    called again before this state is allowed to write.
+            windowNegative = windowSize < 0;
+            if (hasFrame && (windowSize > 0 || (windowSize == 0 && !writing))) {
                 addToQueue();
             }
         }


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/UniformStreamByteDistributorTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/UniformStreamByteDistributorTest.java
index 98ae689627d..fc358468b09 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/UniformStreamByteDistributorTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/UniformStreamByteDistributorTest.java
@@ -211,6 +211,19 @@ public void emptyFrameAtHeadIsWritten() throws Http2Exception {
         verifyNoMoreInteractions(writer);
     }
 
+    @Test
+    public void streamWindowExhaustedDoesNotWrite() throws Http2Exception {
+        updateStream(STREAM_A, 0, true, false);
+        updateStream(STREAM_B, 0, true);
+        updateStream(STREAM_C, 0, true);
+        updateStream(STREAM_D, 0, true, false);
+
+        assertFalse(write(10));
+        verifyWrite(STREAM_B, 0);
+        verifyWrite(STREAM_C, 0);
+        verifyNoMoreInteractions(writer);
+    }
+
     private Http2Stream stream(int streamId) {
         return connection.stream(streamId);
     }