remove socket `error` handler after request is done

[addaleax]

Hi y’all! :smile: Carrying this over from https://github.com/nodejs/node/issues/8279#issuecomment-242894705:

npm-registry-client uses request, which in turn uses an HTTP agent for reusing sockets, so the error handlers registered in these lines in npm-registry-client just pile up and keep being attached over the entire lifetime of the socket:

  req.on('socket', function (s) {
    s.on('error', cb)
  })

which leads to the famous Warning: Possible EventEmitter memory leak detected. 11 error listeners added. warning.

This patch seeks to fix this by removing the listener from the socket once the callback is invoked, as keeping it around after that would really just be a memory leak (… I can’t believe that warning message is right for once).

(btw, I’m still not sure having npm-registry-client attach error listeners on the socket itself is a correct thing to do, but given that it’s been that way since 2012 and this is a kind of high-profile package, I’m reluctant to do actual behavioural changes…)

[zkat]

This LGTM, although I'll ping @othiym23 or @iarna as a backup set of eyes because request is... quirky sometimes.

Although it's a relatively trivial patch, it might be pretty nice to have a test written for it so no one in the distant future decides that "well, the socket is closing, so this cb is unnecessary" :)

Thanks a lot for this. It'll be nice to stop having all that EventEmitter spam. 8D

[addaleax]

Although it's a relatively trivial patch, it might be pretty nice to have a test written for it so no one in the distant future decides that "well, the socket is closing, so this cb is unnecessary" :)

Thing is, the only observable change that could really be tested for here (I think) is that the warning pops up. I’ve added a check to test/lib/common.js that tests for no such warning being emitted… I’m not sure that’s what you had in mind but it makes the tests fail on master.

[zkat]

I was thinking of something along the lines of emitter.listenerCount() but it's true -- this is a pretty specific corner case test, but it still feels good to make sure something is there to guard against this leak in the future.

[addaleax]

I was thinking of something along the lines of emitter.listenerCount()

Yeah … I’m just afraid it’s going to be tricky (and depending on implementation details?) to get to the emitter itself, and to get a reliable number of event listeners at which to cut off… if you think it’s worth it, I’ll do the digging and see if it’s possible.

[zkat]

I don't know if it's worth it either. I'll check back in on Monday to finalize this, but tbh it's probably fine without significant testing. :\

[othiym23]

Is this solution, which feels a little hacky, just a consequence of using request with our own Agent implementation? I'm fine with the patch, given the EventEmitter leak that's showing up with the new version of request, but I'm a little paranoid that we're adding a temporary fix to a deeper problem, because it's not clear to me why request changed here in the first place.

Either way, I'm inclined to land this, maybe with a comment added to the code to make clear why this patch was added (and that it might need to be updated, changed, or removed) in the future.

[addaleax]

Is this solution, which feels a little hacky, just a consequence of using request with our own Agent implementation?

Are you talking about the setup in lib/initialize.js? If so, the answer is very likely no – the behaviour of keeping user-added listeners when pulling a socket back into the agent’s pool is something that comes from Node core here.

but I'm a little paranoid that we're adding a temporary fix to a deeper problem, because it's not clear to me why request changed here in the first place.

That’s quite understandable, I don’t know why this popped up either (or just became more frequent? I think it’s rather that?).

Either way, I'm inclined to land this, maybe with a comment added to the code to make clear why this patch was added (and that it might need to be updated, changed, or removed) in the future.

Hmm, got anything concrete in mind? Otherwise I’d prefer doing some more digging (starting to bisect request right now) before I try add a comment that may or may not be accurate.

[othiym23]

or just became more frequent? I think it’s rather that?

Nope, we got no reports of this prior to the latest request upgrade (see below for a link to a discussion of why).

Hmm, got anything concrete in mind? Otherwise I’d prefer doing some more digging (starting to bisect request right now) before I try add a comment that may or may not be accurate.

I think it's request/request@bf86f170257de6a0cd7b3342676bb43b63219d9a, which, to be honest, feels like a pretty brute-force fix for request/request#1903. From reading the thread on request/request#1903, it sounds like it's sort of a historical accident that request was removing error handlers in the first place, and it won't be coming back, but I could be wrong.

[addaleax]

I think it's request/request@bf86f17

Yup, I can confirm that… the removed response.connection.setMaxListeners(0) call seems like the reason for this.

If I’m understanding it correctly, the npm-registry-client shouldn’t need to do its own error handling for the socket here… I’ve added a comment that hints at that, and while it would match my first intuition and what the http logic in Node says, but I’d really prefer to err on the side of caution by keeping the semantics of this module as they are.

[isaacs]

This fix is almost verbatim what I'd thought would be the approach when I posted #143.

It's probably safe to just remove the error handler, since socket errors are proxied to the request object now, and have been since (I think?) node v0.8, but I could be wrong and that should be investigated.

[othiym23]

Since 0.10 is the oldest version of Node the npm CLI team currently supports, and we're sunsetting our support for 0.10 real soon now (along with the Node project), I'm fine with just landing this change. Thanks for the followup, @isaacs!

[mhart]

I believe this should fix https://github.com/npm/npm/issues/13656

[othiym23]

Landed as 0ff33527ce218a803ec2708be3458dfd7ce8936e, df9be53d9376ec10c30d44b01a8cdb70a86d0e75, and f62a28df786d132e452b329414a323df9af7fbfd, and released as part of npm-release-client@7.2.0, which will be included in the next release of npm. Thanks so much for putting this together, @addaleax!