doc: note synchronous part of child_process.spawn #21234
Conversation
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
added
child_process
|
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? |
|
@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. |
|
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. |
|
@Trott I like this idea. Something like "As noted here, ...". ? |
|
Maybe "As noted in V8 issue 7381" instead of "here"? But otherwise, yeah, something like that. |
|
@nodejs/documentation @nodejs/child_process |
There was a problem hiding this comment.
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.)
doc/api/child_process.md
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
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.
I agree that the synopsis is pretty long. What about nestling this note in the API documentation for |
|
Yep, sounds good. |
doc/api/child_process.md
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Just a nit: The *Note*: part is unnecessary.
|
@bnoordhuis Check the new location. |
|
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!
|
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>
|
Landed in 25e5ae4 |
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>
If an application has a large RSS (MB-GB), @jorangreef observed that
child_process.spawnwill block the event loop for milliseconds to seconds.This is surprising behavior and merits documentation.
See discussion on #14917.
Checklist