child_process: allow promisified exec to be cancel by metcoder95 · Pull Request #34249 · nodejs/node

metcoder95 · 2020-07-07T21:27:47Z

Using new experimental AbortController, add support for promised
exec to be canceled.

Through AbortController, add support to promisified version of exec and execFile being able to be canceled using AbortController.
e.g.

const { promisify } = require('util');
const { exec } = require('child_process');
const execPromisifed = promisify(exec);

const pwdcommand = 'pwd';
const dir = '/dev';
const ac = new AbortController();
const signal = ac.signal;

execPromisifed(pwdcommand, { cwd: dir, signal })

ac.abort();

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

aduh95 · 2020-10-14T13:02:16Z

Hello 👋 Thanks a lot for your work, and sorry for the late review. I quite like this change, I think it can be useful.

I believe @jasnell would be the right person to ping because I know they've worked on Node.js AbortController implementation.

metcoder95 · 2020-10-23T14:41:59Z

Hello 👋 Thanks a lot for your work, and sorry for the late review. I quite like this change, I think it can be useful.

I believe @jasnell would be the right person to ping because I know they've worked on Node.js AbortController implementation.

Hi! @aduh95 thanks for taking a look! No worries, I know there are more important topics 😄
Congratulations on being added as a contributor btw! 🎉

jasnell · 2020-10-26T16:05:36Z

Looking good overall. One additional bit here... once the child process completes, for completeness, there really should be an additional check to see if the AbortSignal has been triggered, and if it has, the Promise should be rejected here even tho an error may not have been reported.

Got it, in that case, we should do the check even before checking if the error !== null, isn't it?

nodejs-github-bot · 2020-11-08T12:24:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34199/

metcoder95 · 2020-12-07T21:28:57Z

Is there anything else that I should do?

metcoder95 · 2020-12-22T21:37:31Z

With this approach and following this implementation, when using AbortController within spawn. The error event is getting called twice with AbortError as is being triggered by spawn and spawnWithSignal.

Should I handle it in some other way? @benjamingr

nodejs-github-bot · 2021-01-21T23:09:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/35659/

jasnell · 2021-01-23T04:03:44Z

Landed in 2242cbb

Using new AbortController, add support for promisified exec to be cancelled. PR-URL: #34249 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: assert: * change status of legacy asserts (James M Snell) #38113 async_hooks: * (SEMVER-MINOR) use new v8::Context PromiseHook API (Stephen Belanger) #36394 buffer: * (SEMVER-MINOR) introduce Blob (James M Snell) #36811 * (SEMVER-MINOR) add base64url encoding option (Filip Skokan) #36952 child_process: * (SEMVER-MINOR) allow `options.cwd` receive a URL (Khaidi Chu) #38862 * (SEMVER-MINOR) add timeout to spawn and fork (Nitzan Uziely) #37256 * (SEMVER-MINOR) allow promisified exec to be cancel (Carlos Fuentes) #34249 * (SEMVER-MINOR) add 'overlapped' stdio flag (Thiago Padilha) #29412 cli: * (SEMVER-MINOR) add -C alias for --conditions flag (Guy Bedford) #38755 * (SEMVER-MINOR) add --node-memory-debug option (Anna Henningsen) #35537 deps: * (SEMVER-MINOR) V8: cherry-pick fa4cb172cde2 (Stephen Belanger) #38577 * (SEMVER-MINOR) V8: cherry-pick 4c074516397b (Stephen Belanger) #36394 * (SEMVER-MINOR) V8: cherry-pick 5f4413194480 (Stephen Belanger) #36394 * (SEMVER-MINOR) V8: cherry-pick 272445f10927 (Stephen Belanger) #36394 * (SEMVER-MINOR) V8: backport c0fceaa0669b (Stephen Belanger) #36394 dns: * (SEMVER-MINOR) add "tries" option to Resolve options (Luan Devecchi) #39610 * (SEMVER-MINOR) allow `--dns-result-order` to change default dns verbatim (Ouyang Yadong) #38099 doc: * (SEMVER-MINOR) add missing change to resolver ctor (Luan Devecchi) #39610 * refactor fs docs structure (Michaël Zasso) #37170 errors: * (SEMVER-MINOR) remove experimental from --enable-source-maps (Benjamin Coe) #37362 esm: * deprecate legacy main lookup for modules (Guy Bedford) #36918 fs: * (SEMVER-MINOR) allow empty string for temp directory prefix (Voltrex) #39028 * (SEMVER-MINOR) allow no-params fsPromises fileHandle read (Nitzan Uziely) #38287 * (SEMVER-MINOR) add support for async iterators to `fsPromises.writeFile` (HiroyukiYagihashi) #37490 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * (SEMVER-MINOR) add fsPromises.watch() (James M Snell) #37179 * (SEMVER-MINOR) allow `position` parameter to be a `BigInt` in read and readSync (Darshan Sen) #36190 http2: * (SEMVER-MINOR) add support for sensitive headers (Anna Henningsen) #34145 * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978 inspector: * mark as stable (Gireesh Punathil) #37748 module: * (SEMVER-MINOR) add support for `URL` to `import.meta.resolve` (Antoine du Hamel) #38587 * (SEMVER-MINOR) add support for `node:`‑prefixed `require(…)` calls (ExE Boss) #37246 net: * (SEMVER-MINOR) allow net.BlockList to use net.SocketAddress objects (James M Snell) #37917 * (SEMVER-MINOR) add SocketAddress class (James M Snell) #37917 * (SEMVER-MINOR) make net.BlockList cloneable (James M Snell) #37917 * (SEMVER-MINOR) make blocklist family case insensitive (James M Snell) #34864 * (SEMVER-MINOR) introduce net.BlockList (James M Snell) #34625 node-api: * (SEMVER-MINOR) allow retrieval of add-on file name (Gabriel Schulhof) #37195 os: * (SEMVER-MINOR) add os.devNull (Luigi Pinca) #38569 perf_hooks: * (SEMVER-MINOR) introduce createHistogram (James M Snell) #37155 process: * (SEMVER-MINOR) add api to enable source-maps programmatically (legendecas) #39085 * (SEMVER-MINOR) add `'worker'` event (James M Snell) #38659 * (SEMVER-MINOR) add direct access to rss without iterating pages (Adrien Maret) #34291 readline: * (SEMVER-MINOR) add AbortSignal support to interface (Nitzan Uziely) #37932 * (SEMVER-MINOR) add support for the AbortController to the question method (Mattias Runge-Broberg) #33676 * (SEMVER-MINOR) add history event and option to set initial history (Mattias Runge-Broberg) #33662 repl: * (SEMVER-MINOR) add auto‑completion for `node:`‑prefixed `require(…)` calls (ExE Boss) #37246 src: * (SEMVER-MINOR) call overload ctor from the original ctor (Darshan Sen) #39768 * (SEMVER-MINOR) add a constructor overload for CallbackScope (Darshan Sen) #39768 * (SEMVER-MINOR) fix align in cares_wrap.h (Luan) #39610 * (SEMVER-MINOR) allow to negate boolean CLI flags (Michaël Zasso) #39023 * (SEMVER-MINOR) add --heapsnapshot-near-heap-limit option (Joyee Cheung) #33010 * (SEMVER-MINOR) move node_binding to modern THROW_ERR* (James M Snell) #35469 * (SEMVER-MINOR) add way to get IsolateData and allocator from Environment (Anna Henningsen) #36441 * (SEMVER-MINOR) allow preventing SetPrepareStackTraceCallback (Shelley Vohr) #36447 * (SEMVER-MINOR) add maybe versions of EmitExit and EmitBeforeExit (Anna Henningsen) #35486 stream: * (SEMVER-MINOR) add readableDidRead if has been read from (Robert Nagy) #39589 * (SEMVER-MINOR) pipeline accept Buffer as a valid first argument (Nitzan Uziely) #37739 test: * (SEMVER-MINOR) add wpt tests for Blob (Michaël Zasso) #36811 tls: * (SEMVER-MINOR) allow reading data into a static buffer (Andrey Pechkurov) #35753 tools: * (SEMVER-MINOR) add `Worker` to type-parser (James M Snell) #38659 url: * (SEMVER-MINOR) expose urlToHttpOptions utility (Yongsheng Zhang) #35960 util: * (SEMVER-MINOR) expose toUSVString (Robert Nagy) #39814 v8: * (SEMVER-MINOR) implement v8.stopCoverage() (Joyee Cheung) #33807 * (SEMVER-MINOR) implement v8.takeCoverage() (Joyee Cheung) #33807 worker: * (SEMVER-MINOR) add setEnvironmentData/getEnvironmentData (James M Snell) #37486 PR-URL: TODO

Notable changes: assert: * change status of legacy asserts (James M Snell) #38113 buffer: * (SEMVER-MINOR) introduce Blob (James M Snell) #36811 * (SEMVER-MINOR) add base64url encoding option (Filip Skokan) #36952 child_process: * (SEMVER-MINOR) allow `options.cwd` receive a URL (Khaidi Chu) #38862 * (SEMVER-MINOR) add timeout to spawn and fork (Nitzan Uziely) #37256 * (SEMVER-MINOR) allow promisified exec to be cancel (Carlos Fuentes) #34249 * (SEMVER-MINOR) add 'overlapped' stdio flag (Thiago Padilha) #29412 cli: * (SEMVER-MINOR) add -C alias for --conditions flag (Guy Bedford) #38755 * (SEMVER-MINOR) add --node-memory-debug option (Anna Henningsen) #35537 dns: * (SEMVER-MINOR) add "tries" option to Resolve options (Luan Devecchi) #39610 * (SEMVER-MINOR) allow `--dns-result-order` to change default dns verbatim (Ouyang Yadong) #38099 doc: * (SEMVER-MINOR) add missing change to resolver ctor (Luan Devecchi) #39610 * refactor fs docs structure (James M Snell) #37170 errors: * (SEMVER-MINOR) remove experimental from --enable-source-maps (Benjamin Coe) #37362 esm: * deprecate legacy main lookup for modules (Guy Bedford) #36918 fs: * (SEMVER-MINOR) allow empty string for temp directory prefix (Voltrex) #39028 * (SEMVER-MINOR) allow no-params fsPromises fileHandle read (Nitzan Uziely) #38287 * (SEMVER-MINOR) add support for async iterators to `fsPromises.writeFile` (HiroyukiYagihashi) #37490 * improve fsPromises readFile performance (Nitzan Uziely) #37608 * (SEMVER-MINOR) add fsPromises.watch() (James M Snell) #37179 * (SEMVER-MINOR) allow `position` parameter to be a `BigInt` in read and readSync (Darshan Sen) #36190 http2: * (SEMVER-MINOR) add support for sensitive headers (Anna Henningsen) #34145 * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978 inspector: * mark as stable (Gireesh Punathil) #37748 module: * (SEMVER-MINOR) add support for `URL` to `import.meta.resolve` (Antoine du Hamel) #38587 * (SEMVER-MINOR) add support for `node:`‑prefixed `require(…)` calls (ExE Boss) #37246 net: * (SEMVER-MINOR) introduce net.BlockList (James M Snell) #34625 node-api: * (SEMVER-MINOR) allow retrieval of add-on file name (Gabriel Schulhof) #37195 os: * (SEMVER-MINOR) add os.devNull (Luigi Pinca) #38569 perf_hooks: * (SEMVER-MINOR) introduce createHistogram (James M Snell) #37155 process: * (SEMVER-MINOR) add api to enable source-maps programmatically (legendecas) #39085 * (SEMVER-MINOR) add `'worker'` event (James M Snell) #38659 * (SEMVER-MINOR) add direct access to rss without iterating pages (Adrien Maret) #34291 readline: * (SEMVER-MINOR) add AbortSignal support to interface (Nitzan Uziely) #37932 * (SEMVER-MINOR) add support for the AbortController to the question method (Mattias Runge-Broberg) #33676 * (SEMVER-MINOR) add history event and option to set initial history (Mattias Runge-Broberg) #33662 repl: * (SEMVER-MINOR) add auto‑completion for `node:`‑prefixed `require(…)` calls (ExE Boss) #37246 src: * (SEMVER-MINOR) call overload ctor from the original ctor (Darshan Sen) #39768 * (SEMVER-MINOR) add a constructor overload for CallbackScope (Darshan Sen) #39768 * (SEMVER-MINOR) allow to negate boolean CLI flags (Michaël Zasso) #39023 * (SEMVER-MINOR) add --heapsnapshot-near-heap-limit option (Joyee Cheung) #33010 * (SEMVER-MINOR) add way to get IsolateData and allocator from Environment (Anna Henningsen) #36441 * (SEMVER-MINOR) allow preventing SetPrepareStackTraceCallback (Shelley Vohr) #36447 * (SEMVER-MINOR) add maybe versions of EmitExit and EmitBeforeExit (Anna Henningsen) #35486 stream: * (SEMVER-MINOR) add readableDidRead if has been read from (Robert Nagy) #39589 * (SEMVER-MINOR) pipeline accept Buffer as a valid first argument (Nitzan Uziely) #37739 tls: * (SEMVER-MINOR) allow reading data into a static buffer (Andrey Pechkurov) #35753 tools: * (SEMVER-MINOR) add `Worker` to type-parser (James M Snell) #38659 url: * (SEMVER-MINOR) expose urlToHttpOptions utility (Yongsheng Zhang) #35960 util: * (SEMVER-MINOR) expose toUSVString (Robert Nagy) #39814 v8: * (SEMVER-MINOR) implement v8.stopCoverage() (Joyee Cheung) #33807 * (SEMVER-MINOR) implement v8.takeCoverage() (Joyee Cheung) #33807 worker: * (SEMVER-MINOR) add setEnvironmentData/getEnvironmentData (James M Snell) #37486 PR-URL: #39990

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jul 7, 2020

metcoder95 force-pushed the cancel-promisified-child-process branch 2 times, most recently from a42d2cd to 673c505 Compare July 15, 2020 21:15

metcoder95 force-pushed the cancel-promisified-child-process branch from 673c505 to c9e0700 Compare August 13, 2020 21:47

aduh95 requested a review from jasnell October 14, 2020 13:02

jasnell reviewed Oct 14, 2020

View reviewed changes

Comment thread lib/child_process.js Outdated

jasnell reviewed Oct 14, 2020

View reviewed changes

Comment thread lib/child_process.js Outdated

jasnell reviewed Oct 14, 2020

View reviewed changes

Comment thread lib/child_process.js Outdated

jasnell reviewed Oct 14, 2020

View reviewed changes

Comment thread test/parallel/test-child-process-exec-abortcontroller-promisified.js Outdated

jasnell reviewed Oct 16, 2020

View reviewed changes

Comment thread lib/child_process.js Outdated

jasnell reviewed Oct 16, 2020

View reviewed changes

Comment thread lib/child_process.js Outdated

jasnell reviewed Oct 16, 2020

View reviewed changes

Comment thread lib/child_process.js Outdated

metcoder95 force-pushed the cancel-promisified-child-process branch from c9e0700 to dedada8 Compare October 23, 2020 14:39

metcoder95 requested a review from jasnell October 23, 2020 14:42

jasnell reviewed Oct 26, 2020

View reviewed changes

metcoder95 force-pushed the cancel-promisified-child-process branch from d0c4cd2 to fe3e7c6 Compare October 28, 2020 17:45

metcoder95 requested a review from jasnell October 28, 2020 17:50

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. review wanted PRs that need reviews. labels Nov 8, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020

metcoder95 force-pushed the cancel-promisified-child-process branch from 67acaeb to 8a1f682 Compare December 7, 2020 21:27

benjamingr reviewed Dec 18, 2020

View reviewed changes

Comment thread lib/child_process.js Outdated

benjamingr reviewed Dec 18, 2020

View reviewed changes

Comment thread lib/child_process.js Outdated

metcoder95 force-pushed the cancel-promisified-child-process branch from 8a1f682 to 0db89e4 Compare December 22, 2020 21:29

metcoder95 requested a review from benjamingr December 22, 2020 21:33

metcoder95 added 2 commits January 18, 2021 23:49

child_process: revert refactor to spawn

2305734

child_process: fix tests

a60b935

metcoder95 requested a review from aduh95 January 18, 2021 22:57

aduh95 reviewed Jan 18, 2021

View reviewed changes

metcoder95 added 3 commits January 21, 2021 23:03

child_process: refactor

fe077b0

child_process: refactor tests

72ff68a

child_process: refactor listeners args

026021a

metcoder95 requested a review from aduh95 January 21, 2021 22:55

aduh95 reviewed Jan 21, 2021

View reviewed changes

Comment thread lib/child_process.js Outdated

Comment thread lib/child_process.js

child_process: inline listener

c093035

aduh95 approved these changes Jan 21, 2021

View reviewed changes

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 21, 2021

jasnell approved these changes Jan 23, 2021

View reviewed changes

jasnell closed this Jan 23, 2021

targos mentioned this pull request Feb 2, 2021

v15.8.0 release proposal #37183

Merged

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label May 25, 2021

targos mentioned this pull request Sep 4, 2021

v14.18.0 release proposal #39990

Merged

Uh oh!

Uh oh!

Conversation

metcoder95 commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

aduh95 commented Oct 14, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metcoder95 commented Oct 23, 2020

Uh oh!

jasnell Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

metcoder95 Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Nov 8, 2020

Uh oh!

metcoder95 commented Dec 7, 2020

Uh oh!

Uh oh!

Uh oh!

metcoder95 commented Dec 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 21, 2021

Uh oh!

jasnell commented Jan 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

metcoder95 commented Jul 7, 2020 •

edited

Loading