Skip to content

doc: update child_process.md #19075

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
Closed
wants to merge 1 commit into from
Closed

doc: update child_process.md #19075

wants to merge 1 commit into from

Conversation

@AriLFrankel
Copy link
Contributor

@AriLFrankel AriLFrankel commented Mar 1, 2018

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
BridgeAR
BridgeAR reviewed Mar 1, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment •
edited
Loading

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.

doc/api/child_process.md Outdated
Copy link
Member

@BridgeAR BridgeAR Mar 1, 2018

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.

doc/api/child_process.md Outdated
Copy link
Member

@BridgeAR BridgeAR Mar 1, 2018

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.

Trott
Trott reviewed Mar 1, 2018
View reviewed changes
doc/api/child_process.md Outdated
Copy link
Member

@Trott Trott Mar 1, 2018

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!

Trott
Trott reviewed Mar 1, 2018
View reviewed changes
doc/api/child_process.md Outdated
Copy link
Member

@Trott Trott Mar 1, 2018

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

@Trott Trott Mar 1, 2018

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 •
edited
Loading

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

@Trott Trott Mar 1, 2018

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

Trott
Trott reviewed Mar 1, 2018
View reviewed changes
doc/api/child_process.md Outdated
Copy link
Member

@Trott Trott Mar 1, 2018

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.

Trott
Trott reviewed Mar 1, 2018
View reviewed changes
doc/api/child_process.md Outdated
Copy link
Member

@Trott Trott Mar 1, 2018

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?

Trott
Trott reviewed Mar 1, 2018
View reviewed changes
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

AriLFrankel commented Mar 2, 2018

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

BridgeAR
BridgeAR approved these changes Mar 2, 2018
View reviewed changes
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.

doc/api/child_process.md Outdated
Copy link
Member

@BridgeAR BridgeAR Mar 2, 2018

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.

@AriLFrankel
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
@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018

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

@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

BridgeAR commented Mar 11, 2018

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

@AriLFrankel
Copy link
Contributor Author

AriLFrankel commented Mar 21, 2018

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

@BridgeAR
Copy link
Member

BridgeAR commented Mar 21, 2018

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

AriLFrankel commented Mar 21, 2018

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

@addaleax
Copy link
Member

addaleax commented Mar 23, 2018

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
@AriLFrankel @addaleax
doc: update child_process.md
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>
targos pushed a commit that referenced this pull request Mar 24, 2018
@AriLFrankel @targos
doc: update child_process.md
d1c45e2 
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
@AriLFrankel @BethGriggs
doc: update child_process.md
0649148 
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
Merged
This was referenced Dec 18, 2018
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@AriLFrankel @BridgeAR @addaleax @jasnell @Trott @benjamingr @cjihrig @gireeshpunathil @BethGriggs @nodejs-github-bot