child_process: add AbortSignal support by benjamingr · Pull Request #36308 · nodejs/node

benjamingr · 2020-11-28T19:44:59Z

Support AbortSignal in child_process.execFile.

Bikeshedding:

I know everywhere else it's called signal and that it's the web platform guidance - but I think we might want to deviate here and call the parameter abortSignal to prevent confusion with OS signals and killSignal. WDYT?

I will follow up with support in the other child_process APIs like fork and spawn.

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

nodejs-github-bot · 2020-11-29T19:38:36Z

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

Trott · 2020-12-01T13:28:49Z

@nodejs/child_process

Trott · 2020-12-01T13:30:02Z

doc/api/child_process.md

It's called signal above, right? (I guess this is what your bikeshedding note is about.)

No I think this is just a mistake on my part unrelated to the bikeshedding?

Trott · 2020-12-01T13:32:26Z

doc/api/child_process.md

Suggested change

For example:

addaleax · 2020-12-01T13:37:17Z

I, for one, would love more consistency between the different child_process.* methods, so if we could add this to all of the async ones at once or not at all, I think that might be nice (except maybe spawn because that’s “more async” in a way, but then again, why not also spawn? … 🙂 )

benjamingr · 2020-12-01T17:17:27Z

@addaleax I intend to add them to all of the APIs - I just find large'ish PRs are harder to land in Node and get meaningful reviews so I made smaller ones. The idea is to add this to fork and spawn later.

If you feel strongly that it should happen in the same PR I am happy to push the commit that adds it to fork and span here if you will review it (Perhaps I already have a branch with a commit on top of this one that adds it 😅 ).

Otherwise I would rather land this and then land the spawn/fork changes in a follow up PR.

nodejs-github-bot · 2020-12-05T20:21:35Z

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

PR-URL: #36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

benjamingr · 2020-12-06T10:24:15Z

Landed in 20de5f7 will follow up shortly with form and spawn

benjamingr · 2020-12-06T21:43:50Z

I think somehow ncu pushed something that shouldn't have been here - investigating

benjamingr · 2020-12-06T21:52:49Z

It looks like indeed this landed after last CI since I squashed, somehow this change that wasn't in the original (and commits) and didn't go through CI got in.

This is probably a PBKAC on my part and I landed the squashed commit with NCU

PR-URL: #36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #36435 Notable changes: * child_processes: * add AbortSignal support (Benjamin Gruenbaum) (#36308) * deps: * update ICU to 68.1 (Michaël Zasso) (#36187) * events: * support signal in EventTarget (Benjamin Gruenbaum) (#36258) * graduate Event, EventTarget, AbortController (James M Snell) (#35949) * http: * enable call chaining with setHeader() (pooja d.p) (#35924) * module: * add isPreloading indicator (James M Snell) (#36263) * stream: * support abort signal (Benjamin Gruenbaum) (#36061) * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922) * worker: * add experimental BroadcastChannel (James M Snell) (#36271)

PR-URL: nodejs#36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #36435 Notable changes: * child_processes: * add AbortSignal support (Benjamin Gruenbaum) (#36308) * deps: * update ICU to 68.1 (Michaël Zasso) (#36187) * events: * support signal in EventTarget (Benjamin Gruenbaum) (#36258) * graduate Event, EventTarget, AbortController (James M Snell) (#35949) * http: * enable call chaining with setHeader() (pooja d.p) (#35924) * module: * add isPreloading indicator (James M Snell) (#36263) * stream: * support abort signal (Benjamin Gruenbaum) (#36061) * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922) * worker: * add experimental BroadcastChannel (James M Snell) (#36271)

PR-URL: nodejs#36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #36308 Backport-PR-URL: #38386 Reviewed-By: Rich Trott <rtrott@gmail.com>

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

benjamingr requested review from addaleax and jasnell November 28, 2020 19:45

benjamingr force-pushed the child-process-abort-signal branch from 18912bb to a3cdc6c Compare November 28, 2020 19:46

benjamingr marked this pull request as ready for review November 29, 2020 14:38

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2020

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

Trott reviewed Dec 1, 2020

View reviewed changes

doc/api/child_process.md Outdated

Comment on lines 341 to 340

Copy link

Member

Trott Dec 1, 2020

Choose a reason for hiding this comment

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

Suggested change

For example:

benjamingr reacted with thumbs up emoji

Trott approved these changes Dec 1, 2020

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 1, 2020

benjamingr added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 1, 2020

benjamingr force-pushed the child-process-abort-signal branch from a3cdc6c to 1da4e0b Compare December 5, 2020 20:07

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 5, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 5, 2020

child_process: add AbortSignal support

72fac8b

benjamingr force-pushed the child-process-abort-signal branch from 1da4e0b to 72fac8b Compare December 5, 2020 21:05

benjamingr added a commit that referenced this pull request Dec 6, 2020

child_process: add AbortSignal support

20de5f7

PR-URL: #36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

benjamingr closed this Dec 6, 2020

benjamingr deleted the child-process-abort-signal branch December 6, 2020 10:24

benjamingr mentioned this pull request Dec 6, 2020

test: fix bootstrap test #36418

Closed

4 tasks

danielleadams pushed a commit that referenced this pull request Dec 7, 2020

child_process: add AbortSignal support

224a647

PR-URL: #36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

danielleadams mentioned this pull request Dec 7, 2020

v15.4.0 proposal #36435

Merged

cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020

child_process: add AbortSignal support

3cf6f6a

PR-URL: nodejs#36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021

child_process: add AbortSignal support

6bcafff

PR-URL: nodejs#36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

targos added the backport-open-v14.x label Apr 24, 2021

targos mentioned this pull request Apr 24, 2021

[v14.x] Backport AbortController and friends #38386

Closed

targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021

child_process: add AbortSignal support

9a17e1f

PR-URL: nodejs#36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021

child_process: add AbortSignal support

8277ec1

PR-URL: nodejs#36308 Reviewed-By: Rich Trott <rtrott@gmail.com>

targos pushed a commit that referenced this pull request Apr 30, 2021

child_process: add AbortSignal support

a5c0f39

PR-URL: #36308 Backport-PR-URL: #38386 Reviewed-By: Rich Trott <rtrott@gmail.com>

targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 30, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

child_process: add AbortSignal support#36308

child_process: add AbortSignal support#36308
benjamingr wants to merge 1 commit intonodejs:masterfrom
benjamingr:child-process-abort-signal

benjamingr commented Nov 28, 2020 •

edited

Loading

Uh oh!

nodejs-github-bot commented Nov 29, 2020

Uh oh!

Trott commented Dec 1, 2020

Uh oh!

Trott Dec 1, 2020

Uh oh!

benjamingr Dec 1, 2020

Uh oh!

Trott Dec 1, 2020

Uh oh!

addaleax commented Dec 1, 2020

Uh oh!

benjamingr commented Dec 1, 2020

Uh oh!

nodejs-github-bot commented Dec 5, 2020

Uh oh!

benjamingr commented Dec 6, 2020

Uh oh!

benjamingr commented Dec 6, 2020

Uh oh!

benjamingr commented Dec 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

benjamingr commented Nov 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Nov 29, 2020

Uh oh!

Trott commented Dec 1, 2020

Uh oh!

Trott Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

benjamingr Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

Trott Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 1, 2020

Uh oh!

benjamingr commented Dec 1, 2020

Uh oh!

nodejs-github-bot commented Dec 5, 2020

Uh oh!

benjamingr commented Dec 6, 2020

Uh oh!

benjamingr commented Dec 6, 2020

Uh oh!

benjamingr commented Dec 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benjamingr commented Nov 28, 2020 •

edited

Loading