From f2e3974c8e0130c6a7241932ea03ea5aecef5fa7 Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Tue, 2 May 2023 16:48:27 +0530 Subject: [PATCH 1/5] child_process: use signal.reason in child process abort Fixes: https://github.com/nodejs/node/issues/47814 --- lib/child_process.js | 7 ++-- ...rocess-exec-abortcontroller-promisified.js | 6 ++- .../test-child-process-fork-abort-signal.js | 35 +++++++++++++++++ .../test-child-process-spawn-controller.js | 39 +++++++++++++++++++ 4 files changed, 81 insertions(+), 6 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 5d3e2d63a744c2..5395672183f1f4 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -62,7 +62,6 @@ const { Buffer } = require('buffer'); const { Pipe, constants: PipeConstants } = internalBinding('pipe_wrap'); const { - AbortError, codes: errorCodes, genericNodeError, } = require('internal/errors'); @@ -712,12 +711,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', reason); } } catch (err) { child.emit('error', err); @@ -787,7 +786,7 @@ function spawn(file, args, options) { } function onAbortListener() { - abortChildProcess(child, killSignal); + abortChildProcess(child, killSignal, options.signal.reason); } } diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index 0a409f8b99e0cc..669d400dd3b631 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -18,7 +18,9 @@ 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') + assert.rejects(promise, { + name: 'AbortError' + }) .then(common.mustCall()); ac.abort(); } @@ -40,6 +42,6 @@ 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()); } diff --git a/test/parallel/test-child-process-fork-abort-signal.js b/test/parallel/test-child-process-fork-abort-signal.js index 9982444c429552..72500c45fbec80 100644 --- a/test/parallel/test-child-process-fork-abort-signal.js +++ b/test/parallel/test-child-process-fork-abort-signal.js @@ -21,6 +21,25 @@ 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, 'Error'); + strictEqual(err.message, 'boom'); + })); + process.nextTick(() => ac.abort(new Error('boom'))); +} + { // Test passing an already aborted signal to a forked child_process const signal = AbortSignal.abort(); @@ -36,6 +55,22 @@ 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, 'Error'); + strictEqual(err.message, 'boom'); + })); +} + { // Test passing a different kill signal const signal = AbortSignal.abort(); diff --git a/test/parallel/test-child-process-spawn-controller.js b/test/parallel/test-child-process-spawn-controller.js index 5199c7a09557c9..11446422ba8f03 100644 --- a/test/parallel/test-child-process-spawn-controller.js +++ b/test/parallel/test-child-process-spawn-controller.js @@ -27,6 +27,27 @@ 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, 'Error'); + assert.strictEqual(e.message, 'custom abort error'); + })); + + controller.abort(new Error('custom abort error')); +} + { // Verify that passing an already-aborted signal works. const signal = AbortSignal.abort(); @@ -44,6 +65,24 @@ 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('custom abort error')); + 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, 'Error'); + assert.strictEqual(e.message, 'custom abort error'); + })); +} + { // Verify that waiting a bit and closing works const controller = new AbortController(); From 29954c97309b357d440eee9775baf89b73110eda Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Tue, 2 May 2023 16:51:31 +0530 Subject: [PATCH 2/5] fixup! add another test --- ...ild-process-exec-abortcontroller-promisified.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index 669d400dd3b631..443783ec8a3244 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -20,11 +20,21 @@ const waitCommand = common.isWindows ? const promise = execPromisifed(waitCommand, { signal }); assert.rejects(promise, { name: 'AbortError' - }) - .then(common.mustCall()); + }).then(common.mustCall()); ac.abort(); } +{ + const ac = new AbortController(); + const signal = ac.signal; + const promise = execPromisifed(waitCommand, { signal }); + assert.rejects(promise, { + name: 'Error', + message: 'boom' + }).then(common.mustCall()); + ac.abort(new Error('boom')); +} + { assert.throws(() => { execPromisifed(waitCommand, { signal: {} }); From 6fc91a2e378db159adbead0a13a5ca37ea6f5633 Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Tue, 2 May 2023 20:24:39 +0530 Subject: [PATCH 3/5] fixup! use cause --- lib/child_process.js | 3 +- ...rocess-exec-abortcontroller-promisified.js | 35 +++++++++++-- .../test-child-process-fork-abort-signal.js | 10 ++-- .../test-child-process-spawn-controller.js | 50 ++++++++++++++++--- 4 files changed, 84 insertions(+), 14 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 5395672183f1f4..59c37b97672d39 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -62,6 +62,7 @@ const { Buffer } = require('buffer'); const { Pipe, constants: PipeConstants } = internalBinding('pipe_wrap'); const { + AbortError, codes: errorCodes, genericNodeError, } = require('internal/errors'); @@ -716,7 +717,7 @@ function abortChildProcess(child, killSignal, reason) { return; try { if (child.kill(killSignal)) { - child.emit('error', reason); + child.emit('error', new AbortError(undefined, { cause: reason })); } } catch (err) { child.emit('error', err); diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index 443783ec8a3244..81ec08ee33f348 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -25,14 +25,26 @@ const waitCommand = common.isWindows ? } { + const err = new Error('boom'); const ac = new AbortController(); const signal = ac.signal; const promise = execPromisifed(waitCommand, { signal }); assert.rejects(promise, { - name: 'Error', - message: 'boom' + name: 'AbortError', + cause: err }).then(common.mustCall()); - ac.abort(new Error('boom')); + 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'); } { @@ -55,3 +67,20 @@ const waitCommand = common.isWindows ? 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()); +} diff --git a/test/parallel/test-child-process-fork-abort-signal.js b/test/parallel/test-child-process-fork-abort-signal.js index 72500c45fbec80..b963306fb1bed3 100644 --- a/test/parallel/test-child-process-fork-abort-signal.js +++ b/test/parallel/test-child-process-fork-abort-signal.js @@ -34,8 +34,9 @@ const { fork } = require('child_process'); strictEqual(killSignal, 'SIGTERM'); })); cp.on('error', mustCall((err) => { - strictEqual(err.name, 'Error'); - strictEqual(err.message, 'boom'); + strictEqual(err.name, 'AbortError'); + strictEqual(err.cause.name, 'Error'); + strictEqual(err.cause.message, 'boom'); })); process.nextTick(() => ac.abort(new Error('boom'))); } @@ -66,8 +67,9 @@ const { fork } = require('child_process'); strictEqual(killSignal, 'SIGTERM'); })); cp.on('error', mustCall((err) => { - strictEqual(err.name, 'Error'); - strictEqual(err.message, 'boom'); + strictEqual(err.name, 'AbortError'); + strictEqual(err.cause.name, 'Error'); + strictEqual(err.cause.message, 'boom'); })); } diff --git a/test/parallel/test-child-process-spawn-controller.js b/test/parallel/test-child-process-spawn-controller.js index 11446422ba8f03..20facb09b374d5 100644 --- a/test/parallel/test-child-process-spawn-controller.js +++ b/test/parallel/test-child-process-spawn-controller.js @@ -41,11 +41,32 @@ const aliveScript = fixtures.path('child-process-stay-alive-forever.js'); })); cp.on('error', common.mustCall((e) => { - assert.strictEqual(e.name, 'Error'); - assert.strictEqual(e.message, 'custom abort error'); + 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(new Error('custom abort error')); + controller.abort('boom'); } { @@ -68,7 +89,7 @@ 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('custom abort error')); + const signal = AbortSignal.abort(new Error('boom')); const cp = spawn(process.execPath, [aliveScript], { signal, }); @@ -78,8 +99,25 @@ const aliveScript = fixtures.path('child-process-stay-alive-forever.js'); })); cp.on('error', common.mustCall((e) => { - assert.strictEqual(e.name, 'Error'); - assert.strictEqual(e.message, 'custom abort error'); + 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'); })); } From e26e5726d0899de7a293a7a1265f5f90ba7e863e Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Sun, 7 May 2023 22:02:41 +0530 Subject: [PATCH 4/5] fixup! update test Co-authored-by: Darshan Sen --- .../test-child-process-exec-abortcontroller-promisified.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index 81ec08ee33f348..99021b9c321428 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -19,7 +19,8 @@ const waitCommand = common.isWindows ? const signal = ac.signal; const promise = execPromisifed(waitCommand, { signal }); assert.rejects(promise, { - name: 'AbortError' + name: 'AbortError', + cause: signal.reason, }).then(common.mustCall()); ac.abort(); } From ece8b5e48db98baf74d738aedc35909034064fb0 Mon Sep 17 00:00:00 2001 From: Debadree Chatterjee Date: Sun, 7 May 2023 22:45:31 +0530 Subject: [PATCH 5/5] fixup! update default exception --- .../test-child-process-exec-abortcontroller-promisified.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js index 99021b9c321428..d87b502ab8e2d7 100644 --- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js +++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js @@ -20,7 +20,7 @@ const waitCommand = common.isWindows ? const promise = execPromisifed(waitCommand, { signal }); assert.rejects(promise, { name: 'AbortError', - cause: signal.reason, + cause: new DOMException('This operation was aborted', 'AbortError'), }).then(common.mustCall()); ac.abort(); }