Skip to content

Fix memory leak on initial requests with HttpObjectAggregator#351

Merged
jekh merged 1 commit into
adamfisk:masterfrom
jekh:fix-memory-leak-on-initial-request
Mar 5, 2017
Merged

Fix memory leak on initial requests with HttpObjectAggregator#351
jekh merged 1 commit into
adamfisk:masterfrom
jekh:fix-memory-leak-on-initial-request

Conversation

@jekh
Copy link
Copy Markdown
Collaborator

@jekh jekh commented Feb 26, 2017

This PR should fix issue #348. The change is very simple: initial HttpRequests are released if they are ReferenceCounted. However, the reasoning is complicated and requires a non-trivial understanding of netty reference counting, so here's a more detailed explanation of what's happening:

When HttpObjectAggregator is in the pipeline, it generates ReferenceCounted FullHttpRequests (since they have content as well as the request headers). Whereas, when HttpObjectAggregator is not in the pipeline, the HttpRequest does not contain body content and therefore is not ReferenceCounted.

When the ProxyToServerConnection#write() method is called, it always increments ReferenceCounted objects, since netty will automatically decrement them when they are written to the server (and we want to keep them around, since our ClientToProxyConnection will also try to clean them up). Generally this works fine, even for FullHttpRequests, but it breaks when dealing with the initial request.

For initial HTTP messages, which are eventually written to the server, the ProxyToServerConnection#write() method is called twice: first when the server is disconnected, and then again after the connection is complete. This causes the ReferenceCounted object to be .retain()ed twice, but only released by netty once (when it is actually written to the server).

For initial HTTPS messages (always a CONNECT), which are not written to the server at all, they are only .retain()ed once, when the server is disconnected, then "dropped", since the CONNECT is never written to the upstream server. But since the FullHttpRequest was already retained by the write() method, it must be manually released.

@mjallday
Copy link
Copy Markdown

we have verified that this resolves #348. lgtm!

@jekh jekh merged commit 070e6c1 into adamfisk:master Mar 5, 2017
@jekh jekh deleted the fix-memory-leak-on-initial-request branch March 5, 2017 18:47
@linking12
Copy link
Copy Markdown

connectionSucceeded will release request,but when connectionfailed happen, it must be also release request! @jekh

asolntsev added a commit to LittleProxy/LittleProxy that referenced this pull request Aug 8, 2022
This change continues PR adamfisk/LittleProxy#351 - it seems it didn't cover all possible cases.
@asolntsev
Copy link
Copy Markdown

asolntsev commented Aug 8, 2022

@linking12 Hi. I was investigating issue LittleProxy/LittleProxy#131 and encountered your comment.

Can you review my PR that hopefully should fix the issue?

asolntsev added a commit to LittleProxy/LittleProxy that referenced this pull request Aug 9, 2022
This change continues PR adamfisk/LittleProxy#351 - it seems it didn't cover all possible cases.
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