From fe2caf6fb587a8373a4e45476a9c86f6b0993861 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Thu, 1 Jun 2017 13:07:20 -0400 Subject: [PATCH] test: fix test-inspector-port-zero-cluster * re-implemented test to parse args instead of post binding (exit 12) * saved failing case as known issue PR-URL: https://github.com/nodejs/node/pull/13373 Fixes: https://github.com/nodejs/node/issues/13343 Reviewed-By: James M Snell --- test/inspector/inspector.status | 1 - .../test-inspector-port-zero-cluster.js | 59 ++++++++++------- .../test-inspector-cluster-port-clash.js | 63 +++++++++++++++++++ 3 files changed, 101 insertions(+), 22 deletions(-) create mode 100644 test/known_issues/test-inspector-cluster-port-clash.js diff --git a/test/inspector/inspector.status b/test/inspector/inspector.status index 070d817b2c3ab2..ed6a782b9031a7 100644 --- a/test/inspector/inspector.status +++ b/test/inspector/inspector.status @@ -5,6 +5,5 @@ prefix inspector # sample-test : PASS,FLAKY [true] # This section applies to all platforms -test-inspector-port-zero-cluster : PASS,FLAKY [$system==win32] diff --git a/test/inspector/test-inspector-port-zero-cluster.js b/test/inspector/test-inspector-port-zero-cluster.js index 4582b4bb38657b..f64e05f314c0c6 100644 --- a/test/inspector/test-inspector-port-zero-cluster.js +++ b/test/inspector/test-inspector-port-zero-cluster.js @@ -4,31 +4,48 @@ const common = require('../common'); common.skipIfInspectorDisabled(); +// Assert that even when started with `--inspect=0` workers are assigned +// consecutive (i.e. deterministically predictable) debug ports + const assert = require('assert'); const cluster = require('cluster'); -if (cluster.isMaster) { - const ports = []; - for (const worker of [cluster.fork(), - cluster.fork(), - cluster.fork()]) { - worker.on('message', common.mustCall((message) => { - ports.push(message.debugPort); - worker.kill(); +function serialFork() { + return new Promise((res) => { + const worker = cluster.fork(); + worker.on('exit', common.mustCall((code, signal) => { + // code 0 is normal + // code 12 can happen if inspector could not bind because of a port clash + if (code !== 0 && code !== 12) + assert.fail(`code: ${code}, signal: ${signal}`); + const port = worker.process.spawnargs + .map((a) => (/=(?:.*:)?(\d{2,5})$/.exec(a) || [])[1]) + .filter((p) => p) + .pop(); + res(Number(port)); })); - worker.send('debugPort'); - } - process.on('exit', () => { - ports.sort(); - assert.strictEqual(ports.length, 3); - assert(ports.every((port) => port > 0)); - assert(ports.every((port) => port < 65536)); - assert.strictEqual(ports[0] + 1, ports[1]); // Ports should be consecutive. - assert.strictEqual(ports[1] + 1, ports[2]); }); +} + +if (cluster.isMaster) { + Promise.all([serialFork(), serialFork(), serialFork()]) + .then(common.mustCall((ports) => { + ports.push(process.debugPort); + ports.sort(); + // 4 = [master, worker1, worker2, worker3].length() + assert.strictEqual(ports.length, 4); + assert(ports.every((port) => port > 0)); + assert(ports.every((port) => port < 65536)); + // Ports should be consecutive. + assert.strictEqual(ports[0] + 1, ports[1]); + assert.strictEqual(ports[1] + 1, ports[2]); + assert.strictEqual(ports[2] + 1, ports[3]); + })) + .catch( + (err) => { + console.error(err); + process.exit(1); + }); } else { - process.on('message', (message) => { - if (message === 'debugPort') - process.send({ debugPort: process.debugPort }); - }); + process.exit(0); } diff --git a/test/known_issues/test-inspector-cluster-port-clash.js b/test/known_issues/test-inspector-cluster-port-clash.js new file mode 100644 index 00000000000000..41b00aacc148be --- /dev/null +++ b/test/known_issues/test-inspector-cluster-port-clash.js @@ -0,0 +1,63 @@ +// Flags: --inspect=0 +'use strict'; +const common = require('../common'); + +// With the current behavior of Node.js (at least as late as 8.1.0), this +// test fails with the following error: +// `AssertionError [ERR_ASSERTION]: worker 2 failed to bind port` +// Ideally, there would be a way for the user to opt out of sequential port +// assignment. +// +// Refs: https://github.com/nodejs/node/issues/13343 + +common.skipIfInspectorDisabled(); + +const assert = require('assert'); +const cluster = require('cluster'); +const net = require('net'); + +const ports = [process.debugPort]; +const clashPort = process.debugPort + 2; +function serialFork() { + return new Promise((res) => { + const worker = cluster.fork(); + worker.on('error', (err) => assert.fail(err)); + // no common.mustCall since 1 out of 3 should fail + worker.on('online', () => { + worker.on('message', common.mustCall((message) => { + ports.push(message.debugPort); + })); + }); + worker.on('exit', common.mustCall((code, signal) => { + assert.strictEqual(signal, null); + // worker 2 should fail because of port clash with `server` + if (code === 12) { + return assert.fail(`worker ${worker.id} failed to bind port`); + } + assert.strictEqual(0, code); + })); + worker.on('disconnect', common.mustCall(res)); + }); +} + +if (cluster.isMaster) { + cluster.on('online', common.mustCall((worker) => worker.send('dbgport'), 2)); + + // block one of the ports with a listening socket + const server = net.createServer(); + server.listen(clashPort, common.localhostIPv4, common.mustCall(() => { + // try to fork 3 workers No.2 should fail + Promise.all([serialFork(), serialFork(), serialFork()]) + .then(common.mustNotCall()) + .catch((err) => console.error(err)); + })); + server.unref(); +} else { + const sentinel = common.mustCall(); + process.on('message', (message) => { + if (message !== 'dbgport') return; + process.send({ debugPort: process.debugPort }); + sentinel(); + process.disconnect(); + }); +}