http: guard against failed sockets creation

PR-URL: nodejs#13839 Fixes: nodejs#13045 Fixes: nodejs#13831 Refs: nodejs#13352 Refs: nodejs#13548 (comment) Reviewed-By: Trevor Norris <trev.norris@gmail.com>
addaleax · Jul 3, 2017 · 45a9c58 · 45a9c58
1 parent 095bf4c
commit 45a9c58
Show file tree

Hide file tree

Showing 2 changed files with 72 additions and 47 deletions.
diff --git a/lib/_http_agent.js b/lib/_http_agent.js
@@ -181,15 +181,7 @@ Agent.prototype.addRequest = function addRequest(req, options, port/*legacy*/,
   } else if (sockLen < this.maxSockets) {
     debug('call onSocket', sockLen, freeLen);
     // If we are under maxSockets create a new one.
-    this.createSocket(req, options, function(err, newSocket) {
-      if (err) {
-        nextTick(newSocket._handle.getAsyncId(), function() {
-          req.emit('error', err);
-        });
-        return;
-      }
-      req.onSocket(newSocket);
-    });
+    this.createSocket(req, options, handleSocketCreation(req, true));
   } else {
     debug('wait for socket');
     // We are over limit so we'll add it to the queue.
@@ -222,6 +214,7 @@ Agent.prototype.createSocket = function createSocket(req, options, cb) {
   const newSocket = self.createConnection(options, oncreate);
   if (newSocket)
     oncreate(null, newSocket);
+
   function oncreate(err, s) {
     if (called)
       return;
@@ -294,15 +287,7 @@ Agent.prototype.removeSocket = function removeSocket(s, options) {
     debug('removeSocket, have a request, make a socket');
     var req = this.requests[name][0];
     // If we have pending requests and a socket gets closed make a new one
-    this.createSocket(req, options, function(err, newSocket) {
-      if (err) {
-        nextTick(newSocket._handle.getAsyncId(), function() {
-          req.emit('error', err);
-        });
-        return;
-      }
-      newSocket.emit('free');
-    });
+    this.createSocket(req, options, handleSocketCreation(req, false));
   }
 };
 
@@ -332,6 +317,22 @@ Agent.prototype.destroy = function destroy() {
   }
 };
 
+function handleSocketCreation(request, informRequest) {
+  return function handleSocketCreation_Inner(err, socket) {
+    if (err) {
+      const asyncId = (socket && socket._handle && socket._handle.getAsyncId) ?
+        socket._handle.getAsyncId() :
+        null;
+      nextTick(asyncId, () => request.emit('error', err));
+      return;
+    }
+    if (informRequest)
+      request.onSocket(socket);
+    else
+      socket.emit('free');
+  };
+}
+
 module.exports = {
   Agent,
   globalAgent: new Agent()

diff --git a/test/parallel/test-http-agent.js b/test/parallel/test-http-agent.js
@@ -20,41 +20,65 @@
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 'use strict';
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 const http = require('http');
 
-const server = http.Server(function(req, res) {
-  res.writeHead(200);
-  res.end('hello world\n');
-});
-
-let responses = 0;
 const N = 4;
 const M = 4;
+const server = http.Server(common.mustCall(function(req, res) {
+  res.writeHead(200);
+  res.end('hello world\n');
+}, (N * M))); // N * M = good requests (the errors will not be counted)
 
-server.listen(0, function() {
-  const port = this.address().port;
-  for (let i = 0; i < N; i++) {
-    setTimeout(function() {
-      for (let j = 0; j < M; j++) {
-        http.get({ port: port, path: '/' }, function(res) {
-          console.log('%d %d', responses, res.statusCode);
-          if (++responses === N * M) {
-            console.error('Received all responses, closing server');
-            server.close();
-          }
-          res.resume();
-        }).on('error', function(e) {
-          console.log('Error!', e);
-          process.exit(1);
-        });
+function makeRequests(outCount, inCount, shouldFail) {
+  let responseCount = outCount * inCount;
+  let onRequest = common.mustNotCall(); // Temporary
+  const p = new Promise((resolve) => {
+    onRequest = common.mustCall((res) => {
+      if (--responseCount === 0) {
+        server.close();
+        resolve();
       }
-    }, i);
-  }
-});
+      if (!shouldFail)
+        res.resume();
+    }, outCount * inCount);
+  });
+
+  server.listen(0, () => {
+    const port = server.address().port;
+    for (let i = 0; i < outCount; i++) {
+      setTimeout(() => {
+        for (let j = 0; j < inCount; j++) {
+          const req = http.get({ port: port, path: '/' }, onRequest);
+          if (shouldFail)
+            req.on('error', common.mustCall(onRequest));
+          else
+            req.on('error', (e) => assert.fail(e));
+        }
+      }, i);
+    }
+  });
+  return p;
+}
 
+const test1 = makeRequests(N, M);
 
-process.on('exit', function() {
-  assert.strictEqual(N * M, responses);
-});
+const test2 = () => {
+  // Should not explode if can not create sockets.
+  // Ref: https://github.com/nodejs/node/issues/13045
+  // Ref: https://github.com/nodejs/node/issues/13831
+  http.Agent.prototype.createConnection = function createConnection(_, cb) {
+    process.nextTick(cb, new Error('nothing'));
+  };
+  return makeRequests(N, M, true);
+};
+
+test1
+  .then(test2)
+  .catch((e) => {
+    // This is currently the way to fail a test with a Promise.
+    console.error(e);
+    process.exit(1);
+  }
+);