Http2 Hang with AbstractHttp2StreamChannel

[carl-mastrangelo]

Netty version

4.1.42-Final

I am working on upgrading to Netty 4.1.42 from 4.1.27, and noticed a bug where HTTP/2 streams will sometimes hang. I am pretty close to the bug, but wanted to get a second set of eyes on it. We (Netflix/Zuul) are using the Http2MultiplexCodecBuilder to do Http/1.1 to Http2 connection upgrades. When one of the H2 child channels tries to write a lot of data (>64k) the overall connection hangs. I believe this is due to a dropped writability event. Here is the (approx) sequence of events:

1. Child channel tries to flush a lot of data. There is ample H2 flow control window is. The channel checks the max writable bytes for the channel, which is about 64K.
2. Channel writes the data, and becomes unwritable.
3. The channel (an EpollSocketChannel) becomes writable again and triggers ChannelOutboundBuffer.fireChannelWritabilityChanged
4. This propagates up to Http2MultiplexCodec.channelWritabilityChanged
5. Each stream (child channel) gets the AbstractHttp2StreamChannel.WRITABLE_VISITOR applied to it.
6. The child channel (a Http2MultiplexCodecStreamChannel) . gets trySetWritable() called. It looks like:

final void trySetWritable() {
        // The parent is writable again but the child channel itself may still not be writable.
        // Lets try to set the child channel writable to match the state of the parent channel
        // if (and only if) the totalPendingSize is smaller then the low water-mark.
        // If this is not the case we will try again later once we drop under it.
        if (totalPendingSize < config().getWriteBufferLowWaterMark()) {
            setWritable(false);
        }
    }

7. The amount of pending data is very large, so it never gets setWritable called.
8. The connection hangs from this point on.

I think the bug is in 6, since this is last event that happens on the child channel. I don't fully understand this code, so I may be mistaken. Also also realize Http2MultiplexCodecBuilder is deprecated, but I think the bug is in this shared code. This was not previously an issue in 4.1.27, so I think it is new.

cc: @normanmaurer @artgon

[carl-mastrangelo]

After some more debugging, I think I know the change that caused this: #9254 or #9235

One of those changes made Http2MultiplexCodec override channelWritabilityChanged, which prevents the parent class Http2ConnectionHandler method, prevents the encoder.flowController() writability from getting updated. I think the child class should still call the parent method.

[normanmaurer]

@carl-mastrangelo this sounds about right... Can you ope a PR ?

That said I think you should switch to use Http2FrameCodec + Http2MultiplexHandler :) See https://netty.io/news/2019/06/28/4-1-37-Final.html

[carl-mastrangelo]

@normanmaurer Yup, I was about to send out PR, but tried writing a unit test to replicate it. It's surprisingly difficult to get the conditions just right to happen, but I do have an exact reproducer internally. I patched in the change (calling super) and can confirm it works.

Definitely take your point about using the new thing (and already updated our code), but since Http2MultiplexCodec is still around, I figured I'd at least try to fix it.