-
-
Notifications
You must be signed in to change notification settings - Fork 15.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix buffer leak regression in HttpClientCodec #12762
Conversation
Motivation: netty#12709 changed HttpObjectEncoder to override the write method of MessageToMessageEncoder, with slightly changed semantics: The `msg` argument to `encode` is not released anymore. To accommodate this change, netty#12709 also update `HttpObjectEncoder.encode` to release the `msg`. However, `HttpClientCodec.Encoder` overrides `encode` and simply forwards the message if a HTTP upgrade has been completed. This code path was not updated to release the input message. This leads to a memory leak. Modifications: Changed the `encode` implementation to not retain the message that is forwarded. Added a test case to verify that the refCnt to the data passed through is unchanged. Result: The buffer retains its correct refCnt and will be released properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find!
We don't really have benchmarks that could tell the difference unfortunately |
Hi there, From my understanding, 4.1.80 is basically burnt because of this regression. Cheers! |
release should happen within the next days. |
Awesome! Thanks a lot @normanmaurer ! |
Motivation:
#12709 changed HttpObjectEncoder to override the write method of MessageToMessageEncoder, with slightly changed semantics: The
msg
argument toencode
is not released anymore. To accommodate this change, #12709 also updateHttpObjectEncoder.encode
to release themsg
. However,HttpClientCodec.Encoder
overridesencode
and simply forwards the message if a HTTP upgrade has been completed. This code path was not updated to release the input message. This leads to a memory leak.Modifications:
Changed the
encode
implementation to not retain the message that is forwarded. Added a test case to verify that the refCnt to the data passed through is unchanged.Result:
The buffer retains its correct refCnt and will be released properly.