Fix exception if Response has content #11093
Conversation
|
@normanmaurer in the pass through case the message is added to out without being retained: https://github.com/netty/netty/blob/12fbc8320fb935ed7a8de08729de9bc41aadfdbf/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java#L146 but then when it runs the PASS_THROUGH branch it is added again and is retained: Is it intentional to have the same object in the out list twice? |
|
I am not sure how these failures could be related, is this a known issue? |
|
@stuartwdouglas imho have the same object in the out list twice is not expected |
|
@stuartwdouglas yeah these failures are not related... not sure yet why these popped up. |
If compression is enabled and the HttpResponse also implements HttpContent (but not LastHttpContent) then the buffer will be freed to eagerly. Fixes netty#11092
|
Turns out I misread the code. I think it is fine, and there is no need to retain it because if it is pass through there is no body anyway. |
|
@stuartwdouglas ok... so you say this PR is ready to go then ? |
|
Yes, it is good to go. |
|
The test failure is fixed by #11102 and not related |
Motivation: If compression is enabled and the HttpResponse also implements HttpContent (but not LastHttpContent) then the buffer will be freed to eagerly. Modification: I retain the buffer the same way that is done for the LastHttpContent case. Note that there is another suspicious looking call a few lines above (if beginEncode returns null). I am not sure if this should also be retained. Result: Fixes #11092
|
@stuartwdouglas thanks a lot |
Motivation: If compression is enabled and the HttpResponse also implements HttpContent (but not LastHttpContent) then the buffer will be freed to eagerly. Modification: I retain the buffer the same way that is done for the LastHttpContent case. Note that there is another suspicious looking call a few lines above (if beginEncode returns null). I am not sure if this should also be retained. Result: Fixes netty#11092
Motivation:
If compression is enabled and the HttpResponse also
implements HttpContent (but not LastHttpContent) then
the buffer will be freed to eagerly.
Modification:
I retain the buffer the same way that is done for the LastHttpContent case.
Note that there is another suspicious looking call a few lines above (if beginEncode returns null). I am not sure if this should also be retained.
Result:
Fixes #11092