net: cleanup connect logic

Separates out the lookup logic for net.Socket. In the event the `host` property is an IP address, the lookup is skipped. PR-URL: nodejs#1505 Reviewed-By: Chris Dickinson <[email protected]> Reviewed-By: Yosuke Furukawa <[email protected]>
mscdex · Apr 24, 2015 · 1bef717 · 1bef717
1 parent 3d3083b
commit 1bef717
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 54 deletions.
diff --git a/lib/net.js b/lib/net.js
@@ -881,64 +881,77 @@ Socket.prototype.connect = function(options, cb) {
     connect(self, options.path);
 
   } else {
-    const dns = require('dns');
-    var host = options.host || 'localhost';
-    var 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 && typeof localPort !== 'number')
-      throw new TypeError('localPort should be a number: ' + localPort);
-
-    port = options.port;
-    if (typeof port !== 'undefined') {
-      if (typeof port !== 'number' && typeof port !== 'string')
-        throw new TypeError('port should be a number or string: ' + port);
-      if (!isLegalPort(port))
-        throw new RangeError('port should be >= 0 and < 65536: ' + port);
-    }
-    port |= 0;
+    lookupAndConnect(self, options);
+  }
+  return self;
+};
 
-    if (dnsopts.family !== 4 && dnsopts.family !== 6)
-      dnsopts.hints = dns.ADDRCONFIG | dns.V4MAPPED;
 
-    debug('connect: find host ' + host);
-    debug('connect: dns options ' + dnsopts);
-    self._host = host;
-    dns.lookup(host, dnsopts, function(err, ip, addressType) {
-      self.emit('lookup', err, ip, addressType);
+function lookupAndConnect(self, options) {
+  const dns = require('dns');
+  var host = options.host || 'localhost';
+  var port = options.port;
+  var localAddress = options.localAddress;
+  var localPort = options.localPort;
 
-      // It's possible we were destroyed while looking this up.
-      // XXX it would be great if we could cancel the promise returned by
-      // the look up.
-      if (!self._connecting) return;
+  if (localAddress && !exports.isIP(localAddress))
+    throw new TypeError('localAddress must be a valid IP: ' + localAddress);
 
-      if (err) {
-        // net.createConnection() creates a net.Socket object and
-        // immediately calls net.Socket.connect() on it (that's us).
-        // There are no event listeners registered yet so defer the
-        // error event to the next tick.
-        process.nextTick(connectErrorNT, self, err, options);
-      } else {
-        self._unrefTimer();
-        connect(self,
-                ip,
-                port,
-                addressType,
-                localAddress,
-                localPort);
-      }
-    });
+  if (localPort && typeof localPort !== 'number')
+    throw new TypeError('localPort should be a number: ' + localPort);
+
+  if (typeof port !== 'undefined') {
+    if (typeof port !== 'number' && typeof port !== 'string')
+      throw new TypeError('port should be a number or string: ' + port);
+    if (!isLegalPort(port))
+      throw new RangeError('port should be >= 0 and < 65536: ' + port);
   }
-  return self;
-};
+  port |= 0;
+
+  // If host is an IP, skip performing a lookup
+  // TODO(evanlucas) should we hot path this for localhost?
+  var addressType = exports.isIP(host);
+  if (addressType) {
+    connect(self, host, port, addressType, localAddress, localPort);
+    return;
+  }
+
+  var dnsopts = {
+    family: options.family,
+    hints: 0
+  };
+
+  if (dnsopts.family !== 4 && dnsopts.family !== 6)
+    dnsopts.hints = dns.ADDRCONFIG | dns.V4MAPPED;
+
+  debug('connect: find host ' + host);
+  debug('connect: dns options ' + dnsopts);
+  self._host = host;
+  dns.lookup(host, dnsopts, function(err, ip, addressType) {
+    self.emit('lookup', err, ip, addressType);
+
+    // It's possible we were destroyed while looking this up.
+    // XXX it would be great if we could cancel the promise returned by
+    // the look up.
+    if (!self._connecting) return;
+
+    if (err) {
+      // net.createConnection() creates a net.Socket object and
+      // immediately calls net.Socket.connect() on it (that's us).
+      // There are no event listeners registered yet so defer the
+      // error event to the next tick.
+      process.nextTick(connectErrorNT, self, err, options);
+    } else {
+      self._unrefTimer();
+      connect(self,
+              ip,
+              port,
+              addressType,
+              localAddress,
+              localPort);
+    }
+  });
+}
 
 
 function connectErrorNT(self, err, options) {

diff --git a/test/parallel/test-net-dns-lookup-skip.js b/test/parallel/test-net-dns-lookup-skip.js
@@ -0,0 +1,18 @@
+var common = require('../common');
+var assert = require('assert');
+var net = require('net');
+
+function check(addressType) {
+  var server = net.createServer(function(client) {
+    client.end();
+    server.close();
+  });
+
+  var address = addressType === 4 ? '127.0.0.1' : '::1';
+  server.listen(common.PORT, address, function() {
+    net.connect(common.PORT, address).on('lookup', assert.fail);
+  });
+}
+
+check(4);
+common.hasIPv6 && check(6);
diff --git a/test/parallel/test-net-dns-lookup.js b/test/parallel/test-net-dns-lookup.js
@@ -9,7 +9,7 @@ var server = net.createServer(function(client) {
 });
 
 server.listen(common.PORT, '127.0.0.1', function() {
-  net.connect(common.PORT, '127.0.0.1').on('lookup', function(err, ip, type) {
+  net.connect(common.PORT, 'localhost').on('lookup', function(err, ip, type) {
     assert.equal(err, null);
     assert.equal(ip, '127.0.0.1');
     assert.equal(type, '4');