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: note synchronous part of child_process.spawn #21234

Closed
wants to merge 4 commits into from

Conversation

davisjam
Copy link
Contributor

@davisjam davisjam commented Jun 9, 2018

If an application has a large RSS (MB-GB), @jorangreef observed that child_process.spawn will block the event loop for milliseconds to seconds.

This is surprising behavior and merits documentation.

See discussion on #14917.

Checklist

If an application has a large RSS (GB), jorangreef
observed that child_process.spawn will block the
event loop for ms-s.

This is surprising behavior and merits documentation.

Refs: nodejs#14917
@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 Jun 9, 2018
@vsemozhetbyt
Copy link
Contributor

@mscdex
Copy link
Contributor

mscdex commented Jun 9, 2018

Isn't this only an issue until V8 incorporates the patch mentioned in the original issue? If so, is this really worth it and are we going to remember to remove the text if/when V8 lands the proposed change?

@davisjam
Copy link
Contributor Author

davisjam commented Jun 9, 2018

@mscdex, fair point. However, I'm not aware of any movement on that patch since it was opened in January. On a related note, is there a system in place to track documentation tied to change requests to dependencies like V8 or libuv?

In favor of this PR, several people commented on #14917 and noted how long it took them to identify the root cause. Adding this documentation should help.

@Trott
Copy link
Member

Trott commented Jun 9, 2018

This might not be a good idea but maybe it is?: Include a link to the V8 issue with an explanation that after it lands and is downstreamed to Node.js core, the problem goes away. That will at least have the potential to tip someone else off that the paragraph can be removed at that time.

@davisjam
Copy link
Contributor Author

davisjam commented Jun 9, 2018

@Trott I like this idea. Something like

"As noted here, ...".

?

@Trott
Copy link
Member

Trott commented Jun 9, 2018

Maybe "As noted in V8 issue 7381" instead of "here"? But otherwise, yeah, something like that.

@davisjam
Copy link
Contributor Author

@Trott See tweaked wording in 3119e9b.

@Trott
Copy link
Member

Trott commented Jun 10, 2018

@nodejs/documentation @nodejs/child_process

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I'm -.2 on documenting this because the synopsis is already too long.

For most people this will just be a piece of irrelevant trivia or possibly a source of irrational fear ("I can't use child processes because performance" even though it's insignificant 99.99% of the time.)

If you look at e.g. python's subprocess or ruby's process docs, they don't mention this caveat either even though they're just as susceptible.

(I didn't fully check perl since it has a gazillion ways to start a child process but the few I checked don't mention it either.)

@@ -39,6 +39,12 @@ without blocking the Node.js event loop. The [`child_process.spawnSync()`][]
function provides equivalent functionality in a synchronous manner that blocks
the event loop until the spawned process either exits or is terminated.

*Note*: On some operating systems, the [`child_process.spawn()`][] method
performs memory operations synchronously before decoupling the event loop
Copy link
Member

Choose a reason for hiding this comment

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

I'd be more specific and write "On UNIX-like operating systems" and tweak the wording to clarify that it's the underlying fork() system call that is responsible for copying memory, not child_process.spawn() (directly.)

Copy link
Contributor Author

@davisjam davisjam Jun 15, 2018

Choose a reason for hiding this comment

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

I like "UNIX-like" (217acc4), but I don't think we need to go into detail on the root cause. The V8 link explains things.

@davisjam
Copy link
Contributor Author

davisjam commented Jun 15, 2018

@bnoordhuis

If you look at e.g. python's subprocess or ruby's process docs, they don't mention this caveat either even though they're just as susceptible.

True. But Node embraces a single-threaded event loop, so I think it's more important that we document potential blocking calls -- especially calls that are listed as the "asynchronous" version of things.

because the synopsis is already too long.

I agree that the synopsis is pretty long. What about nestling this note in the API documentation for child_process.spawn here?

@bnoordhuis
Copy link
Member

Yep, sounds good.

@@ -39,6 +39,12 @@ without blocking the Node.js event loop. The [`child_process.spawnSync()`][]
function provides equivalent functionality in a synchronous manner that blocks
the event loop until the spawned process either exits or is terminated.

*Note*: On UNIX-like operating systems, the [`child_process.spawn()`][] method
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit: The *Note*: part is unnecessary.

@davisjam
Copy link
Contributor Author

@bnoordhuis Check the new location.
@jasnell I dropped the "Note".

@Trott
Copy link
Member

Trott commented Jun 18, 2018

@davisjam
Copy link
Contributor Author

Looks like this PR has approval. Shall I merge?

@Trott
Copy link
Member

Trott commented Jun 19, 2018

Looks like this PR has approval. Shall I merge?

CI is green, nobody seems to object, it has multiple approvals, and it's been open more than 72 hours. GO FOR IT!

git node land 21234

jasnell pushed a commit that referenced this pull request Jun 21, 2018
If an application has a large RSS (GB), jorangreef
observed that child_process.spawn will block the
event loop for ms-s.

This is surprising behavior and merits documentation.

Refs: #14917

PR-URL: #21234
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 21, 2018

Landed in 25e5ae4

@jasnell jasnell closed this Jun 21, 2018
targos pushed a commit that referenced this pull request Jun 22, 2018
If an application has a large RSS (GB), jorangreef
observed that child_process.spawn will block the
event loop for ms-s.

This is surprising behavior and merits documentation.

Refs: #14917

PR-URL: #21234
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request Jul 3, 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.

9 participants