Skip to content

Leaking memory when handling redirects #1731

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

Closed
will-lauer opened this issue Jul 27, 2020 · 1 comment
Closed

Leaking memory when handling redirects #1731

will-lauer opened this issue Jul 27, 2020 · 1 comment

Comments

@will-lauer
Copy link
Contributor

We've encountered a memory leak when handling redirects. It looks like when a redirect is encountered by the system, it ends up reusing the existing NettyResponseFuture to handle the request to the new destination. This ends in turn replaces the old request timeout with a new one, orphaning the old timeout. Since references to timeouts are kept in the HashedWheelTimer, and since timeouts have references to the associated RequestTimoutTimerTask, which has a reference to the NettyResponseFuture, none of these objects get cleaned up until the timeout expires and the HashedWheelTimer goes through its periodic cleanup.

In our case, we had accidentially set the request timeout up to 10 hours. We also are using an AsyncResponseHandler to process requests. The AsyncResponseHandler maintains a reference to the ResponseBuilder, and since the Future maintains a reference the AsyncResponseHandler, the result was that all responses from redirected requests were kept in memory until the timeout expired, even though the response handler had finished processing them. Due to oddities of the services we interact with, around 50% of our requests were redirects, causing the system to build up tens of gigabytes of requests in memory in only a couple of hours before the system finally fell over.

will-lauer added a commit to will-lauer/async-http-client that referenced this issue Jul 28, 2020
Fix for issue AsyncHttpClient#1731.

When setting the TimeoutHolder, cancel any prior timeout so that they don't leak. 

Previously, they would just be lost and remain in the timer until their timeout expired, which could be a long time. Previously, encountering a redirect would trigger this code, causing the old request timer to be replaced with a new one. The old timer would maintain a link back to this Future, but couldn't be canceled by this future (as its reference had been overwritten) causing the Future, is associated Response, and any AsyncResponseHandler to be retained in memory instead of being garbage collected once the request had been processed.
slandelle pushed a commit that referenced this issue Aug 2, 2020
Fix for issue #1731.

When setting the TimeoutHolder, cancel any prior timeout so that they don't leak. 

Previously, they would just be lost and remain in the timer until their timeout expired, which could be a long time. Previously, encountering a redirect would trigger this code, causing the old request timer to be replaced with a new one. The old timer would maintain a link back to this Future, but couldn't be canceled by this future (as its reference had been overwritten) causing the Future, is associated Response, and any AsyncResponseHandler to be retained in memory instead of being garbage collected once the request had been processed.
@slandelle
Copy link
Contributor

Closed by #1732

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 a pull request may close this issue.

2 participants