Fix event listener leak

[daguej]

When request handles a redirect, extra event listeners for the pipe and end events are erroneously attached to the request object. This causes a few problems:

• When the request finally completes and end is fired, the cleanup function ends up running multiple times (1 + number of redirects). This doesn't actually hurt anything, but it is wasteful.
• If someone were to pipe to the request after a redirect had occurred (which is an error), the pipe event handler will run multiple times, thus emitting the error event multiple times.
• If there are 7 or more redirects, node will print leak warnings to stderr:

(node) warning: possible EventEmitter memory leak detected. 11 pipe listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at Request.addListener (events.js:239:17)
    at Request.init (./node_modules/request.js:507:8)
    at Redirect.onResponse (./node_modules/lib/redirect.js:148:11)
    at Request.onRequestResponse (./node_modules/request.js:898:22)
    at emitOne (events.js:77:13)
    at ClientRequest.emit (events.js:169:7)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:415:21)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:88:23)
    at Socket.socketOnData (_http_client.js:305:20)
    at emitOne (events.js:77:13)
(node) warning: possible EventEmitter memory leak detected. 11 end listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at Request.addListener (events.js:239:17)
    at Request.start (./node_modules/request.js:797:8)
    at Request.end (./node_modules/request.js:1358:10)
    at end (./node_modules/request.js:575:14)
    at Immediate._onImmediate (./node_modules/request.js:589:7)
    at processImmediate [as _immediateCallback] (timers.js:383:17)

These warnings have been reported many, many times (#107, #281, #311, #585, #622, #673, #708,
#842, #870, #1497, #1809) but the underlying issue has never actually been properly resolved.

This fixes the leak for good by not attaching additional handlers if they're already there.

The diff makes it look like there are more changes than there actually are due to changes in indentation. The only actual change is to wrap the event listeners:

+  if (EventEmitter.listenerCount(self, '...') === 0) {
     ...
+  }

The small client & server I used to demonstrate this issue can be found here.

[roddik]

I'm just here to confirm that this issue is still not fixed, a simple way to test is to put this:

<?php

$n = (int) @$_GET['n'];

if ($n == 10) {
    echo 'RESULT';
} else {
    header('Location: '.$_SERVER['SCRIPT_NAME'].'?n='.($n+1));
}

script on localhost and fetch with request it to see the annoying warning.

[instantshinramen]

Are there any plans to resolve this issue for good? We're seeing this memory leak as well and would appreciate the resolution.

[daguej]

We're still waiting for this to be resolved. @mikeal @nylen @FredKSchott @simov?

[presidenten]

So is it time to merge this or what? :-)

[zackarychapple]

+1 waiting on this.

[zarenner]

+1

[vbardales]

👍

[prasadKodeInCloud]

+1

[jpike88]

Is anyone maintaining this repo anymore?

[FredKSchott]

This PR has been closed as "abandoned" as a part of an automated cleanup. If this is a mistake and you are still interested in merging this PR, please do the following:

1. Rebase your changes onto the master branch (even if there are no merge conflicts, we want to make sure your change is under the most recent tests).
2. Push your updated branch to Github to update this PR.
3. Make sure this PR's Travis tests are still passing. (Run npm test locally if tests are still failing).
4. Comment here to let us know that you've updated the PR and are still interested in getting it merged. Somebody from the project will respond and work with you to review.

Thank you for your understanding. If you have any questions, please check out #2556 and feel free to comment / ask anything there.

[FredKSchott]

@daguej would love to get this merged, sorry I'm only just seeing this now. In addition to the above, we'll also need a test to confirm this issue / confirm this fix / make sure it never regresses again.