Fix BrotliEncoder bug that does not mark ByteBuf it encodes as read

[vietj]

Expected behavior

The BrotliEncoder respects the implicit HttpContentCompressor contract that ByteBuf should be marked as read (readableBytes() == 0).

Actual behavior

The BrotliEncoder does not mark encoded ByteBuf as read, HttpContent encoded by HttpContentCompressor can have their content be sent twice once decoded and once encoded, corrupting the response. This happens because the Brotli4J library uses the ByteBuf NIO buffer and does not read it through the ByteBuf like other compressor usually do.

Steps to reproduce

Sending a chunked HTTP response encoded with the Brotli compressor reproduces it.

Netty version

Any Brotli4j version, but I haven't checked.

[franz1981]

LGTM, good find @vietj !!

[franz1981]

@hyperxpro wanna take a look bud?

[hyperxpro]

Thanks for the fix. :)

[hyperxpro]

@hyperxpro wanna take a look bud?

I was under the impression that ByteBuf would get discarded later but seems not. :(

[vietj]

@hyperxpro wanna take a look bud?

I was under the impression that ByteBuf would get discarded later but seems not. :(

Actually it can be passed to the HTTP encoder when it is used in a response that has a content.

I think that having the HttpContentCompressor recreate a new response with the new encoded buffer and discard the other one would be much cleaner. Currently it relies on the fact that the HTTP encoder will skip encoding a zero length chunk as side effect.

[hyperxpro]

Error:  testCompressionOfBatchedFlowOfData{ByteBuf}[2]  Time elapsed: 0.062 s  <<< ERROR!
io.netty.handler.codec.EncoderException: java.lang.IllegalArgumentException: minimumReadableBytes : -54 (expected: >= 0)

[normanmaurer]

@hyperxpro my bad ... fixed

[normanmaurer]

@hyperxpro wanna take a look bud?

I was under the impression that ByteBuf would get discarded later but seems not. :(

Actually it can be passed to the HTTP encoder when it is used in a response that has a content.

I think that having the HttpContentCompressor recreate a new response with the new encoded buffer and discard the other one would be much cleaner. Currently it relies on the fact that the HTTP encoder will skip encoding a zero length chunk as side effect.

@vietj maybe you want to do another PR as well ?

[vietj]

@hyperxpro wanna take a look bud?

I was under the impression that ByteBuf would get discarded later but seems not. :(

Actually it can be passed to the HTTP encoder when it is used in a response that has a content.
I think that having the HttpContentCompressor recreate a new response with the new encoded buffer and discard the other one would be much cleaner. Currently it relies on the fact that the HTTP encoder will skip encoding a zero length chunk as side effect.

@vietj maybe you want to do another PR as well ?

I will give a try @normanmaurer

[normanmaurer]

@vietj thanks a lot!