search

npm / npm-registry-client

http://npm.im/npm-registry-client
ISC License
264 stars 108 forks source link

EventEmitter leak detection #143

Closed isaacs closed 8 years ago

isaacs commented 8 years ago 
(node) warning: possible EventEmitter memory leak detected. 11 error listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at TLSSocket.addListener (events.js:239:17)
    at TLSSocket.Readable.on (_stream_readable.js:680:33)
    at Request.<anonymous> (/Users/isaacs/dev/npm/npm/node_modules/npm-registry-client/lib/request.js:153:7)
    at emitOne (events.js:77:13)
    at Request.emit (events.js:169:7)
    at ClientRequest.<anonymous> (/Users/isaacs/dev/npm/npm/node_modules/request/request.js:791:10)
    at emitOne (events.js:82:20)
    at ClientRequest.emit (events.js:169:7)
    at tickOnSocket (_http_client.js:523:7)
    at onSocketNT (_http_client.js:535:5)

This is happening at this line in lib/request.js:

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

Because requests re-use those connections, you can end up attaching the error handler a lot of times.

It'd be good to remove it or something once we're done.

Apologies, this would be a PR instead of an issue but I didn't have time to dig in and figure out the best place to do that. Posting it this way so that it's not lost, like tears in rain.

addaleax commented 8 years ago

@isaacs take a look at https://github.com/npm/npm-registry-client/pull/142 if you want – and since you’re the one who wrote the req.on('socket') bit… do you happen to know if that’s necessary? it seems node has pretty much always been forwarding socket errors to the HTTP objects.

isaacs commented 8 years ago

@addaleax Ha! Yes, this is a duplicate, then. I'll go comment over there.