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

doc: add subprocess.ref() and subprocess.unref() methods #22220

Closed
wants to merge 1 commit into from
Closed

doc: add subprocess.ref() and subprocess.unref() methods #22220

wants to merge 1 commit into from

Conversation

tlhunter
Copy link
Contributor

@tlhunter tlhunter commented Aug 9, 2018

This PR documents the subprocess.ref() and subprocess.unref() methods. The latter method does appear in the documentation as a side node in the options.detached section while the prior does not appear anywhere.

Neither takes an argument nor has a return. I copied the documentation for unref from the existing detached docs. This is now redundant, perhaps the content should be removed from detached and a link be added to ref.

The introduction version number should be correct, see git log v0.7.10 to confirm.

Checklist

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Aug 9, 2018
bengl
bengl reviewed Aug 9, 2018
View reviewed changes
doc/api/child_process.md Outdated
@@ -1094,6 +1094,27 @@ console.log(`Spawned child pid: ${grep.pid}`);
grep.stdin.end();
```

### subprocess.ref()
<!-- YAML
added: v0.7.0
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be 0.7.10? And same for unref.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! Will fix.

lundibundi
lundibundi reviewed Aug 9, 2018
View reviewed changes
doc/api/child_process.md Outdated
-->

By default, the parent will wait for the detached child to exit. To prevent
the parent from waiting for a given `subprocess`, use the `subprocess.unref()`
Copy link
Member

Choose a reason for hiding this comment

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

Maybe `for a given `subprocess` to exit`?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change made

doc/api/child_process.md Outdated
the parent from waiting for a given `subprocess`, use the `subprocess.unref()`
method. Doing so will cause the parent's event loop to not include the child in
its reference count, allowing the parent to exit independently of the child,
unless there is an established IPC channel between the child and parent.
Copy link
Member

Choose a reason for hiding this comment

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

between the child and the parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change made

@tlhunter tlhunter force-pushed the child-process-doc branch from cb9e2d8 to ebbf8e6 Compare August 9, 2018 22:13
sagirk
sagirk approved these changes Aug 10, 2018
View reviewed changes
Copy link
Contributor

@sagirk sagirk left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

A few minor nits below.

doc/api/child_process.md Outdated

Calling `subprocess.ref()` after making a call to `subprocess.unref()` will
restore the removed reference count for the child process, allowing the parent
process to wait for the child process to exit before itself exiting.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ... before exiting itself. reads better, IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

doc/api/child_process.md Outdated
-->

Calling `subprocess.ref()` after making a call to `subprocess.unref()` will
restore the removed reference count for the child process, allowing the parent
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: forcing instead of allowing seems to be the right word here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

doc/api/child_process.md Outdated

Calling `subprocess.ref()` after making a call to `subprocess.unref()` will
restore the removed reference count for the child process, allowing the parent
process to wait for the child process to exit before itself exiting.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: given the context, usage of the word process to describe parent and child can be safely omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@vsemozhetbyt
Copy link
Contributor

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/498/

@vsemozhetbyt
Copy link
Contributor

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

@vsemozhetbyt vsemozhetbyt added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Aug 10, 2018
@vsemozhetbyt
Copy link
Contributor

Landed in 7b1f3a4
Thank you!

vsemozhetbyt pushed a commit that referenced this pull request Aug 10, 2018
@tlhunter @vsemozhetbyt
doc: add subprocess.ref() and subprocess.unref()
7b1f3a4 
PR-URL: #22220
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
targos pushed a commit that referenced this pull request Aug 11, 2018
@tlhunter @targos
doc: add subprocess.ref() and subprocess.unref()
e313961 
PR-URL: #22220
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@rvagg rvagg mentioned this pull request Aug 13, 2018
Merged
@tlhunter tlhunter deleted the child-process-doc branch August 13, 2018 17:11
This was referenced Aug 16, 2018
Closed
Closed
Closed
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi left review comments

@bengl bengl bengl approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@sagirk sagirk sagirk approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.