From c623001f976e2459f65fc0bff376fd6c55e00ee8 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 25 Nov 2015 19:41:09 -0800 Subject: [PATCH 01/12] test: eliminate multicast test FreeBSD flakiness test-dgram-multicast-multi-process was flaky on FreeBSD and Raspeberry Pi. This refactoring fixes the issue by eliminating a race condition. Fixes: https://github.com/nodejs/node/issues/2474 --- .../test-dgram-multicast-multi-process.js | 275 +++++++++--------- 1 file changed, 136 insertions(+), 139 deletions(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 0bef2e1dc028c1..99de541dab7d30 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -1,18 +1,109 @@ 'use strict'; -var common = require('../common'), - assert = require('assert'), - dgram = require('dgram'), - util = require('util'), - Buffer = require('buffer').Buffer, - fork = require('child_process').fork, - LOCAL_BROADCAST_HOST = '224.0.0.114', - TIMEOUT = common.platformTimeout(5000), - messages = [ - new Buffer('First message to send'), - new Buffer('Second message to send'), - new Buffer('Third message to send'), - new Buffer('Fourth message to send') - ]; +const common = require('../common'), + assert = require('assert'), + dgram = require('dgram'), + Buffer = require('buffer').Buffer, + fork = require('child_process').fork, + LOCAL_BROADCAST_HOST = '224.0.0.114', + TIMEOUT = common.platformTimeout(5000), + messages = [ + new Buffer('First message to send'), + new Buffer('Second message to send'), + new Buffer('Third message to send'), + new Buffer('Fourth message to send') + ]; + +function launchChildProcess(index) { + const worker = fork(process.argv[1], ['child']); + workers[worker.pid] = worker; + + worker.messagesReceived = []; + + //handle the death of workers + worker.on('exit', function(code, signal) { + // don't consider this the true death if the + // worker has finished successfully + + // or if the exit code is 0 + if (worker.isDone || code === 0) { + return; + } + + dead += 1; + console.error('[PARENT] Worker %d died. %d dead of %d', + worker.pid, + dead, + listeners); + + if (dead === listeners) { + console.error('[PARENT] All workers have died.'); + console.error('[PARENT] Fail'); + + killChildren(workers); + + process.exit(1); + } + }); + + worker.on('message', function(msg) { + if (msg.listening) { + listening += 1; + + if (listening === listeners) { + //all child process are listening, so start sending + sendSocket.sendNext(); + } + } + else if (msg.message) { + worker.messagesReceived.push(msg.message); + + if (worker.messagesReceived.length === messages.length) { + done += 1; + worker.isDone = true; + console.error('[PARENT] %d received %d messages total.', + worker.pid, + worker.messagesReceived.length); + } + + if (done === listeners) { + console.error('[PARENT] All workers have received the ' + + 'required number of messages. Will now compare.'); + + Object.keys(workers).forEach(function(pid) { + const worker = workers[pid]; + + var count = 0; + + worker.messagesReceived.forEach(function(buf) { + for (var i = 0; i < messages.length; ++i) { + if (buf.toString() === messages[i].toString()) { + count++; + break; + } + } + }); + + console.error('[PARENT] %d received %d matching messages.', + worker.pid, count); + + assert.equal(count, messages.length, + 'A worker received an invalid multicast message'); + }); + + clearTimeout(timer); + console.error('[PARENT] Success'); + killChildren(workers); + } + } + }); +} + +function killChildren(children) { + Object.keys(children).forEach(function(key) { + const child = children[key]; + child.kill(); + }); +} if (process.argv[2] !== 'child') { var workers = {}, @@ -20,11 +111,10 @@ if (process.argv[2] !== 'child') { listening = 0, dead = 0, i = 0, - done = 0, - timer = null; + done = 0; //exit the test if it doesn't succeed within TIMEOUT - timer = setTimeout(function() { + var timer = setTimeout(function() { console.error('[PARENT] Responses were not received within %d ms.', TIMEOUT); console.error('[PARENT] Fail'); @@ -36,90 +126,7 @@ if (process.argv[2] !== 'child') { //launch child processes for (var x = 0; x < listeners; x++) { - (function() { - var worker = fork(process.argv[1], ['child']); - workers[worker.pid] = worker; - - worker.messagesReceived = []; - - //handle the death of workers - worker.on('exit', function(code, signal) { - // don't consider this the true death if the - // worker has finished successfully - - // or if the exit code is 0 - if (worker.isDone || code === 0) { - return; - } - - dead += 1; - console.error('[PARENT] Worker %d died. %d dead of %d', - worker.pid, - dead, - listeners); - - if (dead === listeners) { - console.error('[PARENT] All workers have died.'); - console.error('[PARENT] Fail'); - - killChildren(workers); - - process.exit(1); - } - }); - - worker.on('message', function(msg) { - if (msg.listening) { - listening += 1; - - if (listening === listeners) { - //all child process are listening, so start sending - sendSocket.sendNext(); - } - } - else if (msg.message) { - worker.messagesReceived.push(msg.message); - - if (worker.messagesReceived.length === messages.length) { - done += 1; - worker.isDone = true; - console.error('[PARENT] %d received %d messages total.', - worker.pid, - worker.messagesReceived.length); - } - - if (done === listeners) { - console.error('[PARENT] All workers have received the ' + - 'required number of messages. Will now compare.'); - - Object.keys(workers).forEach(function(pid) { - var worker = workers[pid]; - - var count = 0; - - worker.messagesReceived.forEach(function(buf) { - for (var i = 0; i < messages.length; ++i) { - if (buf.toString() === messages[i].toString()) { - count++; - break; - } - } - }); - - console.error('[PARENT] %d received %d matching messages.', - worker.pid, count); - - assert.equal(count, messages.length, - 'A worker received an invalid multicast message'); - }); - - clearTimeout(timer); - console.error('[PARENT] Success'); - killChildren(workers); - } - } - }); - })(x); + launchChildProcess(x); } var sendSocket = dgram.createSocket('udp4'); @@ -141,7 +148,7 @@ if (process.argv[2] !== 'child') { }); sendSocket.sendNext = function() { - var buf = messages[i++]; + const buf = messages[i++]; if (!buf) { try { sendSocket.close(); } catch (e) {} @@ -151,61 +158,51 @@ if (process.argv[2] !== 'child') { sendSocket.send(buf, 0, buf.length, common.PORT, LOCAL_BROADCAST_HOST, function(err) { if (err) throw err; - console.error('[PARENT] sent %s to %s:%s', - util.inspect(buf.toString()), + console.error('[PARENT] sent "%s" to %s:%s', + buf.toString(), LOCAL_BROADCAST_HOST, common.PORT); process.nextTick(sendSocket.sendNext); }); }; - - function killChildren(children) { - Object.keys(children).forEach(function(key) { - var child = children[key]; - child.kill(); - }); - } } if (process.argv[2] === 'child') { - var receivedMessages = []; - var listenSocket = dgram.createSocket({ + const receivedMessages = []; + const listenSocket = dgram.createSocket({ type: 'udp4', reuseAddr: true }); - listenSocket.on('message', function(buf, rinfo) { - console.error('[CHILD] %s received %s from %j', process.pid, - util.inspect(buf.toString()), rinfo); + listenSocket.on('listening', function() { + listenSocket.addMembership(LOCAL_BROADCAST_HOST); - receivedMessages.push(buf); + listenSocket.on('message', function(buf, rinfo) { + console.error('[CHILD] %s received "%s" from %j', process.pid, + buf.toString(), rinfo); - process.send({ message: buf.toString() }); + receivedMessages.push(buf); - if (receivedMessages.length == messages.length) { - // .dropMembership() not strictly needed but here as a sanity check - listenSocket.dropMembership(LOCAL_BROADCAST_HOST); - process.nextTick(function() { - listenSocket.close(); - }); - } - }); + process.send({ message: buf.toString() }); - listenSocket.on('close', function() { - //HACK: Wait to exit the process to ensure that the parent - //process has had time to receive all messages via process.send() - //This may be indicitave of some other issue. - setTimeout(function() { - process.exit(); - }, 1000); - }); + if (receivedMessages.length == messages.length) { + // .dropMembership() not strictly needed but here as a sanity check + listenSocket.dropMembership(LOCAL_BROADCAST_HOST); + process.nextTick(function() { + listenSocket.close(); + }); + } + }); - listenSocket.on('listening', function() { + listenSocket.on('close', function() { + //HACK: Wait to exit the process to ensure that the parent + //process has had time to receive all messages via process.send() + //This may be indicitave of some other issue. + setTimeout(function() { + process.exit(); + }, common.platformTimeout(1000)); + }); process.send({ listening: true }); }); listenSocket.bind(common.PORT); - - listenSocket.on('listening', function() { - listenSocket.addMembership(LOCAL_BROADCAST_HOST); - }); } From 0f2782b4962c0695bb296523a857d35d9824de9a Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 26 Nov 2015 19:36:47 -0800 Subject: [PATCH 02/12] fixup --- .../test-dgram-multicast-multi-process.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 99de541dab7d30..2b6602f7b9bf13 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -1,12 +1,12 @@ 'use strict'; -const common = require('../common'), - assert = require('assert'), - dgram = require('dgram'), - Buffer = require('buffer').Buffer, - fork = require('child_process').fork, - LOCAL_BROADCAST_HOST = '224.0.0.114', - TIMEOUT = common.platformTimeout(5000), - messages = [ +const common = require('../common'); +const assert = require('assert'); +const dgram = require('dgram'); +const Buffer = require('buffer').Buffer; +const fork = require('child_process').fork; +const LOCAL_BROADCAST_HOST = '224.0.0.114'; +const TIMEOUT = common.platformTimeout(5000); +const messages = [ new Buffer('First message to send'), new Buffer('Second message to send'), new Buffer('Third message to send'), From 8021d83f4f02078b065babe56c303094ad622ef4 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 28 Nov 2015 11:07:30 -0800 Subject: [PATCH 03/12] fixup: skip if in a FreeBSD jail --- test/internet/test-dgram-multicast-multi-process.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 2b6602f7b9bf13..487d18582b49f0 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -13,6 +13,12 @@ const messages = [ new Buffer('Fourth message to send') ]; +// skip test in FreeBSD jails +if (common.inFreeBSDJail) { + console.log('1..0 # Skipped: In a FreeBSD jail'); + return; +} + function launchChildProcess(index) { const worker = fork(process.argv[1], ['child']); workers[worker.pid] = worker; From 0b5c3cb2a9f369f554b77639f430c595eeddb114 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Nov 2015 13:27:29 -0800 Subject: [PATCH 04/12] fixup: drop Buffer import, it is there by default in current Node --- test/internet/test-dgram-multicast-multi-process.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 487d18582b49f0..71888a56edacc9 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -2,7 +2,6 @@ const common = require('../common'); const assert = require('assert'); const dgram = require('dgram'); -const Buffer = require('buffer').Buffer; const fork = require('child_process').fork; const LOCAL_BROADCAST_HOST = '224.0.0.114'; const TIMEOUT = common.platformTimeout(5000); From 26b56c0890a0525874fc4d5845be6c7a5c93391e Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Nov 2015 13:28:59 -0800 Subject: [PATCH 05/12] fixup: fix funky indentation --- test/internet/test-dgram-multicast-multi-process.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 71888a56edacc9..106a8522518214 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -6,11 +6,11 @@ const fork = require('child_process').fork; const LOCAL_BROADCAST_HOST = '224.0.0.114'; const TIMEOUT = common.platformTimeout(5000); const messages = [ - new Buffer('First message to send'), - new Buffer('Second message to send'), - new Buffer('Third message to send'), - new Buffer('Fourth message to send') - ]; + new Buffer('First message to send'), + new Buffer('Second message to send'), + new Buffer('Third message to send'), + new Buffer('Fourth message to send') +]; // skip test in FreeBSD jails if (common.inFreeBSDJail) { From 9b527da43aeddd9c8819aeb1d70f2c1294ec8b4c Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Nov 2015 13:30:15 -0800 Subject: [PATCH 06/12] fixup: process.argv[1] -> filename --- test/internet/test-dgram-multicast-multi-process.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 106a8522518214..1dbd6e6fca3fd6 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -19,7 +19,7 @@ if (common.inFreeBSDJail) { } function launchChildProcess(index) { - const worker = fork(process.argv[1], ['child']); + const worker = fork(__filename, ['child']); workers[worker.pid] = worker; worker.messagesReceived = []; From fef9193478d5e5b5e28696f9da9629ea7898ede7 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Nov 2015 13:50:40 -0800 Subject: [PATCH 07/12] fixup: hoist workers --- test/internet/test-dgram-multicast-multi-process.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 1dbd6e6fca3fd6..493155727c2153 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -11,6 +11,9 @@ const messages = [ new Buffer('Third message to send'), new Buffer('Fourth message to send') ]; +const workers = {}; +const listeners = 3; + // skip test in FreeBSD jails if (common.inFreeBSDJail) { @@ -111,12 +114,10 @@ function killChildren(children) { } if (process.argv[2] !== 'child') { - var workers = {}, - listeners = 3, - listening = 0, - dead = 0, - i = 0, - done = 0; + var listening = 0; + var dead = 0; + var i = 0; + var done = 0; //exit the test if it doesn't succeed within TIMEOUT var timer = setTimeout(function() { From 71470b5b2da171b85377e2388188fbbe58c8503f Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Nov 2015 13:54:25 -0800 Subject: [PATCH 08/12] fixup: copyedit comments --- .../test-dgram-multicast-multi-process.js | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 493155727c2153..c24242ccef388e 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -15,7 +15,7 @@ const workers = {}; const listeners = 3; -// skip test in FreeBSD jails +// Skip test in FreeBSD jails. if (common.inFreeBSDJail) { console.log('1..0 # Skipped: In a FreeBSD jail'); return; @@ -27,12 +27,10 @@ function launchChildProcess(index) { worker.messagesReceived = []; - //handle the death of workers + // Handle the death of workers. worker.on('exit', function(code, signal) { - // don't consider this the true death if the - // worker has finished successfully - - // or if the exit code is 0 + // Don't consider this the true death if the worker has finished + // successfully or if the exit code is 0. if (worker.isDone || code === 0) { return; } @@ -58,7 +56,7 @@ function launchChildProcess(index) { listening += 1; if (listening === listeners) { - //all child process are listening, so start sending + // All child process are listening, so start sending. sendSocket.sendNext(); } } @@ -119,7 +117,7 @@ if (process.argv[2] !== 'child') { var i = 0; var done = 0; - //exit the test if it doesn't succeed within TIMEOUT + // Exit the test if it doesn't succeed within TIMEOUT. var timer = setTimeout(function() { console.error('[PARENT] Responses were not received within %d ms.', TIMEOUT); @@ -130,18 +128,18 @@ if (process.argv[2] !== 'child') { process.exit(1); }, TIMEOUT); - //launch child processes + // Launch child processes. for (var x = 0; x < listeners; x++) { launchChildProcess(x); } var sendSocket = dgram.createSocket('udp4'); - // FIXME a libuv limitation makes it necessary to bind() - // before calling any of the set*() functions - the bind() - // call is what creates the actual socket... + // FIXME: a libuv limitation makes it necessary to bind() + // before calling any of the set*() functions. The bind() + // call is what creates the actual socket. sendSocket.bind(); - // The socket is actually created async now + // The socket is actually created async now. sendSocket.on('listening', function() { sendSocket.setTTL(1); sendSocket.setBroadcast(true); @@ -191,7 +189,7 @@ if (process.argv[2] === 'child') { process.send({ message: buf.toString() }); if (receivedMessages.length == messages.length) { - // .dropMembership() not strictly needed but here as a sanity check + // .dropMembership() not strictly needed but here as a sanity check. listenSocket.dropMembership(LOCAL_BROADCAST_HOST); process.nextTick(function() { listenSocket.close(); @@ -200,9 +198,9 @@ if (process.argv[2] === 'child') { }); listenSocket.on('close', function() { - //HACK: Wait to exit the process to ensure that the parent - //process has had time to receive all messages via process.send() - //This may be indicitave of some other issue. + // HACK: Wait to exit the process to ensure that the parent + // process has had time to receive all messages via process.send() + // This may be indicitave of some other issue. setTimeout(function() { process.exit(); }, common.platformTimeout(1000)); From 615356a5ccfd41bfe398de3c9af058eeab4e283f Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Nov 2015 13:57:56 -0800 Subject: [PATCH 09/12] fixup: remove unnecessary killChildren() --- test/internet/test-dgram-multicast-multi-process.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index c24242ccef388e..207e946acdb0ee 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -44,9 +44,6 @@ function launchChildProcess(index) { if (dead === listeners) { console.error('[PARENT] All workers have died.'); console.error('[PARENT] Fail'); - - killChildren(workers); - process.exit(1); } }); From ccafb10adf4b60c44ef4128e15803c785091dd67 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Nov 2015 14:00:31 -0800 Subject: [PATCH 10/12] fixup: rm else --- test/internet/test-dgram-multicast-multi-process.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 207e946acdb0ee..883e123eadec2d 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -56,8 +56,9 @@ function launchChildProcess(index) { // All child process are listening, so start sending. sendSocket.sendNext(); } + return; } - else if (msg.message) { + if (msg.message) { worker.messagesReceived.push(msg.message); if (worker.messagesReceived.length === messages.length) { From bd8a3c16f8d4183f4f1c3b341ec085c18b0a698c Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Nov 2015 14:13:02 -0800 Subject: [PATCH 11/12] fixup: equal->strictEqual --- test/internet/test-dgram-multicast-multi-process.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 883e123eadec2d..7ca3bb1fa7bb0e 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -90,7 +90,7 @@ function launchChildProcess(index) { console.error('[PARENT] %d received %d matching messages.', worker.pid, count); - assert.equal(count, messages.length, + assert.strictEqual(count, messages.length, 'A worker received an invalid multicast message'); }); From ec975e4de4cd2711c0897aa8f10adcb469c05b9d Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 30 Nov 2015 14:13:23 -0800 Subject: [PATCH 12/12] fixup: typo fix --- test/internet/test-dgram-multicast-multi-process.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/internet/test-dgram-multicast-multi-process.js b/test/internet/test-dgram-multicast-multi-process.js index 7ca3bb1fa7bb0e..f72402eef64147 100644 --- a/test/internet/test-dgram-multicast-multi-process.js +++ b/test/internet/test-dgram-multicast-multi-process.js @@ -198,7 +198,7 @@ if (process.argv[2] === 'child') { listenSocket.on('close', function() { // HACK: Wait to exit the process to ensure that the parent // process has had time to receive all messages via process.send() - // This may be indicitave of some other issue. + // This may be indicative of some other issue. setTimeout(function() { process.exit(); }, common.platformTimeout(1000));