child_process: add ChildProcess 'spawn' event by zenflow · Pull Request #35369 · nodejs/node

zenflow · 2020-09-27T04:30:44Z

The new event signals that the subprocess has spawned successfully and
no 'error' event will be emitted from failing to spawn.

Fixes: #35288

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

Big thanks to @bnoordhuis for his guidance & the actual changes to lib/!

zenflow · 2020-09-27T05:37:06Z

The new event comes with the specification that, if emitted, it will come "before all other events and before any data is received via stdout or stderr". This is included in documentation & tests.

With this spec:

Users can safely register event handlers after the 'spawn' event fires (without the risk of missing any event):

example

const { spawn } = require('child_process');
const { once } = require('events');

async function doSomethingWithChildProcess(){
  const subprocess = spawn(...spawnArgs);
  await once(subprocess, 'spawn');
  // At this point we know the 'exit' event hasn't been emitted yet.
  // We can expect it to be emitted in the future:
  await once(subprocess, 'exit');
}

Users can safely handle stdout/stderr streams after the 'spawn' event fires (without the risk of missing any data):

example

const { spawn } = require('child_process');
const { once } = require('events');
const streamToPromise = require('stream-to-promise');

async function doSomethingWithChildProcess(){
  const subprocess = spawn(...spawnArgs);
  await once(subprocess, 'spawn');
  return streamToPromise(subprocess.stdout);
}

Is this a good idea? Or should this spec be left out?

zenflow · 2020-09-27T06:03:40Z

Regarding the first item in the PR checklist: I can't check this because I haven't been able to run the whole test suite successfully,
on Windows (using vcbuild test) and on Windows Subsystem for Linux (using make -j4 test).

In both environments, various unrelated tests are failing (even without my changes applied).

But the test I added & the test I updated are both passing and the make lint & make test-doc tasks succeed. (I don't believe there are any other tasks to verify locally.)

If it's not super expensive, could we trigger the full CI run to verify the tests?

zenflow · 2020-09-27T06:06:25Z

The "test-asan" GitHub Action failed due to a timeout in test/parallel/test-cluster-master-kill.js. This seems unrelated to the changes in the PR, and I didn't see it failing locally. Might this be a flaky test? Could we rerun the action to see?

(edit) Yup, it was just a flaky test. All the Github Actions are passing now.

doc/api/child_process.md

nodejs-github-bot · 2020-10-15T12:01:43Z

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

github-actions · 2020-10-27T18:45:37Z

Commit Queue failed

- Loading data for nodejs/node/pull/35369 ✔ Done loading data for nodejs/node/pull/35369 ----------------------------------- PR info ------------------------------------ Title child_process: add ChildProcess 'spawn' event (#35369) Author Matthew Francis Brunetti (@zenflow, first-time contributor) Branch zenflow:child-process-spawn-event -> nodejs:master Labels author ready, child_process, semver-minor Commits 2 - child_process: add ChildProcess 'spawn' event - doc: replace 'added' metadata for ChildProcess 'spawn' event with pla… Committers 2 - Matthew Francis Brunetti - GitHub PR-URL: https://github.com/nodejs/node/pull/35369 Fixes: https://github.com/nodejs/node/issues/35288 Reviewed-By: Anna Henningsen Reviewed-By: Gireesh Punathil ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/35369 Fixes: https://github.com/nodejs/node/issues/35288 Reviewed-By: Anna Henningsen Reviewed-By: Gireesh Punathil -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-10-15T12:01:43Z: https://ci.nodejs.org/job/node-test-pull-request/33642/ - Querying data for job/node-test-pull-request/33642/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Sun, 27 Sep 2020 04:30:44 GMT ✔ Approvals: 2 ✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/35369#pullrequestreview-498505074 ✔ - Gireesh Punathil (@gireeshpunathil) (TSC): https://github.com/nodejs/node/pull/35369#pullrequestreview-509300281 -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 35369 From https://github.com/nodejs/node * branch refs/pull/35369/merge -> FETCH_HEAD ✔ Fetched commits as 0ca861745ad4..d3771c458cf4 -------------------------------------------------------------------------------- Auto-merging lib/internal/child_process.js Auto-merging doc/api/child_process.md warning: inexact rename detection was skipped due to too many files. warning: you may want to set your merge.renamelimit variable to at least 2606 and retry the command. [master 70e16da2ae] child_process: add ChildProcess 'spawn' event Author: Matthew Francis Brunetti Date: Sat Sep 26 05:02:55 2020 -0400 4 files changed, 51 insertions(+) create mode 100644 test/parallel/test-child-process-spawn-event.js Auto-merging doc/api/child_process.md warning: inexact rename detection was skipped due to too many files. warning: you may want to set your merge.renamelimit variable to at least 2606 and retry the command. [master 00c3842c02] doc: replace 'added' metadata for ChildProcess 'spawn' event with placeholder Author: Matthew Francis Brunetti Date: Sun Sep 27 15:00:07 2020 -0400 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
⚠ Found Fixes: #35288, skipping..
--------------------------------- New Message ----------------------------------
child_process: add ChildProcess 'spawn' event

The new event signals that the subprocess has spawned successfully and
no 'error' event will be emitted from failing to spawn.

Fixes: #35288

PR-URL: #35369
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Gireesh Punathil gpunathi@in.ibm.com

[detached HEAD 555eadadeb] child_process: add ChildProcess 'spawn' event
Author: Matthew Francis Brunetti zenflow87@gmail.com
Date: Sat Sep 26 05:02:55 2020 -0400
4 files changed, 51 insertions(+)
create mode 100644 test/parallel/test-child-process-spawn-event.js
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc: replace 'added' metadata for ChildProcess 'spawn' event with placeholder

Co-authored-by: mscdex mscdex@users.noreply.github.com

PR-URL: #35369
Fixes: #35288
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Gireesh Punathil gpunathi@in.ibm.com

[detached HEAD e71a315a34] doc: replace 'added' metadata for ChildProcess 'spawn' event with placeholder
Author: Matthew Francis Brunetti zenflow87@gmail.com
Date: Sun Sep 27 15:00:07 2020 -0400
1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/master.
✔ 555eadadebbfa626c8b19cca9b404f264a3178ea
✔ 4:7 Valid fixes URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 6:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ e71a315a34c8f49f11e8ebc2c6d29c7445adc80c
✔ 4:7 Valid fixes URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 3:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✖ 0:72 Title must be <= 72 columns. title-length

ℹ Please fix the commit message and try again.

Commit Queue action: https://github.com/nodejs/node/actions/runs/332031152

zenflow · 2020-10-28T02:03:35Z

The Commit Queue failed due to the title length of the second commit "doc: replace 'added' metadata for ChildProcess 'spawn' event with placeholder" (d3771c4) which was really just a fixup for the initial commit. I didn't realize it would be included as a separate commit when merged into master.

@gireeshpunathil @aduh95 I will need to rewrite and force-push my branch to fix this, is that right?
I could just fix the title of the second commit, but I would rather squash it into the first commit.

Also regarding that second commit, which was suggested by @mscdex here #35369 (comment):
When can this REPLACEME be replaced with an actual version number?
I guess we need to wait for this PR to actually land in master before we know for certain what Node.js version the event will be introduced in.. so I guess I will make a tiny follow-up PR after this PR is merged.

gireeshpunathil · 2020-10-28T02:57:32Z

@zenflow - yes pls, if you can squash the commits, I will trigger the flow again or land it manually.

The new event signals that the subprocess has spawned successfully and no 'error' event will be emitted from failing to spawn. Fixes: nodejs#35288

nodejs-github-bot · 2020-10-28T03:50:06Z

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

zenflow · 2020-10-28T05:56:44Z

CI run failed due to failed test for http2 on freebsd.. seems to be just an unrelated flaky test

nodejs-github-bot · 2020-10-28T05:58:29Z

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

gireeshpunathil · 2020-10-28T06:48:22Z

the only failure in parallel/test-worker-eventlooputil is captured in #35844

github-actions · 2020-10-28T06:59:01Z

Commit Queue failed

- Loading data for nodejs/node/pull/35369
✔  Done loading data for nodejs/node/pull/35369
----------------------------------- PR info ------------------------------------
Title      child_process: add ChildProcess 'spawn' event (#35369)
Author     Matthew Francis Brunetti  (@zenflow, first-time contributor)
Branch     zenflow:child-process-spawn-event -> nodejs:master
Labels     author ready, child_process, semver-minor
Commits    1
 - child_process: add ChildProcess 'spawn' event
Committers 1
 - Matthew Francis Brunetti 
PR-URL: https://github.com/nodejs/node/pull/35369
Fixes: https://github.com/nodejs/node/issues/35288
Reviewed-By: Anna Henningsen 
Reviewed-By: Gireesh Punathil 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35369
Fixes: https://github.com/nodejs/node/issues/35288
Reviewed-By: Anna Henningsen 
Reviewed-By: Gireesh Punathil 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - child_process: add ChildProcess 'spawn' event
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-10-28T05:58:29Z: https://ci.nodejs.org/job/node-test-pull-request/33895/
- Querying data for job/node-test-pull-request/33895/
✔  Build data downloaded
- Querying failures of job/node-test-commit/41694/
✔  Data downloaded
   ✖  1 failure(s) on the last Jenkins CI run
   ℹ  This PR was created on Sun, 27 Sep 2020 04:30:44 GMT
   ✔  Approvals: 2
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/35369#pullrequestreview-498505074
   ✔  - Gireesh Punathil (@gireeshpunathil) (TSC): https://github.com/nodejs/node/pull/35369#pullrequestreview-509300281
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/333051293

The new event signals that the subprocess has spawned successfully and no 'error' event will be emitted from failing to spawn. Fixes: #35288 PR-URL: #35369 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

gireeshpunathil · 2020-10-28T07:06:58Z

landed in 3df5afb . thanks @zenflow for the contribution! 🎉 More importantly, thanks for the perseverance you showed in converting your own feature request, and the meticulous follow-ups on the PR life cycle aspects such as CI runs etc.!

zenflow · 2020-10-28T07:31:49Z

😁 Thanks @gireeshpunathil for your help landing this!

zenflow · 2020-10-28T07:36:01Z

@gireeshpunathil I think this would be a great feature to have in v14.
Do you think a PR to backport it (following the guide on backporting) would be accepted?

This replaces a placeholder that was added in the commit which added the new ChildProcess 'spawn' event. Refs: nodejs#35369

gireeshpunathil · 2020-10-28T08:14:21Z

Yes, I guess so.

The new event signals that the subprocess has spawned successfully and no 'error' event will be emitted from failing to spawn. Fixes: #35288 PR-URL: #35369 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Notable changes: child_process: * (SEMVER-MINOR) add ChildProcess 'spawn' event (Matthew Francis Brunetti) #35369 dns: * (SEMVER-MINOR) add setLocalAddress to Resolver (Josh Dague) #34824 http: * (SEMVER-MINOR) report request start and end with diagnostics_channel (Stephen Belanger) #34895 http2: * (SEMVER-MINOR) add updateSettings to both http2 servers (Vincent Boivin) #35383 lib: * (SEMVER-MINOR) create diagnostics_channel module (Stephen Belanger) #34895 src: * (SEMVER-MINOR) add --heapsnapshot-near-heap-limit option (Joyee Cheung) #33010 v8: * (SEMVER-MINOR) implement v8.stopCoverage() (Joyee Cheung) #33807 * (SEMVER-MINOR) implement v8.takeCoverage() (Joyee Cheung) #33807 worker: * (SEMVER-MINOR) add eventLoopUtilization() (Trevor Norris) #35664 PR-URL: TODO

Notable changes: child_process: * (SEMVER-MINOR) add ChildProcess 'spawn' event (Matthew Francis Brunetti) #35369 dns: * (SEMVER-MINOR) add setLocalAddress to Resolver (Josh Dague) #34824 http: * (SEMVER-MINOR) report request start and end with diagnostics_channel (Stephen Belanger) #34895 http2: * (SEMVER-MINOR) add updateSettings to both http2 servers (Vincent Boivin) #35383 lib: * (SEMVER-MINOR) create diagnostics_channel module (Stephen Belanger) #34895 src: * (SEMVER-MINOR) add --heapsnapshot-near-heap-limit option (Joyee Cheung) #33010 v8: * (SEMVER-MINOR) implement v8.stopCoverage() (Joyee Cheung) #33807 * (SEMVER-MINOR) implement v8.takeCoverage() (Joyee Cheung) #33807 worker: * (SEMVER-MINOR) add eventLoopUtilization() (Trevor Norris) #35664 PR-URL: #35948

The new event signals that the subprocess has spawned successfully and no 'error' event will be emitted from failing to spawn. Fixes: #35288 PR-URL: #35369 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

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

mscdex reviewed Sep 27, 2020

View reviewed changes

doc/api/child_process.md Outdated Show resolved Hide resolved

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 29, 2020

addaleax approved these changes Sep 29, 2020

View reviewed changes

gireeshpunathil approved these changes Oct 15, 2020

View reviewed changes

gireeshpunathil added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2020

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

gireeshpunathil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 25, 2020

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2020

github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 27, 2020

child_process: add ChildProcess 'spawn' event

310c5db

The new event signals that the subprocess has spawned successfully and no 'error' event will be emitted from failing to spawn. Fixes: nodejs#35288

zenflow force-pushed the child-process-spawn-event branch from d3771c4 to 310c5db Compare October 28, 2020 03:14

gireeshpunathil added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2020

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

gireeshpunathil 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 Oct 28, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 28, 2020

github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 28, 2020

gireeshpunathil closed this Oct 28, 2020

zenflow added a commit to zenflow/node that referenced this pull request Oct 28, 2020

doc: Document version introducing ChildProcess 'spawn' event

75343b1

This replaces a placeholder that was added in the commit which added the new ChildProcess 'spawn' event. Refs: nodejs#35369

zenflow mentioned this pull request Oct 28, 2020

doc: Document version introducing ChildProcess 'spawn' event #35846

Closed

4 tasks

targos mentioned this pull request Nov 3, 2020

v15.1.0 release proposal #35948

Merged

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

Uh oh!

Conversation

zenflow commented Sep 27, 2020

Checklist

Uh oh!

zenflow commented Sep 27, 2020

Uh oh!

zenflow commented Sep 27, 2020

Uh oh!

zenflow commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 15, 2020

Uh oh!

github-actions bot commented Oct 27, 2020

PR-URL: #35369 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Gireesh Punathil gpunathi@in.ibm.com

PR-URL: #35369 Fixes: #35288 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Gireesh Punathil gpunathi@in.ibm.com

Uh oh!

zenflow commented Oct 28, 2020

Uh oh!

gireeshpunathil commented Oct 28, 2020

Uh oh!

nodejs-github-bot commented Oct 28, 2020

Uh oh!

zenflow commented Oct 28, 2020

Uh oh!

nodejs-github-bot commented Oct 28, 2020

Uh oh!

gireeshpunathil commented Oct 28, 2020

Uh oh!

github-actions bot commented Oct 28, 2020

Uh oh!

gireeshpunathil commented Oct 28, 2020

Uh oh!

zenflow commented Oct 28, 2020

Uh oh!

zenflow commented Oct 28, 2020

Uh oh!

gireeshpunathil commented Oct 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zenflow commented Sep 27, 2020 •

edited

Loading

PR-URL: #35369
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Gireesh Punathil gpunathi@in.ibm.com

PR-URL: #35369
Fixes: #35288
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Gireesh Punathil gpunathi@in.ibm.com