Skip to content

Commit ddae112

Browse files
Nitzan Uzielytargos
Nitzan Uziely
authored and
targos
committed
child_process: fix spawn and fork abort behavior
Fix AbortSignal in Spawn which doesn't actually abort the process, and fork can emit an AbortError even if the process was already exited. Add documentation For killSignal. Fixes: #37273 PR-URL: #37325 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
1 parent 3b7cb75 commit ddae112

5 files changed

Lines changed: 165 additions & 40 deletions

‎doc/api/child_process.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,9 @@ controller.abort();
351351
<!-- YAML
352352
added: v0.5.0
353353
changes:
354+
- version: REPLACEME
355+
pr-url: https://github.com/nodejs/node/pull/37325
356+
description: killSignal for AbortSignal was added.
354357
- version: v15.6.0
355358
pr-url: https://github.com/nodejs/node/pull/36603
356359
description: AbortSignal support was added.
@@ -383,6 +386,8 @@ changes:
383386
messages between processes. Possible values are `'json'` and `'advanced'`.
384387
See [Advanced serialization][] for more details. **Default:** `'json'`.
385388
* `signal` {AbortSignal} Allows closing the subprocess using an AbortSignal.
389+
* `killSignal` {string} The signal value to be used when the spawned
390+
process will be killed by the abort signal. **Default:** `'SIGTERM'`.
386391
* `silent` {boolean} If `true`, stdin, stdout, and stderr of the child will be
387392
piped to the parent, otherwise they will be inherited from the parent, see
388393
the `'pipe'` and `'inherit'` options for [`child_process.spawn()`][]'s
@@ -431,6 +436,9 @@ The `signal` option works exactly the same way it does in
431436
<!-- YAML
432437
added: v0.1.90
433438
changes:
439+
- version: REPLACEME
440+
pr-url: https://github.com/nodejs/node/pull/37325
441+
description: killSignal for AbortSignal was added.
434442
- version: v15.5.0
435443
pr-url: https://github.com/nodejs/node/pull/36432
436444
description: AbortSignal support was added.
@@ -477,6 +485,8 @@ changes:
477485
* `windowsHide` {boolean} Hide the subprocess console window that would
478486
normally be created on Windows systems. **Default:** `false`.
479487
* `signal` {AbortSignal} allows aborting the execFile using an AbortSignal.
488+
* `killSignal` {string} The signal value to be used when the spawned
489+
process will be killed by the abort signal. **Default:** `'SIGTERM'`.
480490

481491
* Returns: {ChildProcess}
482492

‎lib/child_process.js‎

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,18 @@ function normalizeSpawnArguments(file, args, options) {
585585
};
586586
}
587587

588+
function abortChildProcess(child, killSignal) {
589+
if (!child)
590+
return;
591+
try {
592+
if (child.kill(killSignal)) {
593+
child.emit('error', new AbortError());
594+
}
595+
} catch (err) {
596+
child.emit('error', err);
597+
}
598+
}
599+
588600

589601
function spawn(file, args, options) {
590602
const child = new ChildProcess();
@@ -594,21 +606,19 @@ function spawn(file, args, options) {
594606
const signal = options.signal;
595607
// Validate signal, if present
596608
validateAbortSignal(signal, 'options.signal');
597-
609+
const killSignal = sanitizeKillSignal(options.killSignal);
598610
// Do nothing and throw if already aborted
599611
if (signal.aborted) {
600612
onAbortListener();
601613
} else {
602614
signal.addEventListener('abort', onAbortListener, { once: true });
603-
child.once('close',
615+
child.once('exit',
604616
() => signal.removeEventListener('abort', onAbortListener));
605617
}
606618

607619
function onAbortListener() {
608620
process.nextTick(() => {
609-
child?.kill?.(options.killSignal);
610-
611-
child.emit('error', new AbortError());
621+
abortChildProcess(child, killSignal);
612622
});
613623
}
614624
}
@@ -764,19 +774,18 @@ function spawnWithSignal(file, args, options) {
764774
}
765775
const child = spawn(file, args, opts);
766776

767-
if (options && options.signal) {
777+
if (options?.signal) {
778+
const killSignal = sanitizeKillSignal(options.killSignal);
779+
768780
function kill() {
769-
if (child._handle) {
770-
child._handle.kill(options.killSignal || 'SIGTERM');
771-
child.emit('error', new AbortError());
772-
}
781+
abortChildProcess(child, killSignal);
773782
}
774783
if (options.signal.aborted) {
775784
process.nextTick(kill);
776785
} else {
777-
options.signal.addEventListener('abort', kill);
786+
options.signal.addEventListener('abort', kill, { once: true });
778787
const remove = () => options.signal.removeEventListener('abort', kill);
779-
child.once('close', remove);
788+
child.once('exit', remove);
780789
}
781790
}
782791
return child;

‎test/parallel/test-child-process-exec-abortcontroller-promisified.js‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,48 +4,47 @@ const assert = require('assert');
44
const exec = require('child_process').exec;
55
const { promisify } = require('util');
66

7-
let pwdcommand, dir;
87
const execPromisifed = promisify(exec);
98
const invalidArgTypeError = {
109
code: 'ERR_INVALID_ARG_TYPE',
1110
name: 'TypeError'
1211
};
1312

14-
13+
let waitCommand = '';
1514
if (common.isWindows) {
16-
pwdcommand = 'echo %cd%';
17-
dir = 'c:\\windows';
15+
waitCommand = 'TIMEOUT 120';
1816
} else {
19-
pwdcommand = 'pwd';
20-
dir = '/dev';
17+
waitCommand = 'sleep 2m';
2118
}
2219

23-
2420
{
2521
const ac = new AbortController();
2622
const signal = ac.signal;
27-
const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
28-
assert.rejects(promise, /AbortError/).then(common.mustCall());
23+
const promise = execPromisifed(waitCommand, { signal });
24+
assert.rejects(promise, /AbortError/, 'post aborted sync signal failed')
25+
.then(common.mustCall());
2926
ac.abort();
3027
}
3128

3229
{
3330
assert.throws(() => {
34-
execPromisifed(pwdcommand, { cwd: dir, signal: {} });
31+
execPromisifed(waitCommand, { signal: {} });
3532
}, invalidArgTypeError);
3633
}
3734

3835
{
3936
function signal() {}
4037
assert.throws(() => {
41-
execPromisifed(pwdcommand, { cwd: dir, signal });
38+
execPromisifed(waitCommand, { signal });
4239
}, invalidArgTypeError);
4340
}
4441

4542
{
4643
const ac = new AbortController();
47-
const signal = (ac.abort(), ac.signal);
48-
const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
44+
const { signal } = ac;
45+
ac.abort();
46+
const promise = execPromisifed(waitCommand, { signal });
4947

50-
assert.rejects(promise, /AbortError/).then(common.mustCall());
48+
assert.rejects(promise, /AbortError/, 'pre aborted signal failed')
49+
.then(common.mustCall());
5150
}

‎test/parallel/test-child-process-fork-abort-signal.js‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use strict';
22

3-
const { mustCall } = require('../common');
3+
const { mustCall, mustNotCall } = require('../common');
44
const { strictEqual } = require('assert');
55
const fixtures = require('../common/fixtures');
66
const { fork } = require('child_process');
@@ -12,7 +12,10 @@ const { fork } = require('child_process');
1212
const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
1313
signal
1414
});
15-
cp.on('exit', mustCall());
15+
cp.on('exit', mustCall((code, killSignal) => {
16+
strictEqual(code, null);
17+
strictEqual(killSignal, 'SIGTERM');
18+
}));
1619
cp.on('error', mustCall((err) => {
1720
strictEqual(err.name, 'AbortError');
1821
}));
@@ -26,8 +29,44 @@ const { fork } = require('child_process');
2629
const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
2730
signal
2831
});
29-
cp.on('exit', mustCall());
32+
cp.on('exit', mustCall((code, killSignal) => {
33+
strictEqual(code, null);
34+
strictEqual(killSignal, 'SIGTERM');
35+
}));
36+
cp.on('error', mustCall((err) => {
37+
strictEqual(err.name, 'AbortError');
38+
}));
39+
}
40+
41+
{
42+
// Test passing a different kill signal
43+
const ac = new AbortController();
44+
const { signal } = ac;
45+
ac.abort();
46+
const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
47+
signal,
48+
killSignal: 'SIGKILL',
49+
});
50+
cp.on('exit', mustCall((code, killSignal) => {
51+
strictEqual(code, null);
52+
strictEqual(killSignal, 'SIGKILL');
53+
}));
3054
cp.on('error', mustCall((err) => {
3155
strictEqual(err.name, 'AbortError');
3256
}));
3357
}
58+
59+
{
60+
// Test aborting a cp before close but after exit
61+
const ac = new AbortController();
62+
const { signal } = ac;
63+
const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
64+
signal
65+
});
66+
cp.on('exit', mustCall(() => {
67+
ac.abort();
68+
}));
69+
cp.on('error', mustNotCall());
70+
71+
setTimeout(() => cp.kill(), 1);
72+
}

‎test/parallel/test-child-process-spawn-controller.js‎

Lines changed: 79 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,25 @@
22

33
const common = require('../common');
44
const assert = require('assert');
5-
const cp = require('child_process');
5+
const { spawn } = require('child_process');
6+
const fixtures = require('../common/fixtures');
67

8+
const aliveScript = fixtures.path('child-process-stay-alive-forever.js');
79
{
810
// Verify that passing an AbortSignal works
911
const controller = new AbortController();
1012
const { signal } = controller;
1113

12-
const echo = cp.spawn('echo', ['fun'], {
13-
encoding: 'utf8',
14-
shell: true,
15-
signal
14+
const cp = spawn(process.execPath, [aliveScript], {
15+
signal,
1616
});
1717

18-
echo.on('error', common.mustCall((e) => {
18+
cp.on('exit', common.mustCall((code, killSignal) => {
19+
assert.strictEqual(code, null);
20+
assert.strictEqual(killSignal, 'SIGTERM');
21+
}));
22+
23+
cp.on('error', common.mustCall((e) => {
1924
assert.strictEqual(e.name, 'AbortError');
2025
}));
2126

@@ -29,13 +34,76 @@ const cp = require('child_process');
2934

3035
controller.abort();
3136

32-
const echo = cp.spawn('echo', ['fun'], {
33-
encoding: 'utf8',
34-
shell: true,
35-
signal
37+
const cp = spawn(process.execPath, [aliveScript], {
38+
signal,
39+
});
40+
cp.on('exit', common.mustCall((code, killSignal) => {
41+
assert.strictEqual(code, null);
42+
assert.strictEqual(killSignal, 'SIGTERM');
43+
}));
44+
45+
cp.on('error', common.mustCall((e) => {
46+
assert.strictEqual(e.name, 'AbortError');
47+
}));
48+
}
49+
50+
{
51+
// Verify that waiting a bit and closing works
52+
const controller = new AbortController();
53+
const { signal } = controller;
54+
55+
const cp = spawn(process.execPath, [aliveScript], {
56+
signal,
57+
});
58+
59+
cp.on('exit', common.mustCall((code, killSignal) => {
60+
assert.strictEqual(code, null);
61+
assert.strictEqual(killSignal, 'SIGTERM');
62+
}));
63+
64+
cp.on('error', common.mustCall((e) => {
65+
assert.strictEqual(e.name, 'AbortError');
66+
}));
67+
68+
setTimeout(() => controller.abort(), 1);
69+
}
70+
71+
{
72+
// Test passing a different killSignal
73+
const controller = new AbortController();
74+
const { signal } = controller;
75+
76+
const cp = spawn(process.execPath, [aliveScript], {
77+
signal,
78+
killSignal: 'SIGKILL',
3679
});
3780

38-
echo.on('error', common.mustCall((e) => {
81+
cp.on('exit', common.mustCall((code, killSignal) => {
82+
assert.strictEqual(code, null);
83+
assert.strictEqual(killSignal, 'SIGKILL');
84+
}));
85+
86+
cp.on('error', common.mustCall((e) => {
3987
assert.strictEqual(e.name, 'AbortError');
4088
}));
89+
90+
setTimeout(() => controller.abort(), 1);
91+
}
92+
93+
{
94+
// Test aborting a cp before close but after exit
95+
const controller = new AbortController();
96+
const { signal } = controller;
97+
98+
const cp = spawn(process.execPath, [aliveScript], {
99+
signal,
100+
});
101+
102+
cp.on('exit', common.mustCall(() => {
103+
controller.abort();
104+
}));
105+
106+
cp.on('error', common.mustNotCall());
107+
108+
setTimeout(() => cp.kill(), 1);
41109
}

0 commit comments

Comments
 (0)