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

doc: update child_process.md #19075

Closed
wants to merge 1 commit into from

Conversation

AriLFrankel
Copy link
Contributor

Add an explanation of the risk of exceeding platform pipe capacity with uncaptured output in child_process.spawn with stdio of pipe

Fixes: #4236

Checklist
Affected core subsystem(s)

doc

@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 Mar 1, 2018
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

No strong opinion about the comment itself.

in excess of that limit without capturing the output, that can cause the process
to hang. This is identical behavior to the behavior of pipes in the shell.
If you don't plan on consuming the output from the child process, create it
with { stdio: 'ignore' }.* It is possible to stream data It is possible to stream data
Copy link
Member

Choose a reason for hiding this comment

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

Seems like here is a copy paste error. It is possible to stream data is duplicated.

@@ -27,7 +27,12 @@ ls.on('close', (code) => {
```

By default, pipes for `stdin`, `stdout`, and `stderr` are established between
the parent Node.js process and the spawned child. It is possible to stream data
the parent Node.js process and the spawned child. *Note that these pipes have
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to remove Note and not to have it italic.

in excess of that limit without capturing the output, that can cause the process
to hang. This is identical behavior to the behavior of pipes in the shell.
If you don't plan on consuming the output from the child process, create it
with { stdio: 'ignore' }. It is possible to stream data through these pipes
Copy link
Member

Choose a reason for hiding this comment

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

Please place { stdio: 'ignore' } within backticks: `{ stdio: 'ignore' }`. Thanks!

the parent Node.js process and the spawned child. These pipes have
limited (and platform-specific) capacity, and if a process writes stdout
in excess of that limit without capturing the output, that can cause the process
to hang. This is identical behavior to the behavior of pipes in the shell.
Copy link
Member

Choose a reason for hiding this comment

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

Is "hang" a term we want to use? (We may already use it elsewhere, but I'm not sure it is sufficiently specific. I'm also not sure what a better term is, though. /cc @nodejs/documentation )

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Don't use behavior twice in the same sentence. Maybe this instead?: "This is identical to the behavior of pipes in the shell."

Copy link
Contributor Author

@AriLFrankel AriLFrankel Mar 1, 2018

Choose a reason for hiding this comment

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

@Trott I definitely see what you mean, maybe something like "...this can cause the child process to block waiting for the pipe buffer to accept more data." would be better? Borrowing from the description provided in the Python docs for subprocess

Copy link
Member

Choose a reason for hiding this comment

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

I like that. (I also don't fell that strongly about "hang" so if you or others prefer to keep it that way, you can ignore my comment.)

limited (and platform-specific) capacity, and if a process writes stdout
in excess of that limit without capturing the output, that can cause the process
to hang. This is identical behavior to the behavior of pipes in the shell.
If you don't plan on consuming the output from the child process, create it
Copy link
Member

Choose a reason for hiding this comment

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

Per our style guide, avoid personal pronouns in the API docs. Maybe change this sentence to this?:

Use the `{stdio: 'ignore' }` option if the output will not be consumed.

with { stdio: 'ignore' }. It is possible to stream data through these pipes
in a non-blocking way. *Note, however, that some programs use line-buffered
I/O internally. While that does not affect Node.js, it can mean that data sent
to the child process may not be immediately consumed.*
Copy link
Member

Choose a reason for hiding this comment

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

Optional nit: While we're in here, maybe remove the arbitrary-seeming italics?

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Hi, @AriLFrankel and welcome! Thank you for the pull request! Documentation changes can often attract a lot of tiny comments. Don't get discouraged!

@AriLFrankel
Copy link
Contributor Author

@Trott Thanks very much for your welcoming words 😄 , and thank you both @Trott and @BridgeAR for your feedback 👍

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM with my comment addressed.

limited (and platform-specific) capacity. If the child process writes to
stdout in excess of that limit without the output being captured, the child
process will block waiting for the pipe buffer to accept more data. This is
identical to the behavior of pipes in the shell. Use the `{stdio: 'ignore' }`
Copy link
Member

Choose a reason for hiding this comment

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

Please either remove the whitespace at the end or add one at the beginning of the object.

Add an explanation of the risk of exceeding platform pipe capacity with uncaptured output in child_process.spawn with stdio of pipe

Fixes: nodejs#4236
@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 6, 2018
@BridgeAR
Copy link
Member

@nodejs/child_process I think it would be good to have another LG here.

@AriLFrankel
Copy link
Contributor Author

@BridgeAR Do you feel this is ready to move out of "author ready"?

@BridgeAR
Copy link
Member

author-ready is a signal for contributors that this PR should be ready to land. So there is no other step before landing it.

@AriLFrankel
Copy link
Contributor Author

@BridgeAR Got ya thanks for the clarification! I was a bit confused I think by the docs for author-ready.

@addaleax
Copy link
Member

Landed in 5fdee52 🎉

@addaleax addaleax closed this Mar 23, 2018
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 23, 2018
addaleax pushed a commit that referenced this pull request Mar 23, 2018
Add an explanation of the risk of exceeding platform pipe
capacity with uncaptured output in child_process.spawn
with stdio of pipe

PR-URL: #19075
Fixes: #4236
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Mar 24, 2018
Add an explanation of the risk of exceeding platform pipe
capacity with uncaptured output in child_process.spawn
with stdio of pipe

PR-URL: #19075
Fixes: #4236
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 3, 2018
Add an explanation of the risk of exceeding platform pipe
capacity with uncaptured output in child_process.spawn
with stdio of pipe

PR-URL: #19075
Fixes: #4236
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants