net: refactor Server.prototype.listen

This PR simplifies Server.prototype.listen, removing some redundancy and
inconsistency. Because listen and connect have a similar function signature,
normalizeConnectArgs can be reused for listen.
listenAfterLookup renamed to lookupAndListen for consistency with
lookupAndConnect, and moved out of Server.prototype.listen.

PR-URL: https://github.com/nodejs/node/pull/4039
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Glen Keane <glenkeane.94@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

lib/_tls_wrap.js

@@ -952,7 +952,7 @@ function SNICallback(servername, callback) {
 //
 //
 function normalizeConnectArgs(listArgs) {
-  var args = net._normalizeConnectArgs(listArgs);
+  var args = net._normalizeArgs(listArgs);
   var options = args[0];
   var cb = args[1];
 

lib/net.js

@@ -63,15 +63,16 @@ exports.connect = exports.createConnection = function() {
   var args = new Array(arguments.length);
   for (var i = 0; i < arguments.length; i++)
     args[i] = arguments[i];
-  args = normalizeConnectArgs(args);
+  args = normalizeArgs(args);
   debug('createConnection', args);
   var s = new Socket(args[0]);
   return Socket.prototype.connect.apply(s, args);
 };
 
-// Returns an array [options] or [options, cb]
+// Returns an array [options, cb], where cb can be null.
 // It is the same as the argument of Socket.prototype.connect().
-function normalizeConnectArgs(args) {
+// This is used by Server.prototype.listen() and Socket.prototype.connect().
+function normalizeArgs(args) {
   var options = {};
 
   if (args.length === 0) {
@@ -91,9 +92,11 @@ function normalizeConnectArgs(args) {
   }
 
   var cb = args[args.length - 1];
-  return typeof cb === 'function' ? [options, cb] : [options];
+  if (typeof cb !== 'function')
+    cb = null;
+  return [options, cb];
 }
-exports._normalizeConnectArgs = normalizeConnectArgs;
+exports._normalizeArgs = normalizeArgs;
 
 
 // called when creating new Socket, or when re-using a closed Socket
@@ -888,7 +891,7 @@ Socket.prototype.connect = function(options, cb) {
     var args = new Array(arguments.length);
     for (var i = 0; i < arguments.length; i++)
       args[i] = arguments[i];
-    args = normalizeConnectArgs(args);
+    args = normalizeArgs(args);
     return Socket.prototype.connect.apply(this, args);
   }
 
@@ -1325,84 +1328,70 @@ function listen(self, address, port, addressType, backlog, fd, exclusive) {
 
 
 Server.prototype.listen = function() {
-  var self = this;
+  var args = new Array(arguments.length);
+  for (var i = 0; i < arguments.length; i++)
+    args[i] = arguments[i];
+  var [options, cb] = normalizeArgs(args);
 
-  var lastArg = arguments[arguments.length - 1];
-  if (typeof lastArg === 'function') {
-    self.once('listening', lastArg);
+  if (typeof cb === 'function') {
+    this.once('listening', cb);
   }
 
-  var port = toNumber(arguments[0]);
+  if (args.length === 0 || typeof args[0] === 'function') {
+    // Bind to a random port.
+    options.port = 0;
+  }
 
   // The third optional argument is the backlog size.
   // When the ip is omitted it can be the second argument.
-  var backlog = toNumber(arguments[1]) || toNumber(arguments[2]);
+  var backlog = toNumber(args.length > 1 && args[1]) ||
+                toNumber(args.length > 2 && args[2]);
 
-  if (arguments.length === 0 || typeof arguments[0] === 'function') {
-    // Bind to a random port.
-    listen(self, null, 0, null, backlog);
-  } else if (arguments[0] !== null && typeof arguments[0] === 'object') {
-    var h = arguments[0];
-    h = h._handle || h.handle || h;
-
-    if (h instanceof TCP) {
-      self._handle = h;
-      listen(self, null, -1, -1, backlog);
-    } else if (typeof h.fd === 'number' && h.fd >= 0) {
-      listen(self, null, null, null, backlog, h.fd);
-    } else {
-      // The first argument is a configuration object
-      if (h.backlog)
-        backlog = h.backlog;
-
-      if (typeof h.port === 'number' || typeof h.port === 'string' ||
-          (typeof h.port === 'undefined' && 'port' in h)) {
-        // Undefined is interpreted as zero (random port) for consistency
-        // with net.connect().
-        assertPort(h.port);
-        if (h.host)
-          listenAfterLookup(h.port | 0, h.host, backlog, h.exclusive);
-        else
-          listen(self, null, h.port | 0, 4, backlog, undefined, h.exclusive);
-      } else if (h.path && isPipeName(h.path)) {
-        const pipeName = self._pipeName = h.path;
-        listen(self, pipeName, -1, -1, backlog, undefined, h.exclusive);
-      } else {
-        throw new Error('Invalid listen argument: ' + h);
-      }
-    }
-  } else if (isPipeName(arguments[0])) {
-    // UNIX socket or Windows pipe.
-    const pipeName = self._pipeName = arguments[0];
-    listen(self, pipeName, -1, -1, backlog);
-
-  } else if (arguments[1] === undefined ||
-             typeof arguments[1] === 'function' ||
-             typeof arguments[1] === 'number') {
-    // The first argument is the port, no IP given.
-    assertPort(port);
-    listen(self, null, port, 4, backlog);
+  options = options._handle || options.handle || options;
 
+  if (options instanceof TCP) {
+    this._handle = options;
+    listen(this, null, -1, -1, backlog);
+  } else if (typeof options.fd === 'number' && options.fd >= 0) {
+    listen(this, null, null, null, backlog, options.fd);
   } else {
-    // The first argument is the port, the second an IP.
-    assertPort(port);
-    listenAfterLookup(port, arguments[1], backlog);
-  }
-
-  function listenAfterLookup(port, address, backlog, exclusive) {
-    require('dns').lookup(address, function(err, ip, addressType) {
-      if (err) {
-        self.emit('error', err);
+    backlog = options.backlog || backlog;
+
+    if (typeof options.port === 'number' || typeof options.port === 'string' ||
+        (typeof options.port === 'undefined' && 'port' in options)) {
+      // Undefined is interpreted as zero (random port) for consistency
+      // with net.connect().
+      assertPort(options.port);
+      if (options.host) {
+        lookupAndListen(this, options.port | 0, options.host, backlog,
+                        options.exclusive);
       } else {
-        addressType = ip ? addressType : 4;
-        listen(self, ip, port, addressType, backlog, undefined, exclusive);
+        listen(this, null, options.port | 0, 4, backlog, undefined,
+               options.exclusive);
       }
-    });
+    } else if (options.path && isPipeName(options.path)) {
+      // UNIX socket or Windows pipe.
+      const pipeName = this._pipeName = options.path;
+      listen(this, pipeName, -1, -1, backlog, undefined, options.exclusive);
+    } else {
+      throw new Error('Invalid listen argument: ' + options);
+    }
   }
 
-  return self;
+  return this;
 };
 
+function lookupAndListen(self, port, address, backlog, exclusive) {
+  require('dns').lookup(address, function(err, ip, addressType) {
+    if (err) {
+      self.emit('error', err);
+    } else {
+      addressType = ip ? addressType : 4;
+      listen(self, ip, port, addressType, backlog, undefined, exclusive);
+    }
+  });
+}
+
 Object.defineProperty(Server.prototype, 'listening', {
   get: function() {
     return !!this._handle;

test/parallel/test-net-listen-port-option.js

@@ -1,28 +1,45 @@
 'use strict';
-var common = require('../common');
-var assert = require('assert');
-var net = require('net');
+const common = require('../common');
+const assert = require('assert');
+const net = require('net');
 
 function close() { this.close(); }
-net.Server().listen({ port: undefined }, close);
-net.Server().listen({ port: '' + common.PORT }, close);
 
-[ 'nan',
-  -1,
-  123.456,
-  0x10000,
-  1 / 0,
-  -1 / 0,
-  '+Infinity',
-  '-Infinity'
-].forEach(function(port) {
-  assert.throws(function() {
-    net.Server().listen({ port: port }, common.fail);
-  }, /"port" argument must be >= 0 and < 65536/i);
-});
+// From lib/net.js
+function toNumber(x) { return (x = Number(x)) >= 0 ? x : false; }
+
+function isPipeName(s) {
+  return typeof s === 'string' && toNumber(s) === false;
+}
+
+const listenVariants = [
+  (port, cb) => net.Server().listen({port}, cb),
+  (port, cb) => net.Server().listen(port, cb)
+];
+
+listenVariants.forEach((listenVariant, i) => {
+  listenVariant(undefined, common.mustCall(close));
+  listenVariant('0', common.mustCall(close));
+
+  [
+    'nan',
+    -1,
+    123.456,
+    0x10000,
+    1 / 0,
+    -1 / 0,
+    '+Infinity',
+    '-Infinity'
+  ].forEach((port) => {
+    if (i === 1 && isPipeName(port)) {
+      // skip this, because listen(port) can also be listen(path)
+      return;
+    }
+    assert.throws(() => listenVariant(port, common.fail),
+                  /"port" argument must be >= 0 and < 65536/i);
+  });
 
-[null, true, false].forEach(function(port) {
-  assert.throws(function() {
-    net.Server().listen({ port: port }, common.fail);
-  }, /invalid listen argument/i);
+  [null, true, false].forEach((port) =>
+    assert.throws(() => listenVariant(port, common.fail),
+                  /invalid listen argument/i));
 });