Skip to content
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

Jump to bottom

child_process: add signal support to spawn #36432

Closed
wants to merge 1 commit into from
Closed

child_process: add signal support to spawn #36432

wants to merge 1 commit into from
+67 −3

Conversation

benjamingr
Copy link
Member

Next one in the "add AbortSignal to the child_process APIs"

I initially tried to make this generic and use a strategy pattern in the spawn implementation but after playing with it it was much harder to verify so I duplicated the specific code and I think it's more readable/obvious now.

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

@benjamingr benjamingr added the child_process Issues and PRs related to the child_process subsystem. label Dec 7, 2020
@benjamingr benjamingr force-pushed the add-signal-to-child-process-spawn branch from d9e78c3 to 40b97af Compare December 7, 2020 17:12
benjamingr
benjamingr commented Dec 7, 2020
View reviewed changes
doc/api/child_process.md
@@ -344,7 +344,7 @@ const { signal } = controller;
const child = execFile('node', ['--version'], { signal }, (error) => {
console.log(error); // an AbortError
});
signal.abort();
controller.abort();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an unrelated fix I noticed when reading the docs.

@benjamingr benjamingr added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 7, 2020
@benjamingr benjamingr force-pushed the add-signal-to-child-process-spawn branch from 40b97af to ff4492e Compare December 7, 2020 23:29
targos
targos reviewed Dec 8, 2020
View reviewed changes
doc/api/child_process.md Outdated Show resolved Hide resolved
@benjamingr benjamingr force-pushed the add-signal-to-child-process-spawn branch from ff4492e to 8f3cf59 Compare December 8, 2020 18:48
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 12, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 12, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/36432
✔  Done loading data for nodejs/node/pull/36432
----------------------------------- PR info ------------------------------------
Title      child_process: add signal support to spawn (#36432)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     benjamingr:add-signal-to-child-process-spawn -> nodejs:master
Labels     child_process, semver-minor
Commits    1
 - child_process: add signal support to spawn
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/36432
Reviewed-By: James M Snell 
Reviewed-By: Rich Trott 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36432
Reviewed-By: James M Snell 
Reviewed-By: Rich Trott 
--------------------------------------------------------------------------------
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-12-10T19:07:23Z: https://ci.nodejs.org/job/node-test-pull-request/34890/
- Querying data for job/node-test-pull-request/34890/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Mon, 07 Dec 2020 17:11:17 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/36432#pullrequestreview-547674438
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36432#pullrequestreview-550835437
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/417481139

@benjamingr benjamingr added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 12, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 12, 2020
@github-actions
Copy link
Contributor

Landed in d50b2ff...738cd60

@github-actions github-actions bot closed this Dec 12, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 12, 2020
@benjamingr @nodejs-github-bot
child_process: add signal support to spawn
738cd60 
PR-URL: #36432
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@benjamingr benjamingr deleted the add-signal-to-child-process-spawn branch December 12, 2020 17:20
targos pushed a commit that referenced this pull request Dec 21, 2020
@benjamingr @targos
child_process: add signal support to spawn
e449571 
PR-URL: #36432
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Dec 22, 2020
Merged
targos added a commit that referenced this pull request Dec 22, 2020
@targos
2020-12-22, Version 15.5.0 (Current)
eb84150 
Notable changes:

    child_process:
      * (SEMVER-MINOR) add signal support to spawn (Benjamin Gruenbaum) #36432
    doc:
      * add PoojaDurgad to collaborators (Pooja D P) #36511
    lib:
      * (SEMVER-MINOR) support BigInt in querystring.stringify (raisinten) #36499
    src:
      * (SEMVER-MINOR) add way to get IsolateData and allocator from Environment (Anna Henningsen) #36441
      * (SEMVER-MINOR) allow preventing SetPrepareStackTraceCallback (Shelley Vohr) #36447
    stream:
      * (SEMVER-MINOR) support abortsignal in constructor (Benjamin Gruenbaum) #36431

PR-URL: #36597
targos added a commit that referenced this pull request Dec 22, 2020
@targos
2020-12-22, Version 15.5.0 (Current)
f978628 
Notable changes:

    child_process:
      * (SEMVER-MINOR) add signal support to spawn (Benjamin Gruenbaum) #36432
    doc:
      * add PoojaDurgad to collaborators (Pooja D P) #36511
    lib:
      * (SEMVER-MINOR) support BigInt in querystring.stringify (raisinten) #36499
    src:
      * (SEMVER-MINOR) add way to get IsolateData and allocator from Environment (Anna Henningsen) #36441
      * (SEMVER-MINOR) allow preventing SetPrepareStackTraceCallback (Shelley Vohr) #36447
    stream:
      * (SEMVER-MINOR) support abortsignal in constructor (Benjamin Gruenbaum) #36431

PR-URL: #36597
targos added a commit that referenced this pull request Dec 22, 2020
@targos
2020-12-22, Version 15.5.0 (Current)
80d2fbe 
Notable changes:

    child_process:
      * (SEMVER-MINOR) add signal support to spawn (Benjamin Gruenbaum) #36432
    doc:
      * add PoojaDurgad to collaborators (Pooja D P) #36511
    lib:
      * (SEMVER-MINOR) support BigInt in querystring.stringify (raisinten) #36499
    src:
      * (SEMVER-MINOR) add way to get IsolateData and allocator from Environment (Anna Henningsen) #36441
      * (SEMVER-MINOR) allow preventing SetPrepareStackTraceCallback (Shelley Vohr) #36447
    stream:
      * (SEMVER-MINOR) support abortsignal in constructor (Benjamin Gruenbaum) #36431

PR-URL: #36597
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
@benjamingr @targos
child_process: add signal support to spawn
f2cbc23 
PR-URL: nodejs#36432
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Apr 24, 2021
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
@benjamingr @targos
child_process: add signal support to spawn
20262d1 
PR-URL: nodejs#36432
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@benjamingr @targos
child_process: add signal support to spawn
44a304d 
PR-URL: nodejs#36432
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
child_process: add signal support to spawn
0b691ce 
PR-URL: #36432
Backport-PR-URL: #38386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@benjamingr @nodejs-github-bot @jasnell @Trott @targos