From fd6af98c2d623c87f910437086ea04c7be195df8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Sch=C3=A4r?= Date: Fri, 12 Aug 2016 18:22:22 +0200 Subject: [PATCH] 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 Reviewed-By: Glen Keane Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- lib/_tls_wrap.js | 2 +- lib/net.js | 127 +++++++++---------- test/parallel/test-net-listen-port-option.js | 61 +++++---- 3 files changed, 98 insertions(+), 92 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index c2ecd07a4b77bc..e1e4ab8af64ca6 100644 --- a/lib/_tls_wrap.js +++ b/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]; diff --git a/lib/net.js b/lib/net.js index 298e7186aa6d60..8a7e3c2ad2de67 100644 --- a/lib/net.js +++ b/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; diff --git a/test/parallel/test-net-listen-port-option.js b/test/parallel/test-net-listen-port-option.js index 77c9cb42f0a930..028ecdde784440 100644 --- a/test/parallel/test-net-listen-port-option.js +++ b/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)); });