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.

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

child_process: add signal support to spawn #36432

Closed

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
@@ -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
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

@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
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
PR-URL: #36432
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Dec 22, 2020
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
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
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
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 26, 2021
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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants