Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Commit

Permalink
remove socket error handler after request is done
Browse files Browse the repository at this point in the history
`npm-registry-client` uses `request`, which in turn uses an HTTP agent
for reusing sockets, so the `error` handlers registered on it in
`npm-registry-client` just piled up and kept being attached over the
entire lifetime of the socket.

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
just be a memory leak.
  • Loading branch information
addaleax authored and othiym23 committed Sep 3, 2016
1 parent 04f418a commit 0ff3352
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 2 deletions.
18 changes: 16 additions & 2 deletions lib/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,16 @@ function regRequest (uri, params, cb_) {
}

function makeRequest (uri, params, cb_) {
var cb = once(cb_)
var socket
var cb = once(function (er, parsed, raw, response) {
if (socket) {
// The socket might be returned to a pool for re-use, so don’t keep
// the 'error' listener from here attached.
socket.removeListener('error', cb)
}

return cb_(er, parsed, raw, response)
})

var parsed = url.parse(uri)
var headers = {}
Expand Down Expand Up @@ -149,8 +158,13 @@ function makeRequest (uri, params, cb_) {
var req = request(opts, params.streaming ? undefined : decodeResponseBody(done))

req.on('error', cb)

// This should not be necessary, as the HTTP implementation in Node
// passes errors occurring on the socket to the request itself. Being overly
// cautious comes at a low cost, though.
req.on('socket', function (s) {
s.on('error', cb)
socket = s
socket.on('error', cb)
})

if (params.streaming) {
Expand Down
8 changes: 8 additions & 0 deletions test/lib/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ if (!global.setImmediate || !require('timers').setImmediate) {
}
}

// See https://github.com/npm/npm-registry-client/pull/142 for background.
// Note: `process.on('warning')` only works with Node >= 6.
process.on('warning', function (warning) {
if (/Possible EventEmitter memory leak detected/.test(warning.message)) {
throw new Error('There should not be any EventEmitter memory leaks')
}
})

module.exports = {
port: server.port,
registry: REGISTRY,
Expand Down

0 comments on commit 0ff3352

Please sign in to comment.