From 5dcd307a6bfcdc92d96429cbcc72ff3cf4159903 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 14 Aug 2014 20:02:13 -0400 Subject: [PATCH 1/3] net: make Socket.connect() consistently asynchronous `Socket.prototype.connect()` establishes a connection synchronously in some cases and asynchronously in others. This commit makes it consistently asynchronous. This commit also removes hard coded IPv4 addresses, and throws on bad inputs immediately, instead of potentially after an asynchronous operation. --- lib/net.js | 85 +++++++++--------------- test/simple/test-net-localerror.js | 47 ++++--------- test/simple/test-process-active-wraps.js | 15 ++++- 3 files changed, 59 insertions(+), 88 deletions(-) diff --git a/lib/net.js b/lib/net.js index 5f3247349218..f13683c0c9a5 100644 --- a/lib/net.js +++ b/lib/net.js @@ -769,34 +769,18 @@ function connect(self, address, port, addressType, localAddress, localPort) { assert.ok(self._connecting); var err; - if (localAddress || localPort) { - if (localAddress && !exports.isIP(localAddress)) - err = new TypeError( - 'localAddress should be a valid IP: ' + localAddress); - - if (localPort && !util.isNumber(localPort)) - err = new TypeError('localPort should be a number: ' + localPort); + if (localAddress || localPort) { var bind; - switch (addressType) { - case 4: - if (!localAddress) - localAddress = '0.0.0.0'; - bind = self._handle.bind; - break; - case 6: - if (!localAddress) - localAddress = '::'; - bind = self._handle.bind6; - break; - default: - err = new TypeError('Invalid addressType: ' + addressType); - break; - } - - if (err) { - self._destroy(err); + if (addressType === 4) { + localAddress = localAddress || '0.0.0.0'; + bind = self._handle.bind; + } else if (addressType === 6) { + localAddress = localAddress || '::'; + bind = self._handle.bind6; + } else { + self._destroy(new TypeError('Invalid addressType: ' + addressType)); return; } @@ -814,16 +798,11 @@ function connect(self, address, port, addressType, localAddress, localPort) { } var req = { oncomplete: afterConnect }; - if (addressType === 6 || addressType === 4) { - port = port | 0; - if (port <= 0 || port > 65535) - throw new RangeError('Port should be > 0 and < 65536'); - if (addressType === 6) { - err = self._handle.connect6(req, address, port); - } else if (addressType === 4) { - err = self._handle.connect(req, address, port); - } + if (addressType === 4) { + err = self._handle.connect(req, address, port); + } else if (addressType === 6) { + err = self._handle.connect6(req, address, port); } else { err = self._handle.connect(req, address, afterConnect); } @@ -872,26 +851,33 @@ Socket.prototype.connect = function(options, cb) { } timers._unrefActive(this); - self._connecting = true; self.writable = true; if (pipe) { - connect(self, options.path); - - } else if (!options.host) { - debug('connect: missing host'); - self._host = '127.0.0.1'; - connect(self, self._host, options.port, 4); - + process.nextTick(function() { + connect(self, options.path); + }); } else { var dns = require('dns'); - var host = options.host; + var host = options.host || 'localhost'; + var port = options.port | 0; + var localAddress = options.localAddress; + var localPort = options.localPort; var dnsopts = { family: options.family, hints: 0 }; + if (localAddress && !exports.isIP(localAddress)) + throw new TypeError('localAddress must be a valid IP: ' + localAddress); + + if (localPort && !util.isNumber(localPort)) + throw new TypeError('localPort should be a number: ' + localPort); + + if (port <= 0 || port > 65535) + throw new RangeError('Port should be > 0 and < 65536'); + if (dnsopts.family !== 4 && dnsopts.family !== 6) dnsopts.hints = dns.ADDRCONFIG | dns.V4MAPPED; @@ -917,19 +903,12 @@ Socket.prototype.connect = function(options, cb) { }); } else { timers._unrefActive(self); - - addressType = addressType || 4; - - // node_net.cc handles null host names graciously but user land - // expects remoteAddress to have a meaningful value - ip = ip || (addressType === 4 ? '127.0.0.1' : '0:0:0:0:0:0:0:1'); - connect(self, ip, - options.port, + port, addressType, - options.localAddress, - options.localPort); + localAddress, + localPort); } }); } diff --git a/test/simple/test-net-localerror.js b/test/simple/test-net-localerror.js index c4d04aa921ac..29b471887781 100644 --- a/test/simple/test-net-localerror.js +++ b/test/simple/test-net-localerror.js @@ -23,39 +23,20 @@ var common = require('../common'); var assert = require('assert'); var net = require('net'); -var server = net.createServer(function(socket) { - assert.ok(false, 'no clients should connect'); -}).listen(common.PORT).on('listening', function() { - server.unref(); + connect({ + host: 'localhost', + port: common.PORT, + localPort: 'foobar', + }, 'localPort should be a number: foobar'); - function test1(next) { - connect({ - host: '127.0.0.1', - port: common.PORT, - localPort: 'foobar', - }, - 'localPort should be a number: foobar', - next); - } + connect({ + host: 'localhost', + port: common.PORT, + localAddress: 'foobar', + }, 'localAddress should be a valid IP: foobar'); - function test2(next) { - connect({ - host: '127.0.0.1', - port: common.PORT, - localAddress: 'foobar', - }, - 'localAddress should be a valid IP: foobar', - next) - } - - test1(test2); -}) - -function connect(opts, msg, cb) { - var client = net.connect(opts).on('connect', function() { - assert.ok(false, 'we should never connect'); - }).on('error', function(err) { - assert.strictEqual(err.message, msg); - if (cb) cb(); - }); +function connect(opts, msg) { + assert.throws(function() { + var client = net.connect(opts); + }, msg); } diff --git a/test/simple/test-process-active-wraps.js b/test/simple/test-process-active-wraps.js index 63fc218debc4..1ce29e6cdb8d 100644 --- a/test/simple/test-process-active-wraps.js +++ b/test/simple/test-process-active-wraps.js @@ -41,9 +41,16 @@ var handles = []; })(); (function() { + function onlookup() { + setImmediate(function() { + assert.equal(process._getActiveRequests().length, 0); + }); + }; + expect(1, 0); var conn = net.createConnection(common.PORT); - conn.on('error', function() { /* ignore */ }); + conn.on('lookup', onlookup); + conn.on('error', function() { assert(false); }); expect(2, 1); conn.destroy(); expect(2, 1); // client handle doesn't shut down until next tick @@ -52,10 +59,14 @@ var handles = []; (function() { var n = 0; + handles.forEach(function(handle) { handle.once('close', onclose); }); function onclose() { - if (++n === handles.length) setImmediate(expect, 0, 0); + if (++n === handles.length) + setImmediate(function() { + assert.equal(process._getActiveHandles().length, 0); + }); } })(); From 5f88d41c480882d942349ae15aac440ed9425624 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Wed, 27 Aug 2014 10:46:55 -0400 Subject: [PATCH 2/3] update based on comments on #8180 --- lib/net.js | 6 +++--- test/simple/test-process-active-wraps.js | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/net.js b/lib/net.js index f13683c0c9a5..ba538800013b 100644 --- a/lib/net.js +++ b/lib/net.js @@ -851,13 +851,13 @@ Socket.prototype.connect = function(options, cb) { } timers._unrefActive(this); + self._connecting = true; self.writable = true; if (pipe) { - process.nextTick(function() { - connect(self, options.path); - }); + connect(self, options.path); + } else { var dns = require('dns'); var host = options.host || 'localhost'; diff --git a/test/simple/test-process-active-wraps.js b/test/simple/test-process-active-wraps.js index 1ce29e6cdb8d..bd4941b786e7 100644 --- a/test/simple/test-process-active-wraps.js +++ b/test/simple/test-process-active-wraps.js @@ -64,9 +64,10 @@ var handles = []; handle.once('close', onclose); }); function onclose() { - if (++n === handles.length) + if (++n === handles.length) { setImmediate(function() { assert.equal(process._getActiveHandles().length, 0); }); + } } })(); From 9983e769f7e0eabf88f264e8a0d44b55fc015aa2 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Mon, 3 Nov 2014 17:37:54 -0500 Subject: [PATCH 3/3] improve port check error message and add tests --- lib/net.js | 2 +- test/simple/test-net-localerror.js | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/net.js b/lib/net.js index ba538800013b..ed9bca5098c4 100644 --- a/lib/net.js +++ b/lib/net.js @@ -876,7 +876,7 @@ Socket.prototype.connect = function(options, cb) { throw new TypeError('localPort should be a number: ' + localPort); if (port <= 0 || port > 65535) - throw new RangeError('Port should be > 0 and < 65536'); + throw new RangeError('port should be > 0 and < 65536: ' + port); if (dnsopts.family !== 4 && dnsopts.family !== 6) dnsopts.hints = dns.ADDRCONFIG | dns.V4MAPPED; diff --git a/test/simple/test-net-localerror.js b/test/simple/test-net-localerror.js index 29b471887781..d04d9c707204 100644 --- a/test/simple/test-net-localerror.js +++ b/test/simple/test-net-localerror.js @@ -35,6 +35,16 @@ var net = require('net'); localAddress: 'foobar', }, 'localAddress should be a valid IP: foobar'); + connect({ + host: 'localhost', + port: 65536 + }, 'port should be > 0 and < 65536: 65536'); + + connect({ + host: 'localhost', + port: 0 + }, 'port should be > 0 and < 65536: 0'); + function connect(opts, msg) { assert.throws(function() { var client = net.connect(opts);