AbstractHttp2StreamChannel fails to flush window update under certain read patterns

[bryce-anderson]

Expected behavior

Window updates get flushed by AbstractHttp2StreamChannel.

Actual behavior

Under certain conditions that doesn't happen.

Steps to reproduce

Minimal yet complete reproducer code (or URL to code)

I have a test case that is really ugly and should probably be condensed but it illustrates the issue. I've annotated it with values observed with a debugger during it's execution.

    static ByteBuf bb(int size) {
        ByteBuf buf = UnpooledByteBufAllocator.DEFAULT.buffer();
        for (int i = 0; i < size; i++) {
            buf.writeByte('a');
        }
        return buf;
    }


    class FlushSniffer extends ChannelOutboundHandlerAdapter {

        private boolean didFlush;

        public boolean checkFlush() {
            boolean r = didFlush;
            didFlush = false;
            return r;
        }

        @Override
        public void flush(ChannelHandlerContext ctx) throws Exception {
            didFlush = true;
            super.flush(ctx);
        }
    }

    @Test
    public void windowUpdatesAreFlushed() {
        LastInboundHandler inboundHandler = new LastInboundHandler();
        FlushSniffer flushSniffer = new FlushSniffer();
        parentChannel.pipeline().addFirst(flushSniffer);

        Http2StreamChannel childChannel = newInboundStream(3, false, inboundHandler);
        assertTrue(childChannel.config().isAutoRead());
        childChannel.config().setAutoRead(false);
        assertFalse(childChannel.config().isAutoRead());

        Http2HeadersFrame headersFrame = inboundHandler.readInbound();
        assertNotNull(headersFrame);
        // From the headers.
        assertTrue(flushSniffer.checkFlush());

        // childChannel.readState == IN_PROGRESS due to initial auto-read.
        frameInboundWriter.writeInboundData(childChannel.stream().id(), bb(1), 0, false);
        // readState == IDLE, flowControlledBytes = 1

        frameInboundWriter.writeInboundData(childChannel.stream().id(), bb(1020), 0, false);
        // readState == IDLE, flowControlledBytes = 1, 1 message(1020) buffered.
        assertTrue(flushSniffer.checkFlush());

        // This will trigger a read of the second message (1020 bytes).
        childChannel.read();
        assertTrue(flushSniffer.checkFlush()); // fails.
        // This should have flushed the window update of 1
        // flowControlledBytes = 1020

        childChannel.read();
        // triggered a flow control window updatte in DefaultHttp2LocalFlowCoontroller:465
        // flowControlledBytes = 0 <- we wrote the window update but didn't flush!
        assertTrue(flushSniffer.checkFlush()); // fails: no flush was called.

        Http2DataFrame data = inboundHandler.blockingReadInbound();
        release(data);
    }

There are a few problems that I see so far.

• One is that on beginRead() we will write any flow bytes as an update frame and enter doBeginRead() but if there are no messages to be had we break out of the loop without flushing here.
• A second problem is that when we do write the window update we check if the promise is complete and if it is, we assume it's been flushed here. However, the Http2FrameCodec intercepts these frames and always just sets the promise to done here. This may cause an explicit flush() call to be ineffective as the stream channel attempts to be clever about it and elid unnecessary flushes here.

Netty version

branch 4.1, SHA e0d73bc (Fri Feb 28)

[bryce-anderson]

And I'm happy to work on a patch once I get a 👍 that someone else agrees this is a problem.

[normanmaurer]

@bryce-anderson yes this sounds like a bug.

[normanmaurer]

so yep please work on a fix @bryce-anderson :)