From 532d9929c7e6eba8c35943f45dffc93926e34cb9 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 9 Feb 2012 06:22:50 +0100 Subject: [PATCH] cluster: propagate bind errors This commit fixes a bug where the cluster module fails to propagate EADDRINUSE errors. When a worker starts a (net, http) server, it requests the listen socket from its master who then creates and binds the socket. Now, OS X and Windows don't always signal EADDRINUSE from bind() but instead defer the error until a later syscall. libuv mimics this behaviour to provide consistent behaviour across platforms but that means the worker could end up with a socket that is not actually bound to the requested addresss. That's why the worker now checks if the socket is bound, raising EADDRINUSE if that's not the case. Fixes #2721. --- lib/net.js | 24 +++-- ...bind-twice.js => test-cluster-bind-twice-v1.js} | 0 test/simple/test-cluster-bind-twice-v2.js | 115 +++++++++++++++++++++ 3 files changed, 133 insertions(+), 6 deletions(-) rename test/simple/{test-cluster-bind-twice.js => test-cluster-bind-twice-v1.js} (100%) create mode 100644 test/simple/test-cluster-bind-twice-v2.js diff --git a/lib/net.js b/lib/net.js index c8d7f60..3898521 100644 --- a/lib/net.js +++ b/lib/net.js @@ -928,14 +928,26 @@ Server.prototype._listen2 = function(address, port, addressType, backlog, fd) { function listen(self, address, port, addressType, backlog, fd) { if (!cluster) cluster = require('cluster'); - if (cluster.isWorker) { - cluster._getServer(self, address, port, addressType, fd, function(handle) { - self._handle = handle; - self._listen2(address, port, addressType, backlog, fd); - }); - } else { + if (cluster.isMaster) { self._listen2(address, port, addressType, backlog, fd); + return; } + + cluster._getServer(self, address, port, addressType, fd, function(handle) { + // Some operating systems (notably OS X and Solaris) don't report EADDRINUSE + // errors right away. libuv mimics that behavior for the sake of platform + // consistency but that means we have have a socket on our hands that is + // not actually bound. That's why we check if the actual port matches what + // we requested and if not, raise an error. The exception is when port == 0 + // because that means "any random port". + if (port && handle.getsockname && port != handle.getsockname().port) { + self.emit('error', errnoException('EADDRINUSE', 'bind')); + return; + } + + self._handle = handle; + self._listen2(address, port, addressType, backlog, fd); + }); } diff --git a/test/simple/test-cluster-bind-twice.js b/test/simple/test-cluster-bind-twice-v1.js similarity index 100% rename from test/simple/test-cluster-bind-twice.js rename to test/simple/test-cluster-bind-twice-v1.js diff --git a/test/simple/test-cluster-bind-twice-v2.js b/test/simple/test-cluster-bind-twice-v2.js new file mode 100644 index 0000000..87a932b --- /dev/null +++ b/test/simple/test-cluster-bind-twice-v2.js @@ -0,0 +1,115 @@ +// 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. + +// This test starts two clustered HTTP servers on the same port. It expects the +// first cluster to succeed and the second cluster to fail with EADDRINUSE. +// +// The test may seem complex but most of it is plumbing that routes messages +// from the child processes back to the supervisor. As a tree it looks something +// like this: +// +// +// / \ +// +// / \ +// +// +// The first worker starts a server on a fixed port and fires a ready message +// that is routed to the second worker. When it tries to bind, it expects to +// see an EADDRINUSE error. +// +// See https://github.com/joyent/node/issues/2721 for more details. + +var common = require('../common'); +var assert = require('assert'); +var cluster = require('cluster'); +var fork = require('child_process').fork; +var http = require('http'); + +var id = process.argv[2]; + +if (!id) { + var a = fork(__filename, ['one']); + var b = fork(__filename, ['two']); + + a.on('message', function(m) { + if (typeof m === 'object') return; + assert.equal(m, 'READY'); + b.send('START'); + }); + + var ok = false; + + b.on('message', function(m) { + if (typeof m === 'object') return; // ignore system messages + assert.equal(m, 'EADDRINUSE'); + a.kill(); + b.kill(); + ok = true; + }); + + process.on('exit', function() { + a.kill(); + b.kill(); + assert(ok); + }); +} +else if (id === 'one') { + if (cluster.isMaster) return startWorker(); + + http.createServer(assert.fail).listen(common.PORT, function() { + process.send('READY'); + }); +} +else if (id === 'two') { + if (cluster.isMaster) return startWorker(); + + var ok = false; + process.on('SIGTERM', process.exit); + process.on('exit', function() { + assert(ok); + }); + + process.on('message', function(m) { + if (typeof m === 'object') return; // ignore system messages + assert.equal(m, 'START'); + var server = http.createServer(assert.fail); + server.listen(common.PORT, assert.fail); + server.on('error', function(e) { + assert.equal(e.code, 'EADDRINUSE'); + process.send(e.code); + ok = true; + }); + }); +} +else { + assert(0); // bad command line argument +} + +function startWorker() { + var worker = cluster.fork(); + worker.on('message', process.send.bind(process)); + process.on('message', worker.send.bind(worker)); + process.on('SIGTERM', function() { + worker.destroy(); + process.exit(); + }); +} -- 2.7.4