Skip to content
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 in HttpObjectEncoder when content is always empty #12828

Conversation

pderop
Copy link
Contributor

@pderop pderop commented Sep 21, 2022

Motivation:

When the HttpObjectEncoder.encodeAndClose method encodes a response which content is always empty (as described in _HttpObjectEncoder. isContentAlwaysEmpty ), then the empty last http content or the empty FullHttpResponse is not closed.

An http response content is always empty for example when status code is INFORMATIONAL, is a 204, 205, 304 ... See for example here

Example scenario:

  1. a DefaultHttpResponse is first encoded with status 204, in this case, the HttpObjectEncoder.encodeAndClose method will initialize its state to ST_CONTENT_ALWAYS_EMPTY, see here.

  2. next, an EmptyLastHttpContent is encoded. So, the HttpObjectEncoder.encodeAndClose state being in ST_CONTENT_ALWAYS_EMPTY, then the encodeAndClose method switch statement will proceed here directly, which is not closing the EmptyLastHttpContent

Modification:

Move the code which was disposing the message from here to the case on ST_CONTENT_ALWAYS_EMPTY here.
Also added some test cases in HttpResponseEncoderTest.

Result:

Disposing the message to the case on ST_CONTENT_ALWAYS_EMPTY will continue to ensure that the message is closed when the state is ST_CONTENT_NON_CHUNK and when content length is zero (because the case fall-through, see here), and also ensures that any HttpContent is closed when state is ST_CONTENT_ALWAYS_EMPTY.

@pderop
Copy link
Contributor Author

pderop commented Sep 21, 2022

@chrisvest , @normanmaurer , can you take a look ?
thanks.

Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, when I'll port the netty 4.1 version (probably doing things differently really ie using visitors/virtual dispatch) -> see #12708 the test will be our saviour to capture regressions

@pderop
Copy link
Contributor Author

pderop commented Sep 21, 2022

@franz1981 ,

Thanks for the info; to be honest, I did not take a look at the false sharing issue yet, but I'll try to look into #12708 soon.
(for the moment, I'm investigating some remaining buffer leaks).

Copy link
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@chrisvest chrisvest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

@chrisvest chrisvest merged commit 50b1efb into netty:main Sep 21, 2022
@chrisvest chrisvest added this to the 5.0.0.Alpha5 milestone Sep 21, 2022
@pderop
Copy link
Contributor Author

pderop commented Sep 21, 2022

thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants