Skip to content

Commit 259868c

Browse files
author
Nitzan Uziely
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
1 parent cf5f6af commit 259868c

4 files changed

Lines changed: 150 additions & 25 deletions

File tree

‎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: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -586,6 +586,18 @@ function normalizeSpawnArguments(file, args, options) {
586586
};
587587
}
588588

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

590602
function spawn(file, args, options) {
591603
const child = new ChildProcess();
@@ -595,21 +607,19 @@ function spawn(file, args, options) {
595607
const signal = options.signal;
596608
// Validate signal, if present
597609
validateAbortSignal(signal, 'options.signal');
598-
610+
const killSignal = sanitizeKillSignal(options.killSignal);
599611
// Do nothing and throw if already aborted
600612
if (signal.aborted) {
601613
onAbortListener();
602614
} else {
603615
signal.addEventListener('abort', onAbortListener, { once: true });
604-
child.once('close',
616+
child.once('exit',
605617
() => signal.removeEventListener('abort', onAbortListener));
606618
}
607619

608620
function onAbortListener() {
609621
process.nextTick(() => {
610-
child?.kill?.(options.killSignal);
611-
612-
child.emit('error', new AbortError());
622+
abortChildProcess(child, killSignal);
613623
});
614624
}
615625
}
@@ -761,20 +771,18 @@ function spawnWithSignal(file, args, options) {
761771
const child = spawn(file, args, opts);
762772

763773
if (options && options.signal) {
774+
const killSignal = sanitizeKillSignal(options.killSignal);
764775
// Validate signal, if present
765776
validateAbortSignal(options.signal, 'options.signal');
766777
function kill() {
767-
if (child._handle) {
768-
child._handle.kill(options.killSignal || 'SIGTERM');
769-
child.emit('error', new AbortError());
770-
}
778+
abortChildProcess(child, killSignal);
771779
}
772780
if (options.signal.aborted) {
773781
process.nextTick(kill);
774782
} else {
775-
options.signal.addEventListener('abort', kill);
783+
options.signal.addEventListener('abort', kill, { once: true });
776784
const remove = () => options.signal.removeEventListener('abort', kill);
777-
child.once('close', remove);
785+
child.once('exit', remove);
778786
}
779787
}
780788
return child;

‎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)