Skip to content

Commit

Permalink
test: deflake child process exec timeout tests
Browse files Browse the repository at this point in the history
On Windows it might take too long for the parent to start the
communication with a child process, so by the time the parent
starts its own timer, the child process might have already
completed running, and the parent in those tests won't have a
chance to terminate these child processes after the timeout.
To address this issue, raise the time for which the child is
supposed to run to make sure that the parent starts
its own timer before the child terminates in the tests.
Also, split the test into smaller ones to reduce the overhead.

PR-URL: nodejs#44390
Refs: nodejs/reliability#356
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
  • Loading branch information
joyeecheung authored and Fyko committed Sep 15, 2022
1 parent 15cdf39 commit a3202f8
Show file tree
Hide file tree
Showing 5 changed files with 168 additions and 73 deletions.
45 changes: 45 additions & 0 deletions test/common/child_process.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';

const assert = require('assert');
const common = require('./');

// Workaround for Windows Server 2008R2
// When CMD is used to launch a process and CMD is killed too quickly, the
// process can stay behind running in suspended state, never completing.
function cleanupStaleProcess(filename) {
if (!common.isWindows) {
return;
}
process.once('beforeExit', () => {
const basename = filename.replace(/.*[/\\]/g, '');
require('child_process')
.execFileSync(`${process.env.SystemRoot}\\System32\\wbem\\WMIC.exe`, [
'process',
'where',
`commandline like '%${basename}%child'`,
'delete',
'/nointeractive',
]);
});
}

// This should keep the child process running long enough to expire
// the timeout.
const kExpiringChildRunTime = common.platformTimeout(20 * 1000);
const kExpiringParentTimer = 1;
assert(kExpiringChildRunTime > kExpiringParentTimer);

function logAfterTime(time) {
setTimeout(() => {
// The following console statements are part of the test.
console.log('child stdout');
console.error('child stderr');
}, time);
}

module.exports = {
cleanupStaleProcess,
logAfterTime,
kExpiringChildRunTime,
kExpiringParentTimer
};
50 changes: 50 additions & 0 deletions test/parallel/test-child-process-exec-timeout-expire.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict';

// Test exec() with a timeout that expires.

const common = require('../common');
const assert = require('assert');
const cp = require('child_process');

const {
cleanupStaleProcess,
logAfterTime,
kExpiringChildRunTime,
kExpiringParentTimer
} = require('../common/child_process');

if (process.argv[2] === 'child') {
logAfterTime(kExpiringChildRunTime);
return;
}

const cmd = `"${process.execPath}" "${__filename}" child`;

cp.exec(cmd, {
timeout: kExpiringParentTimer,
}, common.mustCall((err, stdout, stderr) => {
console.log('[stdout]', stdout.trim());
console.log('[stderr]', stderr.trim());

let sigterm = 'SIGTERM';
assert.strictEqual(err.killed, true);
// TODO OpenBSD returns a null signal and 143 for code
if (common.isOpenBSD) {
assert.strictEqual(err.code, 143);
sigterm = null;
} else {
assert.strictEqual(err.code, null);
}
// At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM to a
// process that is still starting up kills it with SIGKILL instead of SIGTERM.
// See: https://github.com/libuv/libuv/issues/1226
if (common.isOSX)
assert.ok(err.signal === 'SIGTERM' || err.signal === 'SIGKILL');
else
assert.strictEqual(err.signal, sigterm);
assert.strictEqual(err.cmd, cmd);
assert.strictEqual(stdout.trim(), '');
assert.strictEqual(stderr.trim(), '');
}));

cleanupStaleProcess(__filename);
39 changes: 39 additions & 0 deletions test/parallel/test-child-process-exec-timeout-kill.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';

// Test exec() with both a timeout and a killSignal.

const common = require('../common');
const assert = require('assert');
const cp = require('child_process');

const {
cleanupStaleProcess,
logInTimeout,
kExpiringChildRunTime,
kExpiringParentTimer,
} = require('../common/child_process');

if (process.argv[2] === 'child') {
logInTimeout(kExpiringChildRunTime);
return;
}

const cmd = `"${process.execPath}" "${__filename}" child`;

// Test with a different kill signal.
cp.exec(cmd, {
timeout: kExpiringParentTimer,
killSignal: 'SIGKILL'
}, common.mustCall((err, stdout, stderr) => {
console.log('[stdout]', stdout.trim());
console.log('[stderr]', stderr.trim());

assert.strictEqual(err.killed, true);
assert.strictEqual(err.code, null);
assert.strictEqual(err.signal, 'SIGKILL');
assert.strictEqual(err.cmd, cmd);
assert.strictEqual(stdout.trim(), '');
assert.strictEqual(stderr.trim(), '');
}));

cleanupStaleProcess(__filename);
34 changes: 34 additions & 0 deletions test/parallel/test-child-process-exec-timeout-not-expired.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
'use strict';

// Test exec() when a timeout is set, but not expired.

const common = require('../common');
const assert = require('assert');
const cp = require('child_process');

const {
cleanupStaleProcess,
logAfterTime
} = require('../common/child_process');

const kTimeoutNotSupposedToExpire = 2 ** 30;
const childRunTime = common.platformTimeout(100);

// The time spent in the child should be smaller than the timeout below.
assert(childRunTime < kTimeoutNotSupposedToExpire);

if (process.argv[2] === 'child') {
logAfterTime(childRunTime);
return;
}

const cmd = `"${process.execPath}" "${__filename}" child`;

cp.exec(cmd, {
timeout: kTimeoutNotSupposedToExpire
}, common.mustSucceed((stdout, stderr) => {
assert.strictEqual(stdout.trim(), 'child stdout');
assert.strictEqual(stderr.trim(), 'child stderr');
}));

cleanupStaleProcess(__filename);
73 changes: 0 additions & 73 deletions test/parallel/test-child-process-exec-timeout.js

This file was deleted.

0 comments on commit a3202f8

Please sign in to comment.