FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

child_process: fix spawn and fork abort behavior · nodejs/node@0bcaf9c · GitHub

/ node Public

Commit 0bcaf9c

Browse files
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 8010c83 commit 0bcaf9c

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: v14.17.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: v14.17.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
@@ -610,6 +610,18 @@ function normalizeSpawnArguments(file, args, options) {
610610
};
611611
}
612612

613+
function abortChildProcess(child, killSignal) {
614+
if (!child)
615+
return;
616+
try {
617+
if (child.kill(killSignal)) {
618+
child.emit('error', new AbortError());
619+
}
620+
} catch (err) {
621+
child.emit('error', err);
622+
}
623+
}
624+
613625

614626
/**
615627
* Spawns a new process using the given `file`.
@@ -641,21 +653,19 @@ function spawn(file, args, options) {
641653
const signal = options.signal;
642654
// Validate signal, if present
643655
validateAbortSignal(signal, 'options.signal');
644-
656+
const killSignal = sanitizeKillSignal(options.killSignal);
645657
// Do nothing and throw if already aborted
646658
if (signal.aborted) {
647659
onAbortListener();
648660
} else {
649661
signal.addEventListener('abort', onAbortListener, { once: true });
650-
child.once('close',
662+
child.once('exit',
651663
() => signal.removeEventListener('abort', onAbortListener));
652664
}
653665

654666
function onAbortListener() {
655667
process.nextTick(() => {
656-
child?.kill?.(options.killSignal);
657-
658-
child.emit('error', new AbortError());
668+
abortChildProcess(child, killSignal);
659669
});
660670
}
661671
}
@@ -877,19 +887,18 @@ function spawnWithSignal(file, args, options) {
877887
}
878888
const child = spawn(file, args, opts);
879889

880-
if (options && options.signal) {
890+
if (options?.signal) {
891+
const killSignal = sanitizeKillSignal(options.killSignal);
892+
881893
function kill() {
882-
if (child._handle) {
883-
child._handle.kill(options.killSignal || 'SIGTERM');
884-
child.emit('error', new AbortError());
885-
}
894+
abortChildProcess(child, killSignal);
886895
}
887896
if (options.signal.aborted) {
888897
process.nextTick(kill);
889898
} else {
890-
options.signal.addEventListener('abort', kill);
899+
options.signal.addEventListener('abort', kill, { once: true });
891900
const remove = () => options.signal.removeEventListener('abort', kill);
892-
child.once('close', remove);
901+
child.once('exit', remove);
893902
}
894903
}
895904
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
@@ -5,48 +5,47 @@ const assert = require('assert');
55
const exec = require('child_process').exec;
66
const { promisify } = require('util');
77

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

15-
14+
let waitCommand = '';
1615
if (common.isWindows) {
17-
pwdcommand = 'echo %cd%';
18-
dir = 'c:\\windows';
16+
waitCommand = 'TIMEOUT 120';
1917
} else {
20-
pwdcommand = 'pwd';
21-
dir = '/dev';
18+
waitCommand = 'sleep 2m';
2219
}
2320

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

3330
{
3431
assert.throws(() => {
35-
execPromisifed(pwdcommand, { cwd: dir, signal: {} });
32+
execPromisifed(waitCommand, { signal: {} });
3633
}, invalidArgTypeError);
3734
}
3835

3936
{
4037
function signal() {}
4138
assert.throws(() => {
42-
execPromisifed(pwdcommand, { cwd: dir, signal });
39+
execPromisifed(waitCommand, { signal });
4340
}, invalidArgTypeError);
4441
}
4542

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

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

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

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Flags: --experimental-abortcontroller
22
'use strict';
33

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

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

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

44
const common = require('../common');
55
const assert = require('assert');
6-
const cp = require('child_process');
6+
const { spawn } = require('child_process');
7+
const fixtures = require('../common/fixtures');
78

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

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

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

@@ -30,13 +35,76 @@ const cp = require('child_process');
3035

3136
controller.abort();
3237

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

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

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL