From 6c0bd0b6e4a1a85f7f6eab49a52936af483f39d6 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 27 Dec 2020 09:44:19 -0800 Subject: [PATCH 1/4] test: add test for reused AbortController with execfile() Test that reusing an aborted AbortController with execfile() results in immediate SIGTERM. PR-URL: https://github.com/nodejs/node/pull/36644 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Daijiro Wachi --- test/parallel/test-child-process-execfile.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index aec80b4e2baf22..1788b733934add 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -53,11 +53,24 @@ const execOpts = { encoding: 'utf8', shell: true }; const ac = new AbortController(); const { signal } = ac; - const callback = common.mustCall((err) => { + const firstCheck = common.mustCall((err) => { assert.strictEqual(err.code, 'ABORT_ERR'); assert.strictEqual(err.name, 'AbortError'); + assert.strictEqual(err.signal, undefined); + }); + + const secondCheck = common.mustCall((err) => { + assert.strictEqual(err.code, null); + assert.strictEqual(err.name, 'Error'); + assert.strictEqual(err.signal, 'SIGTERM'); }); - execFile(process.execPath, [echoFixture, 0], { signal }, callback); + + execFile(process.execPath, [echoFixture, 0], { signal }, (err) => { + firstCheck(err); + // Test that re-using the aborted signal results in immediate SIGTERM. + execFile(process.execPath, [echoFixture, 0], { signal }, secondCheck); + }); + ac.abort(); } From a71061a9dce22cee2c08e439baeb825384d88941 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 28 Dec 2020 16:52:51 -0800 Subject: [PATCH 2/4] child_process: treat already-aborted controller as aborting If an AbortController passed to execfile() is already aborted, use the same behavior as if the controller was aborted after calling execfile(). This mimics the behavior of fetch in the browser. PR-URL: https://github.com/nodejs/node/pull/36644 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Daijiro Wachi --- lib/child_process.js | 2 ++ test/parallel/test-child-process-execfile.js | 28 ++++++++------------ 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index b67ea1c7f05eca..54c3649f29f3b9 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -376,6 +376,8 @@ function execFile(file /* , args, options, callback */) { } if (options.signal) { if (options.signal.aborted) { + if (!ex) + ex = new AbortError(); process.nextTick(() => kill()); } else { const childController = new AbortController(); diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index 1788b733934add..49a4bdbda29957 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -53,25 +53,19 @@ const execOpts = { encoding: 'utf8', shell: true }; const ac = new AbortController(); const { signal } = ac; - const firstCheck = common.mustCall((err) => { - assert.strictEqual(err.code, 'ABORT_ERR'); - assert.strictEqual(err.name, 'AbortError'); - assert.strictEqual(err.signal, undefined); - }); - - const secondCheck = common.mustCall((err) => { - assert.strictEqual(err.code, null); - assert.strictEqual(err.name, 'Error'); - assert.strictEqual(err.signal, 'SIGTERM'); - }); - - execFile(process.execPath, [echoFixture, 0], { signal }, (err) => { - firstCheck(err); - // Test that re-using the aborted signal results in immediate SIGTERM. - execFile(process.execPath, [echoFixture, 0], { signal }, secondCheck); - }); + const test = () => { + const check = common.mustCall((err) => { + assert.strictEqual(err.code, 'ABORT_ERR'); + assert.strictEqual(err.name, 'AbortError'); + assert.strictEqual(err.signal, undefined); + }); + execFile(process.execPath, [echoFixture, 0], { signal }, check); + }; + test(); ac.abort(); + // Verify that it still works the same way now that the signal is aborted. + test(); } { From 36394b6782a96e71a0ea0103147cea948c099c7d Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 28 Dec 2020 18:18:46 -0800 Subject: [PATCH 3/4] child_process: reduce abort handler code duplication Move duplicate abort handler logic into a separate function. PR-URL: https://github.com/nodejs/node/pull/36644 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Daijiro Wachi --- lib/child_process.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 54c3649f29f3b9..daa1d44e8974df 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -368,6 +368,12 @@ function execFile(file /* , args, options, callback */) { } } + function abortHandler() { + if (!ex) + ex = new AbortError(); + process.nextTick(() => kill()); + } + if (options.timeout > 0) { timeoutId = setTimeout(function delayedKill() { kill(); @@ -376,16 +382,11 @@ function execFile(file /* , args, options, callback */) { } if (options.signal) { if (options.signal.aborted) { - if (!ex) - ex = new AbortError(); - process.nextTick(() => kill()); + process.nextTick(abortHandler); } else { const childController = new AbortController(); - options.signal.addEventListener('abort', () => { - if (!ex) - ex = new AbortError(); - kill(); - }, { signal: childController.signal }); + options.signal.addEventListener('abort', abortHandler, + { signal: childController.signal }); child.once('close', () => childController.abort()); } } From 37acaf668d42ebc24d07e802674943d38f67f0c6 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Mon, 28 Dec 2020 18:29:30 -0800 Subject: [PATCH 4/4] test: add already-aborted-controller test for spawn() PR-URL: https://github.com/nodejs/node/pull/36644 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Daijiro Wachi --- .../test-child-process-spawn-controller.js | 44 ++++++++++++++----- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/test/parallel/test-child-process-spawn-controller.js b/test/parallel/test-child-process-spawn-controller.js index 399558569cc1ec..c25307907c1b26 100644 --- a/test/parallel/test-child-process-spawn-controller.js +++ b/test/parallel/test-child-process-spawn-controller.js @@ -4,18 +4,38 @@ const common = require('../common'); const assert = require('assert'); const cp = require('child_process'); -// Verify that passing an AbortSignal works -const controller = new AbortController(); -const { signal } = controller; +{ + // Verify that passing an AbortSignal works + const controller = new AbortController(); + const { signal } = controller; -const echo = cp.spawn('echo', ['fun'], { - encoding: 'utf8', - shell: true, - signal -}); + const echo = cp.spawn('echo', ['fun'], { + encoding: 'utf8', + shell: true, + signal + }); -echo.on('error', common.mustCall((e) => { - assert.strictEqual(e.name, 'AbortError'); -})); + echo.on('error', common.mustCall((e) => { + assert.strictEqual(e.name, 'AbortError'); + })); -controller.abort(); + controller.abort(); +} + +{ + // Verify that passing an already-aborted signal works. + const controller = new AbortController(); + const { signal } = controller; + + controller.abort(); + + const echo = cp.spawn('echo', ['fun'], { + encoding: 'utf8', + shell: true, + signal + }); + + echo.on('error', common.mustCall((e) => { + assert.strictEqual(e.name, 'AbortError'); + })); +}