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

[RESTEASY-1205] RestEasy-Netty4: ByteBuf leak in ResteasyHttpRequestDecoder #977

Merged
merged 2 commits into from
Dec 7, 2016

Conversation

gustavocoding
Copy link
Contributor

@AlexKolpa
Copy link

This implementation holds onto the ByteBuf until the entire response has been closed. I'm pretty sure that once the content has been read, it can be closed. So that would be right after this line.

Unfortunately, there's no easy way to get access to the NettyHttpRequest with the currently implementation. One option would be to pass it to the AbstractReaderInterceptorContext from ServerReaderInterceptorContext.

Preferably you'd want this commit in netty to be available, so you'd just need to call new ByteBufInputStream(buff, true) in RestEasyHttpRequestDecoder, and then an inputStream.close() after having read the content in AbstractReaderInterceptorContext

@gustavocoding
Copy link
Contributor Author

@AlexKolpa that commit will surely help, thanks for pointing it out. It apparently will be on next Netty 4.1.x version, but in the meantime, holding the ByteBuf until after the response is closed is certainly better than never releasing the ByteBuf

@asoldano asoldano merged commit ed8b897 into resteasy:master Dec 7, 2016
@gustavocoding gustavocoding deleted the leak branch December 7, 2016 12:08
@crazyzh1984
Copy link

crazyzh1984 commented Dec 8, 2016

Too complex implemention.
I have a simpler implemention on pull request #998
commit b8db623 @asoldano

My implemention will also easy to migrating to the latest netty,
using new ByteBufInputStream(buff, true) referenced by @AlexKolpa

@asoldano
Copy link
Member

@crazyzh1984 actually I found the impl here simpler, anyway, if you feel there's a need for and want to provide an additional improvement PR on top this, feel free to open that up. 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.

4 participants