DelegatingDecompressorFrameListener doesn't support deferral of processed bytes

[npordash]

Http2FrameListener.onDataRead states that reporting of processed bytes to the local flow controller can be deferred by returning something less than data + padding and then later doing something equivalent to connection().local().flowController().consumeBytes(Http2Stream, int).

This works fine until you start using a DelegatingDecompressorFrameListener because once you call consumeBytes you end up with the following exception:

io.netty.handler.codec.http2.Http2Exception: Error while returning bytes to flow control window
    at io.netty.handler.codec.http2.DelegatingDecompressorFrameListener$ConsumedBytesConverter.consumeBytes(DelegatingDecompressorFrameListener.java:361)
...
Caused by: java.lang.IllegalArgumentException: processed bytes cannot be negative
    at io.netty.handler.codec.http2.DelegatingDecompressorFrameListener$Http2Decompressor.incrementProcessedBytes(DelegatingDecompressorFrameListener.java:409)
    at io.netty.handler.codec.http2.DelegatingDecompressorFrameListener$Http2Decompressor.consumeProcessedBytes(DelegatingDecompressorFrameListener.java:445)
    at io.netty.handler.codec.http2.DelegatingDecompressorFrameListener$ConsumedBytesConverter.consumeBytes(DelegatingDecompressorFrameListener.java:349)

As far as I can tell the reason for this is because Http2Decompressor is expecting incrementProcessedBytes to be called prior to consumeProcessedBytes and you can't consume more than it has been incremented, which makes sense, but DelegatingDecompressorFrameListener.onDataRead only calls incrementProcessedBytes after all calls to onDataRead returns and only increments the processed bytes based on what was accumulated from the onDataRead calls which effectively means you need to fully consume the buffer in onDataRead.

I tested this with both 4.1.0.Final and 4.1.1.Final.

[Scottmitch]

Thanks for reporting @npordash! Are you in a situation where listener.onDataRead is called multiple times in DelegatingDecompressorFrameListener, and you are trying to call consumeBytes(..) for data which was not returned by previous calls to Http2FrameListener.onDataRead?

[Scottmitch]

I tested this with both 4.1.0.Final and 4.1.1.Final.

Do you have a unit test which you would be willing to contribute? I have a PR but it would be nice to verify against a unit test if you already have one.

[npordash]

Thanks for the quick response!

I'm not in a situation where onDataRead is being called multiple times, at least I have yet to observe that happening. The exception gets thrown on the first data frame received.

My use-case is that I'm exposing inbound data frames as an rx.Observable<ByteBuf> and I only want to report the processing of the bytes when a ByteBuf has been sent to a subscriber of the Observable. It's possible that the ByteBuf is sent to a subscriber inside the call to onDataRead if the processing is synchronous which means that the call to consumeBytes might happen before onDataRead returns as well.

Unfortunately, I do not have an existing unit test for this yet. I stumbled across this while I was prototyping which involved manual testing with nghttp to send data to a netty http2 server 😦

[Scottmitch]

which means that the call to consumeBytes might happen before onDataRead returns as well.

As long as you are not double releasing the bytes you should be OK. I think I found the issue and I'll ask you to verify with the PR when its ready.

[npordash]

Awesome, thanks again!