http: remove excess calls to removeSocket

socket.destroy() triggers a 'close' event from the socket which triggers
the onClose handler of HTTPAgent which calls self.removeSocket(). So by
calling self.removeSocket() prior to socket.destroy() we end up with two
calls to self.removeSocket().

If there are pending requests, removeSocket ends up creating a new socket.
So if there are pending requests, each time a request completes, we tear
down one socket and create two more. So the total number of sockets grows
exponentially and without regard for any maxSockets settings. This was
noticed in https://github.com/nodejs/node/issues/4050. Let's get rid of
the extra calls to removeSocket so we only call it once per completed
request.

PR-URL: https://github.com/nodejs/node/pull/4172
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>

2 files changed, 43 insertions, 2 deletions

diff --git a/lib/_http_agent.js b/lib/_http_agent.js
index 305baa2cbd..c6e3ef63bd 100644
--- a/lib/_http_agent.js
+++ b/lib/_http_agent.js
@@ -66,7 +66,6 @@ function Agent(options) {
           count += self.sockets[name].length;
 
         if (count > self.maxSockets || freeLen >= self.maxFreeSockets) {
-          self.removeSocket(socket, options);
           socket.destroy();
         } else {
           freeSockets = freeSockets || [];
@@ -78,7 +77,6 @@ function Agent(options) {
           freeSockets.push(socket);
         }
       } else {
-        self.removeSocket(socket, options);
         socket.destroy();
       }
     }
diff --git a/test/parallel/test-http-agent-maxsockets-regress-4050.js b/test/parallel/test-http-agent-maxsockets-regress-4050.js
new file mode 100644
index 0000000000..1cbe37cf0c
--- /dev/null
+++ b/test/parallel/test-http-agent-maxsockets-regress-4050.js
@@ -0,0 +1,43 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const MAX_SOCKETS = 2;
+
+const agent = new http.Agent({
+  keepAlive: true,
+  keepAliveMsecs: 1000,
+  maxSockets: MAX_SOCKETS,
+  maxFreeSockets: 2
+});
+
+const server = http.createServer(function(req, res) {
+  res.end('hello world');
+});
+
+function get(path, callback) {
+  return http.get({
+    host: 'localhost',
+    port: common.PORT,
+    agent: agent,
+    path: path
+  }, callback);
+}
+
+server.listen(common.PORT, function() {
+  var finished = 0;
+  const num_requests = 6;
+  for (var i = 0; i < num_requests; i++) {
+    const request = get('/1', function() {
+    });
+    request.on('response', function() {
+      request.abort();
+      const sockets = agent.sockets[Object.keys(agent.sockets)[0]];
+      assert(sockets.length <= MAX_SOCKETS);
+      if (++finished === num_requests) {
+        server.close();
+      }
+    });
+  }
+});