Repository: netty/netty
Issue: HTTP/2 RemoteFlowController$Listener.streamWritten
Labels: cleanup

We should evaluate whether [Http2RemoteFlowController$Listener.streamWritten](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2RemoteFlowController.java#L151) is necessary. If not we should remove it.


Issue comments:
@louiscryan @nmittler @ejona86 - I checked in gRPC and it looks like this is still not being used? Is the plan still to use this or eventually consume the stream writability changes?

So far it has not proven critical to performance and there are other perf improvements I would prioritize in GRPC above the use-case for this, particularly with the improvements in flow-control write allocations and frame merging.

Im OK with getting rid of this

after we get PR https://github.com/netty/netty/pull/4599 pulled in I'll submit a PR for this (I have one pending)

Thanks Scott

On Fri, Dec 18, 2015 at 1:27 PM, Scott Mitchell notifications@github.com
wrote:

> after we get PR #4599 https://github.com/netty/netty/pull/4599 pulled
> in I'll submit a PR for this.
> 
> —
> Reply to this email directly or view it on GitHub
> https://github.com/netty/netty/issues/4600#issuecomment-165901318.


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java
index 2db3e708652..c99f86d6278 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java
@@ -205,7 +205,7 @@ private boolean isChannelWritable0() {
 
     @Override
     public void listener(Listener listener) {
-        monitor = listener == null ? new DefaultWritabilityMonitor() : new ListenerWritabilityMonitor(listener);
+        monitor = listener == null ? new WritabilityMonitor() : new ListenerWritabilityMonitor(listener);
     }
 
     @Override
@@ -627,13 +627,14 @@ final void markWritability(boolean isWritable) {
     /**
      * Abstract class which provides common functionality for writability monitor implementations.
      */
-    private abstract class WritabilityMonitor {
+    private class WritabilityMonitor {
         private long totalPendingBytes;
-        private final Writer writer;
-
-        protected WritabilityMonitor(Writer writer) {
-            this.writer = writer;
-        }
+        private final Writer writer = new StreamByteDistributor.Writer() {
+            @Override
+            public void write(Http2Stream stream, int numBytes) {
+                state(stream).writeAllocatedBytes(numBytes);
+            }
+        };
 
         /**
          * Called when the writability of the underlying channel changes.
@@ -728,20 +729,6 @@ protected final boolean isWritableConnection() {
         }
     }
 
-    /**
-     * Provides no notification or tracking of writablity changes.
-     */
-    private final class DefaultWritabilityMonitor extends WritabilityMonitor {
-        DefaultWritabilityMonitor() {
-            super(new StreamByteDistributor.Writer() {
-                @Override
-                public void write(Http2Stream stream, int numBytes) {
-                    state(stream).writeAllocatedBytes(numBytes);
-                }
-            });
-        }
-    }
-
     /**
      * Writability of a {@code stream} is calculated using the following:
      * <pre>
@@ -763,17 +750,7 @@ public boolean visit(Http2Stream stream) throws Http2Exception {
             }
         };
 
-        ListenerWritabilityMonitor(final Listener listener) {
-            super(new StreamByteDistributor.Writer() {
-                @Override
-                public void write(Http2Stream stream, int numBytes) {
-                    AbstractState state = state(stream);
-                    int written = state.writeAllocatedBytes(numBytes);
-                    if (written != -1) {
-                        listener.streamWritten(state.stream(), written);
-                    }
-                }
-            });
+        ListenerWritabilityMonitor(Listener listener) {
             this.listener = listener;
         }
 
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2RemoteFlowController.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2RemoteFlowController.java
index 279129c3632..83fe96da842 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2RemoteFlowController.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2RemoteFlowController.java
@@ -138,18 +138,6 @@ interface FlowControlled {
      * Listener to the number of flow-controlled bytes written per stream.
      */
     interface Listener {
-
-        /**
-         * Report the number of {@code writtenBytes} for a {@code stream}. Called after the
-         * flow-controller has flushed bytes for the given stream.
-         * <p>
-         * This method should not throw. Any thrown exceptions are considered a programming error and are ignored.
-         * @param stream that had bytes written.
-         * @param writtenBytes the number of bytes written for a stream, can be 0 in the case of an
-         *                     empty DATA frame.
-         */
-        void streamWritten(Http2Stream stream, int writtenBytes);
-
         /**
          * Notification that {@link Http2RemoteFlowController#isWritable(Http2Stream)} has changed for {@code stream}.
          * <p>


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowControllerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowControllerTest.java
index c98dfd0b7d1..58742311dd2 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowControllerTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowControllerTest.java
@@ -170,7 +170,6 @@ public void payloadSmallerThanWindowShouldBeWrittenImmediately() throws Http2Exc
         verifyZeroInteractions(listener);
         controller.writePendingBytes();
         data.assertFullyWritten();
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 5);
         verifyZeroInteractions(listener);
     }
 
@@ -181,7 +180,6 @@ public void emptyPayloadShouldBeWrittenImmediately() throws Http2Exception {
         data.assertNotWritten();
         controller.writePendingBytes();
         data.assertFullyWritten();
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 0);
         verifyZeroInteractions(listener);
     }
 
@@ -210,7 +208,6 @@ public void payloadsShouldMerge() throws Http2Exception {
         controller.writePendingBytes();
         data1.assertFullyWritten();
         data2.assertNotWritten();
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 15);
         verify(listener, times(1)).writabilityChanged(stream(STREAM_A));
         assertFalse(controller.isWritable(stream(STREAM_A)));
     }
@@ -267,7 +264,6 @@ public void payloadLargerThanWindowShouldWritePartial() throws Http2Exception {
         controller.writePendingBytes();
         // Verify that a partial frame of 5 remains to be sent
         data.assertPartiallyWritten(5);
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 5);
         verify(listener, times(1)).writabilityChanged(stream(STREAM_A));
         assertFalse(controller.isWritable(stream(STREAM_A)));
         verifyNoMoreInteractions(listener);
@@ -286,7 +282,6 @@ public void windowUpdateAndFlushShouldTriggerWrite() throws Http2Exception {
         controller.writePendingBytes();
         data.assertPartiallyWritten(10);
         moreData.assertNotWritten();
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 10);
         verify(listener, times(1)).writabilityChanged(stream(STREAM_A));
         assertFalse(controller.isWritable(stream(STREAM_A)));
         reset(listener);
@@ -302,7 +297,6 @@ public void windowUpdateAndFlushShouldTriggerWrite() throws Http2Exception {
 
         data.assertFullyWritten();
         moreData.assertPartiallyWritten(5);
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 15);
         verify(listener, never()).writabilityChanged(stream(STREAM_A));
         assertFalse(controller.isWritable(stream(STREAM_A)));
 
@@ -331,7 +325,6 @@ public void initialWindowUpdateShouldSendPayload() throws Http2Exception {
         // Verify that the entire frame was sent.
         controller.initialWindowSize(10);
         data.assertFullyWritten();
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 10);
         assertWritabilityChanged(0, false);
     }
 
@@ -356,7 +349,6 @@ public void successiveSendsShouldNotInteract() throws Http2Exception {
         dataA.assertPartiallyWritten(8);
         assertEquals(65527, window(STREAM_A));
         assertEquals(0, window(CONNECTION_STREAM_ID));
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 8);
         assertWritabilityChanged(0, false);
         reset(listener);
 
@@ -376,11 +368,9 @@ public void successiveSendsShouldNotInteract() throws Http2Exception {
         // Verify the rest of A is written.
         dataA.assertFullyWritten();
         assertEquals(65525, window(STREAM_A));
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 2);
 
         dataB.assertFullyWritten();
         assertEquals(65525, window(STREAM_B));
-        verify(listener, times(1)).streamWritten(stream(STREAM_B), 10);
         verifyNoMoreInteractions(listener);
     }
 
@@ -399,7 +389,6 @@ public void negativeWindowShouldNotThrowException() throws Http2Exception {
         sendData(STREAM_A, data1);
         controller.writePendingBytes();
         data1.assertFullyWritten();
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 20);
         assertTrue(window(CONNECTION_STREAM_ID) > 0);
         verify(listener, times(1)).writabilityChanged(stream(STREAM_A));
         verify(listener, never()).writabilityChanged(stream(STREAM_B));
@@ -513,8 +502,6 @@ public void initialWindowUpdateShouldSendEmptyFrame() throws Http2Exception {
 
         data.assertFullyWritten();
         data2.assertFullyWritten();
-
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 10);
     }
 
     @Test
@@ -540,7 +527,6 @@ public void initialWindowUpdateShouldSendPartialFrame() throws Http2Exception {
         assertTrue(controller.isWritable(stream(STREAM_D)));
 
         data.assertPartiallyWritten(5);
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 5);
     }
 
     @Test
@@ -565,7 +551,6 @@ public void connectionWindowUpdateShouldSendFrame() throws Http2Exception {
 
         controller.writePendingBytes();
         data.assertFullyWritten();
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 10);
         assertWritabilityChanged(0, false);
         assertEquals(0, window(CONNECTION_STREAM_ID));
         assertEquals(DEFAULT_WINDOW_SIZE - 10, window(STREAM_A));
@@ -594,7 +579,6 @@ public void connectionWindowUpdateShouldSendPartialFrame() throws Http2Exception
 
         controller.writePendingBytes();
         data.assertPartiallyWritten(5);
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 5);
         assertWritabilityChanged(0, false);
         assertEquals(0, window(CONNECTION_STREAM_ID));
         assertEquals(DEFAULT_WINDOW_SIZE - 5, window(STREAM_A));
@@ -637,7 +621,6 @@ public void streamWindowUpdateShouldSendFrame() throws Http2Exception {
         data.assertNotWritten();
         controller.writePendingBytes();
         data.assertFullyWritten();
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 10);
         verify(listener, never()).writabilityChanged(stream(STREAM_A));
         verify(listener, never()).writabilityChanged(stream(STREAM_B));
         verify(listener, never()).writabilityChanged(stream(STREAM_C));
@@ -687,7 +670,6 @@ public void streamWindowUpdateShouldSendPartialFrame() throws Http2Exception {
         data.assertNotWritten();
         controller.writePendingBytes();
         data.assertPartiallyWritten(5);
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 5);
         assertEquals(DEFAULT_WINDOW_SIZE - 5, window(CONNECTION_STREAM_ID));
         assertEquals(0, window(STREAM_A));
         assertEquals(DEFAULT_WINDOW_SIZE, window(STREAM_B));
@@ -717,7 +699,6 @@ public Void answer(InvocationOnMock invocationOnMock) {
         verify(flowControlled, never()).writeComplete();
 
         assertEquals(90, windowBefore - window(STREAM_A));
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 90);
         assertWritabilityChanged(0, true);
     }
 
@@ -794,7 +775,6 @@ public Void answer(InvocationOnMock invocationOnMock) {
         verify(flowControlled).writeComplete();
 
         assertEquals(150, windowBefore - window(STREAM_A));
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 150);
         assertWritabilityChanged(0, true);
     }
 
@@ -820,7 +800,6 @@ public Void answer(InvocationOnMock invocationOnMock) {
         verify(flowControlled).write(any(ChannelHandlerContext.class), anyInt());
         verify(flowControlled).error(any(ChannelHandlerContext.class), any(Throwable.class));
         verify(flowControlled, never()).writeComplete();
-        verify(listener, times(1)).streamWritten(stream(STREAM_A), 0);
         verify(listener, times(1)).writabilityChanged(stream(STREAM_A));
         verify(listener, never()).writabilityChanged(stream(STREAM_B));
         verify(listener, never()).writabilityChanged(stream(STREAM_C));