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

Why EventEmitter: memory leak detected gets emitted after ten redirects #3139

Closed
kevinburkenotion opened this issue Mar 28, 2019 · 2 comments
Labels

Comments

@kevinburkenotion
Copy link

kevinburkenotion commented Mar 28, 2019

I found the existing answers insufficient on this point so I thought I would try to write up an explanation.

By default request will follow up to ten redirects. If you have a server that is misbehaving, for example redirecting to itself, request will follow the redirect up to ten times and then return an error.

However, the request also listens for an incoming request body - you can pipe a Readable to the request object and this library will read and upload that response. This is done by registering a request.on('pipe', (src) => ...) listener in the request source code.

The problem is that on each followed redirect, request redeclares the .on('pipe', (src) => ...) listener, so by the 7th redirect you have 7 listeners and so on. By default the EventEmitter warns after ten of the same event have been registered, which is why you get Possible memory leak detected exactly once, with the default settings for both the EventEmitter and for request.

This is probably a mistake - if you did manage to pipe the request body to the 7th HTTP request, you would have seven listeners by that point, each firing independently, and it's not clear what would happen - you might end up with the request body duplicated seven times, or just written seven times to something that's not listening. However, there is a separate issue where this piped request body is only sent on the first request, not all of them, see #3138.

So is there actually a memory leak if you have lots of redirects? Probably not... the listeners are registered on the request object, as long as that request object goes out of scope (you're not holding a reference to it somewhere) then it will eventually get garbage collected and the event emitters will be collected.

What is the fix? In this library, either unregister and re-register the pipe handler each time a new redirect is issued, don't re-register the pipe handler if we are following a redirect.

In your code, it's probably safe to either set maxRedirects to 9, or EventEmitter default to 11, either of which will resolve the issue, as long as your request objects are getting garbage collected (they probably are).

Note if you are getting memory leak detected for behavior other than redirect following there may still be a memory leak, see for example #2575.

@stale
Copy link

stale bot commented Mar 27, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 27, 2020
@stale stale bot closed this as completed Apr 3, 2020
@satyamobifyi
Copy link

I have the same issue. Is there any solution for this.

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

No branches or pull requests

2 participants