GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — child_process: add signal support to spawn by benjamingr · Pull Request #36432 · nodejs/node

Conversation

@benjamingr

@benjamingr benjamingr commented Dec 7, 2020

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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
Comment thread doc/api/child_process.md Outdated

@benjamingr benjamingr Dec 7, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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
Comment thread doc/api/child_process.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr
child_process: add signal support to spawn
8f3cf59
@benjamingr
benjamingr force-pushed the add-signal-to-child-process-spawn branch from ff4492e to 8f3cf59 Compare December 8, 2020 18:48
jasnell
jasnell approved these changes Dec 8, 2020
View reviewed changes
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 10, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 10, 2020
@nodejs-github-bot

nodejs-github-bot commented Dec 10, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Dec 12, 2020
View reviewed changes
@benjamingr benjamingr added the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 12, 2020
@github-actions github-actions Bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Dec 12, 2020
@github-actions

github-actions Bot commented Dec 12, 2020

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Dec 12, 2020
@github-actions github-actions Bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 12, 2020
@github-actions

github-actions Bot commented Dec 12, 2020

Copy link
Copy Markdown
Contributor

Landed in d50b2ff...738cd60
benjamingr reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Dec 22, 2020
v15.5.0 release proposal #36597
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 targos added the backport-open-v14.x label Apr 24, 2021
@targos 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 30, 2021
@benjamingr @targos
child_process: add signal support to spawn
44a304d 
PR-URL: nodejs#36432
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 30, 2021
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@Trott Trott Trott approved these changes
+1 more reviewer
@targos targos targos left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@benjamingr @nodejs-github-bot @jasnell @Trott @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.