cluster: fix shared handle bind error propagation

A failed bind() was already being correctly reported in round-robin
mode. This commit fixes bind() error reporting in shared handle mode.

Fixes #5774.

lib/cluster.js

@@ -138,7 +138,11 @@ RoundRobinHandle.prototype.add = function(worker, send) {
   // Still busy binding.
   this.server.once('listening', done);
   this.server.once('error', function(err) {
-    send(err.errno, null);
+    // Hack: translate 'EADDRINUSE' error string back to numeric error code.
+    // It works but ideally we'd have some backchannel between the net and
+    // cluster modules for stuff like this.
+    var errno = process.binding('uv')['UV_' + err.errno];
+    send(errno, null);
   });
 };
 
@@ -388,14 +392,12 @@ function masterInit() {
     // Set custom server data
     handle.add(worker, function(errno, reply, handle) {
       reply = util._extend({
-        ack: message.seq,
+        errno: errno,
         key: key,
+        ack: message.seq,
         data: handles[key].data
       }, reply);
-      if (errno) {
+      if (errno) delete handles[key];  // Gives other workers a chance to retry.
-        reply.errno = errno;
-        delete handles[key];  // Gives other workers a chance to retry.
-      }
       send(worker, reply, handle);
     });
   }
@@ -487,63 +489,52 @@ function workerInit() {
     };
     assert(IS_UNDEFINED(handles[key]));
     handles[key] = handle;
-    cb(handle);
+    cb(message.errno, handle);
   }
 
   // Round-robin. Master distributes handles across workers.
   function rr(message, cb) {
     if (message.errno)
-      onerror(message, cb);
+      return cb(message.errno, null);
-    else
-      onsuccess(message, cb);
 
-    function onerror(message, cb) {
+    var key = message.key;
-      function listen(backlog) {
+    function listen(backlog) {
-        // Translate 'EADDRINUSE' error back to numeric value. This function
+      // TODO(bnoordhuis) Send a message to the master that tells it to
-        // is called as sock._handle.listen().
+      // update the backlog size. The actual backlog should probably be
-        return process.binding('uv')['UV_' + message.errno];
+      // the largest requested size by any worker.
-      }
+      return 0;
-      function close() {
-      }
-      cb({ close: close, listen: listen });
     }
 
-    function onsuccess(message, cb) {
+    function close() {
-      var key = message.key;
+      // lib/net.js treats server._handle.close() as effectively synchronous.
-      function listen(backlog) {
+      // That means there is a time window between the call to close() and
-        // TODO(bnoordhuis) Send a message to the master that tells it to
+      // the ack by the master process in which we can still receive handles.
-        // update the backlog size. The actual backlog should probably be
+      // onconnection() below handles that by sending those handles back to
-        // the largest requested size by any worker.
+      // the master.
-        return 0;
+      if (IS_UNDEFINED(key)) return;
-      }
+      send({ act: 'close', key: key });
-      function close() {
+      delete handles[key];
-        // lib/net.js treats server._handle.close() as effectively synchronous.
+      key = undefined;
-        // That means there is a time window between the call to close() and
-        // the ack by the master process in which we can still receive handles.
-        // onconnection() below handles that by sending those handles back to
-        // the master.
-        if (IS_UNDEFINED(key)) return;
-        send({ act: 'close', key: key });
-        delete handles[key];
-        key = undefined;
-      }
-      function getsockname(out) {
-        if (key) util._extend(out, message.sockname);
-      }
-      // Faux handle. Mimics a TCPWrap with just enough fidelity to get away
-      // with it. Fools net.Server into thinking that it's backed by a real
-      // handle.
-      var handle = {
-        close: close,
-        listen: listen
-      };
-      if (message.sockname) {
-        handle.getsockname = getsockname;  // TCP handles only.
-      }
-      assert(IS_UNDEFINED(handles[key]));
-      handles[key] = handle;
-      cb(handle);
     }
+
+    function getsockname(out) {
+      if (key) util._extend(out, message.sockname);
+      return 0;
+    }
+
+    // Faux handle. Mimics a TCPWrap with just enough fidelity to get away
+    // with it. Fools net.Server into thinking that it's backed by a real
+    // handle.
+    var handle = {
+      close: close,
+      listen: listen
+    };
+    if (message.sockname) {
+      handle.getsockname = getsockname;  // TCP handles only.
+    }
+    assert(IS_UNDEFINED(handles[key]));
+    handles[key] = handle;
+    cb(0, handle);
   }
 
   // Round-robin connection.

lib/dgram.js

@@ -191,7 +191,13 @@ Socket.prototype.bind = function(/*port, address, callback*/) {
       cluster = require('cluster');
 
     if (cluster.isWorker) {
-      cluster._getServer(self, ip, port, self.type, -1, function(handle) {
+      cluster._getServer(self, ip, port, self.type, -1, function(err, handle) {
+        if (err) {
+          self.emit('error', errnoException(err, 'bind'));
+          self._bindState = BIND_STATE_UNBOUND;
+          return;
+        }
+
         if (!self._handle)
           // handle has been closed in the mean time.
           return handle.close();

lib/net.js

@@ -1094,27 +1094,30 @@ function listen(self, address, port, addressType, backlog, fd) {
     return;
   }
 
-  cluster._getServer(self, address, port, addressType, fd, function(handle) {
+  cluster._getServer(self, address, port, addressType, fd, cb);
-    // Some operating systems (notably OS X and Solaris) don't report EADDRINUSE
+
-    // errors right away. libuv mimics that behavior for the sake of platform
+  function cb(err, handle) {
-    // consistency but that means we have have a socket on our hands that is
+    // EADDRINUSE may not be reported until we call listen(). To complicate
-    // not actually bound. That's why we check if the actual port matches what
+    // matters, a failed bind() followed by listen() will implicitly bind to
-    // we requested and if not, raise an error. The exception is when port == 0
+    // a random port. Ergo, check that the socket is bound to the expected
-    // because that means "any random port".
+    // port before calling listen().
-    if (port && handle.getsockname) {
+    //
+    // FIXME(bnoordhuis) Doesn't work for pipe handles, they don't have a
+    // getsockname() method. Non-issue for now, the cluster module doesn't
+    // really support pipes anyway.
+    if (err === 0 && port > 0 && handle.getsockname) {
       var out = {};
-      var err = handle.getsockname(out);
+      err = handle.getsockname(out);
-      if (err === 0 && port !== out.port) {
+      if (err === 0 && port !== out.port)
         err = uv.UV_EADDRINUSE;
-      }
-      if (err) {
-        return self.emit('error', errnoException(err, 'bind'));
-      }
     }
 
+    if (err)
+      return self.emit('error', errnoException(err, 'bind'));
+
     self._handle = handle;
     self._listen2(address, port, addressType, backlog, fd);
-  });
+  }
 }
 
 

test/simple/test-cluster-shared-handle-bind-error.js

@@ -0,0 +1,47 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var cluster = require('cluster');
+var net = require('net');
+
+if (cluster.isMaster) {
+  // Master opens and binds the socket and shares it with the worker.
+  cluster.schedulingPolicy = cluster.SCHED_NONE;
+  // Hog the TCP port so that when the worker tries to bind, it'll fail.
+  net.createServer(assert.fail).listen(common.PORT, function() {
+    var server = this;
+    var worker = cluster.fork();
+    worker.on('exit', common.mustCall(function(exitCode) {
+      assert.equal(exitCode, 0);
+      server.close();
+    }));
+  });
+}
+else {
+  var s = net.createServer(assert.fail);
+  s.listen(common.PORT, assert.fail.bind(null, 'listen should have failed'));
+  s.on('error', common.mustCall(function(err) {
+    assert.equal(err.code, 'EADDRINUSE');
+    process.disconnect();
+  }));
+}