HTTP/2 UniformStreamByteDistributor stream window ignored

[Scottmitch]

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 #4538 I developed the following test case to demonstrate the issue:

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;
+            }
         });
     }

[Scottmitch]

@nmittler - FYI

[nmittler]

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

[Scottmitch]

@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?

[nmittler]

@Scottmitch sgtm, thanks!

[Scottmitch]

pr pending ...