Skip to content

child_process: use signal.reason in child process abort #47817

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nodejs-github-bot merged 5 commits into from
May 8, 2023
Merged

child_process: use signal.reason in child process abort #47817

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f2e3974
child_process: use signal.reason in child process abort
debadree25 May 2, 2023
29954c9
fixup! add another test
debadree25 May 2, 2023
6fc91a2
fixup! use cause
debadree25 May 2, 2023
e26e572
fixup! update test
debadree25 May 7, 2023
ece8b5e
fixup! update default exception
debadree25 May 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,12 +712,12 @@ function normalizeSpawnArguments(file, args, options) {
};
}

function abortChildProcess(child, killSignal) {
function abortChildProcess(child, killSignal, reason) {
if (!child)
return;
try {
if (child.kill(killSignal)) {
child.emit('error', new AbortError());
child.emit('error', new AbortError(undefined, { cause: reason }));
Comment thread
aduh95 marked this conversation as resolved.
}
} catch (err) {
child.emit('error', err);
Expand Down Expand Up @@ -787,7 +787,7 @@ function spawn(file, args, options) {
}

function onAbortListener() {
abortChildProcess(child, killSignal);
abortChildProcess(child, killSignal, options.signal.reason);
}
Copy link
Copy Markdown
Member

@benjamingr benjamingr May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: accessing options.signal.reason can potentially throw though I don't think we guard against that anywhere else to be honest.

Copy link
Copy Markdown
Member Author

@debadree25 debadree25 May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, why does it throw tho? I have never seen it like that, is there any way to guard?

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be a getter that throws. To guard it against that, we would need a try/catch:

let reason;
try { ({ reason } = options.signal); } catch {}

abortChildProcess(child, killSignal, reason);

If we don’t guard against that anywhere else, it’s probably OK to ignore for this PR, but we should do it in a follow up PR IMHO.

Copy link
Copy Markdown
Member Author

@debadree25 debadree25 May 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm i check no where else it guarded maybe we could do a larger refactor in a followup

}

Expand Down
47 changes: 44 additions & 3 deletions test/parallel/test-child-process-exec-abortcontroller-promisified.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,35 @@ const waitCommand = common.isWindows ?
const ac = new AbortController();
const signal = ac.signal;
const promise = execPromisifed(waitCommand, { signal });
assert.rejects(promise, /AbortError/, 'post aborted sync signal failed')
.then(common.mustCall());
assert.rejects(promise, {
name: 'AbortError'
Comment thread
RaisinTen marked this conversation as resolved.
Outdated
}).then(common.mustCall());
ac.abort();
}

{
const err = new Error('boom');
const ac = new AbortController();
const signal = ac.signal;
const promise = execPromisifed(waitCommand, { signal });
assert.rejects(promise, {
name: 'AbortError',
cause: err
}).then(common.mustCall());
ac.abort(err);
}

{
const ac = new AbortController();
const signal = ac.signal;
const promise = execPromisifed(waitCommand, { signal });
assert.rejects(promise, {
name: 'AbortError',
cause: 'boom'
}).then(common.mustCall());
ac.abort('boom');
}

{
assert.throws(() => {
execPromisifed(waitCommand, { signal: {} });
Expand All @@ -40,6 +64,23 @@ const waitCommand = common.isWindows ?
const signal = AbortSignal.abort(); // Abort in advance
const promise = execPromisifed(waitCommand, { signal });

assert.rejects(promise, /AbortError/, 'pre aborted signal failed')
assert.rejects(promise, { name: 'AbortError' })
.then(common.mustCall());
}

{
const err = new Error('boom');
const signal = AbortSignal.abort(err); // Abort in advance
const promise = execPromisifed(waitCommand, { signal });

assert.rejects(promise, { name: 'AbortError', cause: err })
.then(common.mustCall());
}

{
const signal = AbortSignal.abort('boom'); // Abort in advance
const promise = execPromisifed(waitCommand, { signal });

assert.rejects(promise, { name: 'AbortError', cause: 'boom' })
.then(common.mustCall());
}
37 changes: 37 additions & 0 deletions test/parallel/test-child-process-fork-abort-signal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,26 @@ const { fork } = require('child_process');
}));
process.nextTick(() => ac.abort());
}

{
// Test aborting with custom error
const ac = new AbortController();
const { signal } = ac;
const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
signal
});
cp.on('exit', mustCall((code, killSignal) => {
strictEqual(code, null);
strictEqual(killSignal, 'SIGTERM');
}));
cp.on('error', mustCall((err) => {
strictEqual(err.name, 'AbortError');
strictEqual(err.cause.name, 'Error');
strictEqual(err.cause.message, 'boom');
}));
process.nextTick(() => ac.abort(new Error('boom')));
}

{
// Test passing an already aborted signal to a forked child_process
const signal = AbortSignal.abort();
Expand All @@ -36,6 +56,23 @@ const { fork } = require('child_process');
}));
}

{
// Test passing an aborted signal with custom error to a forked child_process
const signal = AbortSignal.abort(new Error('boom'));
const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
signal
});
cp.on('exit', mustCall((code, killSignal) => {
strictEqual(code, null);
strictEqual(killSignal, 'SIGTERM');
}));
cp.on('error', mustCall((err) => {
strictEqual(err.name, 'AbortError');
strictEqual(err.cause.name, 'Error');
strictEqual(err.cause.message, 'boom');
}));
}

{
// Test passing a different kill signal
const signal = AbortSignal.abort();
Expand Down
77 changes: 77 additions & 0 deletions test/parallel/test-child-process-spawn-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,48 @@ const aliveScript = fixtures.path('child-process-stay-alive-forever.js');
controller.abort();
}

{
// Verify that passing an AbortSignal with custom abort error works
const controller = new AbortController();
const { signal } = controller;
const cp = spawn(process.execPath, [aliveScript], {
signal,
});

cp.on('exit', common.mustCall((code, killSignal) => {
assert.strictEqual(code, null);
assert.strictEqual(killSignal, 'SIGTERM');
}));

cp.on('error', common.mustCall((e) => {
assert.strictEqual(e.name, 'AbortError');
assert.strictEqual(e.cause.name, 'Error');
assert.strictEqual(e.cause.message, 'boom');
}));

controller.abort(new Error('boom'));
}

{
const controller = new AbortController();
const { signal } = controller;
const cp = spawn(process.execPath, [aliveScript], {
signal,
});

cp.on('exit', common.mustCall((code, killSignal) => {
assert.strictEqual(code, null);
assert.strictEqual(killSignal, 'SIGTERM');
}));

cp.on('error', common.mustCall((e) => {
assert.strictEqual(e.name, 'AbortError');
assert.strictEqual(e.cause, 'boom');
}));

controller.abort('boom');
}

{
// Verify that passing an already-aborted signal works.
const signal = AbortSignal.abort();
Expand All @@ -44,6 +86,41 @@ const aliveScript = fixtures.path('child-process-stay-alive-forever.js');
}));
}

{
// Verify that passing an already-aborted signal with custom abort error
// works.
const signal = AbortSignal.abort(new Error('boom'));
const cp = spawn(process.execPath, [aliveScript], {
signal,
});
cp.on('exit', common.mustCall((code, killSignal) => {
assert.strictEqual(code, null);
assert.strictEqual(killSignal, 'SIGTERM');
}));

cp.on('error', common.mustCall((e) => {
assert.strictEqual(e.name, 'AbortError');
assert.strictEqual(e.cause.name, 'Error');
assert.strictEqual(e.cause.message, 'boom');
}));
}

{
const signal = AbortSignal.abort('boom');
const cp = spawn(process.execPath, [aliveScript], {
signal,
});
cp.on('exit', common.mustCall((code, killSignal) => {
assert.strictEqual(code, null);
assert.strictEqual(killSignal, 'SIGTERM');
}));

cp.on('error', common.mustCall((e) => {
assert.strictEqual(e.name, 'AbortError');
assert.strictEqual(e.cause, 'boom');
}));
}

{
// Verify that waiting a bit and closing works
const controller = new AbortController();
Expand Down
⚡