fix too many flush #13244

icodening · 2023-02-26T06:58:37Z

Motivation:
fix too many flush.
when I call ctx.write to send end frame, it will be auto flush.

Modification:
check StreamChannel state before flush.
When stream is closed, it may not need auto flush

Result:
reduce the number of flush

Fixes #13232

normanmaurer · 2023-02-28T08:39:56Z

A unit test would be nice

icodening · 2023-03-02T09:45:20Z

In the current version，AbstractHttp2StreamChannel.doBeginRead will auto flush when write end header frame.

@Test
public void closeStreamWithoutFlush() {
    Http2StreamChannel childChannel3 = newInboundStream(3, true, new LastInboundHandler());
    Http2StreamChannel childChannel5 = newInboundStream(5, true, new LastInboundHandler());
    FlushSniffer flushSniffer = new FlushSniffer();
    parentChannel.pipeline().addFirst(flushSniffer);

    //1. write data and batch flush
    byte[] dataBuf = "test".getBytes();
    childChannel5.write(new DefaultHttp2DataFrame(Unpooled.wrappedBuffer(dataBuf), false));
    childChannel3.write(new DefaultHttp2DataFrame(Unpooled.wrappedBuffer(dataBuf), false));
    childChannel3.flush();
    assertTrue(flushSniffer.checkFlush());

    //2. write end header frame
    Http2Headers headers = new DefaultHttp2Headers();
    headers.set("test", "test");
    childChannel3.write(new DefaultHttp2HeadersFrame(headers, true));
    childChannel5.write(new DefaultHttp2HeadersFrame(headers, true));

    assertFalse(flushSniffer.checkFlush()); // fails. write end frame will auto 'flush'
    //batch flush
    childChannel5.flush();
    assertTrue(flushSniffer.checkFlush());
}

normanmaurer · 2023-03-02T14:17:33Z

@icodening hmm I am not convinced this change is "safe". If we not flush we could miss to update the connection related flow control.

normanmaurer · 2023-03-02T15:01:02Z

@icodening I did make some small changes which I think are more correct and safe.

normanmaurer · 2023-03-02T15:01:20Z

@bryce-anderson you might want to check this as well

icodening · 2023-03-03T03:32:57Z

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

@@ -838,6 +842,9 @@ private void updateLocalWindowIfNeeded() {
                int bytes = flowControlledBytes;
                flowControlledBytes = 0;
                ChannelFuture future = write0(parentContext(), new DefaultHttp2WindowUpdateFrame(bytes).stream(stream));
+
+                windowUpdateFrameWritten = true;


io.netty.handler.codec.http2.Http2FrameCodec#consumeBytes may return false, but windowUpdateFrameWritten is always true, this will cause unnecessary flush

There might be some more improvements we can make but I would say we can do another PR for this.

General question not related to this PR: if we write window_update frame to the parentContext() can we rely on the parent channel to manage flushes instead of triggering flush from the stream channel?

normanmaurer · 2023-03-03T07:45:52Z

@icodening one question... Why are you concerned by flushes at all ? If there is nothing to write it should be relative cheap.

icodening · 2023-03-03T09:21:55Z

@icodening one question... Why are you concerned by flushes at all ? If there is nothing to write it should be relative cheap.

I want to batch flush, when I call ctx.flush once, it will actually call flush multiple times.

The following code is from io.netty.handler.codec.http2.DefaultHttp2RemoteFlowController.FlowState#writeAllocatedBytes, it seems that flush is called once for each end stream.

while (!cancelled && (frame = peek()) != null) {
    int maxBytes = min(allocated, writableWindow());
    if (maxBytes <= 0 && frame.size() > 0) {
        // The frame still has data, but the amount of allocated bytes has been exhausted.
        // Don't write needless empty frames.
        break;
    }
    writeOccurred = true;
    int initialFrameSize = frame.size();
    try {
        frame.write(ctx, max(0, maxBytes));
        if (frame.size() == 0) {
            // This frame has been fully written, remove this frame and notify it.
            // Since we remove this frame first, we're guaranteed that its error
            // method will not be called when we call cancel.
            pendingWriteQueue.remove();
            frame.writeComplete();
        }
    } finally {
        // Decrement allocated by how much was actually written.
        allocated -= initialFrameSize - frame.size();
    }
}

icodening · 2023-03-03T14:49:25Z

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

@@ -806,7 +808,9 @@ void doBeginRead() {
                    }
                    // We need to double check that there is nothing left to flush such as a
                    // window update frame.
-                    flush();
+                    if (windowUpdateFrameWritten) {
+                        flush();


if stream state is CLOSE, it should not need to be flush, right?

if (!Http2Stream.State.CLOSED.equals(stream.state())) { flush(); }

well I think it might still need to do to ensure connection window updates are flushed ?

correct, spec allows writing window_update frame after endStream=true

@normanmaurer for ST this change won't make any difference because we manage flushes externally. But I can imagine that there is a risk to break users who rely on the current behavior (flush before closing the stream). Maybe it's worth adding an opt-in property to control this change.

I think this checking of stream state is sufficient. The flow controller does at least profess to free any remaining bytes on stream closure and thus ignores any more stream flow window updates here.

idelpivnitskiy · 2023-03-03T20:08:53Z

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

@@ -838,6 +842,9 @@ private void updateLocalWindowIfNeeded() {
                int bytes = flowControlledBytes;
                flowControlledBytes = 0;
                ChannelFuture future = write0(parentContext(), new DefaultHttp2WindowUpdateFrame(bytes).stream(stream));
+
+                windowUpdateFrameWritten = true;


General question not related to this PR: if we write window_update frame to the parentContext() can we rely on the parent channel to manage flushes instead of triggering flush from the stream channel?

idelpivnitskiy · 2023-03-03T20:26:48Z

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

@@ -806,7 +808,9 @@ void doBeginRead() {
                    }
                    // We need to double check that there is nothing left to flush such as a
                    // window update frame.
-                    flush();
+                    if (windowUpdateFrameWritten) {
+                        flush();


correct, spec allows writing window_update frame after endStream=true

idelpivnitskiy · 2023-03-03T20:57:33Z

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

+        //do not auto flush, it will be 'false'
+        assertFalse(flushSniffer.checkFlush());
+        //batch flush
+        childChannel5.flush();


To clarify: the flush automatically invoked right now only if the stream is closed, meaning that both inbound and outbound side signaled endStream=true. If the inbound side is still open, you won't see the flush.

From StreamChannel point of view, it's not active anymore if the stream is closed. There will be channelInactive() event sent as part of processing write(endStream=true) (line 1388), and the closeFuture() will be notified. There is nothing that users can write after that point.

@normanmaurer it sounds like it's incorrect to invoke childChannel5.flush() here, because childChannel5.isActive() returns false at this point. However, there is no other easy way for users of StreamChannel API to flush after response is written except using writeAndFlush, which complicates usability. In ST, we also invoke flush() after the last write(...) because we have a separate logic to control flushes.

How does it work for the main connection Channel (let's say HTTP/1.1)? If someone writes and closes the channel, I would expect Netty to flush everything before closing. Seems reasonable for StreamChannel to behave consistently with real Channel, at least by default.

Also, multiplexed streams run in parallel if there is > 1 in-flight requests. It complicates how "batch flush" can be implemented if writes from multiple streams intersect with each other.

I want to batch flush, when I call ctx.flush once, it will actually call flush multiple times.

@icodening taking the above description into account, can you clarify the exact flow of how you plan to "batch flush" on a closed stream?

Btw, have you considered adding FlushConsolidationHandler between Http2ConnectionHandler and Http2MultiplexHandler?

Btw, have you considered adding FlushConsolidationHandler between Http2ConnectionHandler and Http2MultiplexHandler?

I tried, this way can reduce flush. thks

@icodening taking the above description into account, can you clarify the exact flow of how you plan to "batch flush" on a closed stream?

I will write the message to the queue and schedule the eventloop to execute.
ctx.write will be called when the number of messages is less than the threshold, and ctx.flush will be called when the number of messages is more than the threshold.

@normanmaurer it sounds like it's incorrect to invoke childChannel5.flush() here, because childChannel5.isActive() returns false at this point. However, there is no other easy way for users of StreamChannel API to flush after response is written except using writeAndFlush, which complicates usability. In ST, we also invoke flush() after the last write(...) because we have a separate logic to control flushes.

It seems more reasonable to use the parent channel flush

idelpivnitskiy · 2023-03-03T21:05:42Z

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

@@ -806,7 +808,9 @@ void doBeginRead() {
                    }
                    // We need to double check that there is nothing left to flush such as a
                    // window update frame.
-                    flush();
+                    if (windowUpdateFrameWritten) {
+                        flush();


@normanmaurer for ST this change won't make any difference because we manage flushes externally. But I can imagine that there is a risk to break users who rely on the current behavior (flush before closing the stream). Maybe it's worth adding an opt-in property to control this change.

bryce-anderson

The modifications look safe to me, but could use a few more tests.

It's been a while since I've looked at this code, but it's not clear to me that it's necessary to flush the flow control frames, otherwise flow control for data buffered when a stream is closed would appear to be broken. Specifically, we only write an explicit flow control frame via updateLocalWindowIfNeeded(), which is only called at the beginning of the HttpChannelUnsafe.beginRead(). However, when a stream is closed it attempts to drain its inbound buffer but does so by going directly to Unsafe.doBeginRead(), skipping the generation of flow control messages for anything that is still in the inboundBuffer. Since those may be data frames and it appears we wouldn't send flow control for them on a stream close they must be accounted for elsewhere, which looks like in the sessions flow controller when the stream is removed here, which includes a flush.
That said, it's been a while so don't be surprised if this isn't an accurate assessment of things. 😅

bryce-anderson · 2023-03-03T19:03:15Z

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

+        Http2StreamChannel childChannel3 = newInboundStream(3, true, new LastInboundHandler());
+        Http2StreamChannel childChannel5 = newInboundStream(5, true, new LastInboundHandler());


It would be good to ensure that we do flush any window update frames. Can we add a third stream that tests that, or if you prefer, another test?

bryce-anderson · 2023-03-03T19:10:45Z

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

@@ -1037,6 +1044,7 @@ public void flush() {
            // We need to set this to false before we call flush0(...) as ChannelFutureListener may produce more data
            // that are explicit flushed.
            writeDoneAndNoFlush = false;
+            windowUpdateFrameWritten = false;


There are strong parallels with the writeDoneAndNoFlush field. It would be good to align the names and make clear the distinction, which is only that writeDoneAndNoFlush includes all frame types (including window updates) whereas windowUpdateFrameWritten is only for window update frames.

He-Pin · 2023-09-13T11:37:16Z

@icodening Anyupdate on this?

icodening · 2023-09-13T13:42:20Z

@icodening Anyupdate on this?

No updates yet

fix too many flush

b8c9760

add unit test

b50949b

Keep track of window update frames produces

6c6e692

normanmaurer requested a review from bryce-anderson March 2, 2023 15:01

icodening commented Mar 3, 2023

View reviewed changes

normanmaurer requested a review from idelpivnitskiy March 3, 2023 11:59

normanmaurer added this to the 4.1.90.Final milestone Mar 3, 2023

icodening commented Mar 3, 2023

View reviewed changes

idelpivnitskiy reviewed Mar 3, 2023

View reviewed changes

bryce-anderson reviewed Mar 3, 2023

View reviewed changes

normanmaurer modified the milestones: 4.1.90.Final, 4.1.91.Final Mar 14, 2023

normanmaurer modified the milestones: 4.1.91.Final, 4.1.92.Final Apr 3, 2023

normanmaurer modified the milestones: 4.1.92.Final, 4.1.93.Final Apr 25, 2023

normanmaurer modified the milestones: 4.1.93.Final, 4.1.94.Final May 24, 2023

normanmaurer modified the milestones: 4.1.94.Final, 4.1.95.Final Jun 19, 2023

normanmaurer modified the milestones: 4.1.95.Final, 4.1.96.Final Jul 20, 2023

normanmaurer added this to the 4.1.97.Final milestone Jul 27, 2023

normanmaurer modified the milestones: 4.1.97.Final, 4.1.98.Final Aug 23, 2023

normanmaurer modified the milestones: 4.1.98.Final, 4.1.99.Final Sep 21, 2023

normanmaurer modified the milestones: 4.1.99.Final, 4.1.100.Final Sep 28, 2023

normanmaurer modified the milestones: 4.1.100.Final, 4.1.101.Final Oct 10, 2023

normanmaurer modified the milestones: 4.1.101.Final, 4.1.102.Final Nov 9, 2023

normanmaurer modified the milestones: 4.1.102.Final, 4.1.104.Final, 4.1.105.Final Dec 13, 2023

normanmaurer modified the milestones: 4.1.105.Final, 4.1.106.Final, 4.1.107.Final Jan 16, 2024

normanmaurer modified the milestones: 4.1.107.Final, 4.1.108.Final Feb 12, 2024

normanmaurer modified the milestones: 4.1.108.Final, 4.1.109.Final Mar 21, 2024

normanmaurer modified the milestones: 4.1.109.Final, 4.1.110.Final Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix too many flush #13244

fix too many flush #13244

icodening commented Feb 26, 2023

normanmaurer commented Feb 28, 2023

icodening commented Mar 2, 2023 •

edited

normanmaurer commented Mar 2, 2023

normanmaurer commented Mar 2, 2023

normanmaurer commented Mar 2, 2023

icodening Mar 3, 2023 •

edited

normanmaurer Mar 3, 2023

idelpivnitskiy Mar 3, 2023

normanmaurer commented Mar 3, 2023

icodening commented Mar 3, 2023

icodening Mar 3, 2023

normanmaurer Mar 3, 2023

idelpivnitskiy Mar 3, 2023

idelpivnitskiy Mar 3, 2023

bryce-anderson Mar 6, 2023

idelpivnitskiy Mar 3, 2023

idelpivnitskiy Mar 3, 2023

idelpivnitskiy Mar 3, 2023

idelpivnitskiy Mar 3, 2023

icodening Mar 5, 2023

icodening Mar 5, 2023

icodening Mar 5, 2023

idelpivnitskiy Mar 3, 2023

bryce-anderson left a comment

bryce-anderson Mar 3, 2023

bryce-anderson Mar 3, 2023

He-Pin commented Sep 13, 2023

icodening commented Sep 13, 2023

		Http2StreamChannel childChannel3 = newInboundStream(3, true, new LastInboundHandler());
		Http2StreamChannel childChannel5 = newInboundStream(5, true, new LastInboundHandler());

fix too many flush #13244

Are you sure you want to change the base?

fix too many flush #13244

Conversation

icodening commented Feb 26, 2023

normanmaurer commented Feb 28, 2023

icodening commented Mar 2, 2023 • edited

normanmaurer commented Mar 2, 2023

normanmaurer commented Mar 2, 2023

normanmaurer commented Mar 2, 2023

icodening Mar 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Mar 3, 2023

icodening commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryce-anderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

He-Pin commented Sep 13, 2023

icodening commented Sep 13, 2023

icodening commented Mar 2, 2023 •

edited

icodening Mar 3, 2023 •

edited