doc: update child_process.md #19075

AriLFrankel · 2018-03-01T20:09:23Z

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

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

BridgeAR

No strong opinion about the comment itself.

BridgeAR · 2018-03-01T20:48:25Z

doc/api/child_process.md

+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


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

BridgeAR · 2018-03-01T20:49:28Z

doc/api/child_process.md

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


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

Trott · 2018-03-01T21:38:23Z

doc/api/child_process.md

+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


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

Trott · 2018-03-01T21:39:12Z

doc/api/child_process.md

+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.


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 )

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

@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

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.)

Trott · 2018-03-01T21:41:56Z

doc/api/child_process.md

+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


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.

Trott · 2018-03-01T21:43:29Z

doc/api/child_process.md

+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.*


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

Trott

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 · 2018-03-02T00:08:08Z

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

BridgeAR

LGTM with my comment addressed.

BridgeAR · 2018-03-02T19:05:09Z

doc/api/child_process.md

+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' }`


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 · 2018-03-06T18:53:46Z

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite/228/

BridgeAR · 2018-03-11T02:10:59Z

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

AriLFrankel · 2018-03-21T16:24:48Z

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

BridgeAR · 2018-03-21T16:28:18Z

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 · 2018-03-21T16:35:11Z

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

addaleax · 2018-03-23T11:51:16Z

Landed in 5fdee52 🎉

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>

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

BridgeAR reviewed Mar 1, 2018

View reviewed changes

AriLFrankel force-pushed the child_process_docs branch from 1718f30 to 31ac81d Compare March 1, 2018 20:56

Trott reviewed Mar 1, 2018

View reviewed changes

AriLFrankel force-pushed the child_process_docs branch from 31ac81d to 5e5e0ea Compare March 2, 2018 00:06

BridgeAR approved these changes Mar 2, 2018

View reviewed changes

doc: update child_process.md

01aa950

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

AriLFrankel force-pushed the child_process_docs branch from 5e5e0ea to 01aa950 Compare March 2, 2018 19:24

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

benjamingr approved these changes Mar 11, 2018

View reviewed changes

cjihrig approved these changes Mar 11, 2018

View reviewed changes

gireeshpunathil approved these changes Mar 11, 2018

View reviewed changes

jasnell approved these changes Mar 11, 2018

View reviewed changes

addaleax closed this Mar 23, 2018

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

BethGriggs added the land-on-v8.x label Dec 3, 2018

BethGriggs mentioned this pull request Dec 4, 2018

v8.14.1 proposal #24832

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: update child_process.md #19075

doc: update child_process.md #19075

AriLFrankel commented Mar 1, 2018

BridgeAR left a comment •

edited

BridgeAR Mar 1, 2018

BridgeAR Mar 1, 2018

Trott Mar 1, 2018

Trott Mar 1, 2018

Trott Mar 1, 2018

AriLFrankel Mar 1, 2018 •

edited

Trott Mar 1, 2018

Trott Mar 1, 2018

Trott Mar 1, 2018

Trott left a comment

AriLFrankel commented Mar 2, 2018

BridgeAR left a comment

BridgeAR Mar 2, 2018

BridgeAR commented Mar 6, 2018

BridgeAR commented Mar 11, 2018

AriLFrankel commented Mar 21, 2018

BridgeAR commented Mar 21, 2018

AriLFrankel commented Mar 21, 2018

addaleax commented Mar 23, 2018

doc: update child_process.md #19075

doc: update child_process.md #19075

Conversation

AriLFrankel commented Mar 1, 2018

Checklist

Affected core subsystem(s)

BridgeAR left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AriLFrankel Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

AriLFrankel commented Mar 2, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Mar 6, 2018

BridgeAR commented Mar 11, 2018

AriLFrankel commented Mar 21, 2018

BridgeAR commented Mar 21, 2018

AriLFrankel commented Mar 21, 2018

addaleax commented Mar 23, 2018

BridgeAR left a comment •

edited

AriLFrankel Mar 1, 2018 •

edited