doc: add note about stdio streams in child_process #55322
Conversation
nodejs-github-bot
added
child_process
Ed-roro
changed the title
Ed-roro
changed the title
doc/api/child_process.md
Outdated
| **NOTE:** You cannot pass `stdin` as a writable or `stdout`/`stderr` as readable | ||
| under `options.stdio`. If you do so on a stream object, there is no guarantee | ||
| that the callback will be called. If the stream errors, all callbacks will be dropped. |
There was a problem hiding this comment.
IIUC you can pass a readable where a writable should be (and vise-versa), however it's not recommended, right?
There was a problem hiding this comment.
@RedYetiDev It seems from further research, you can technically pass a readable stream where a writable is expected (and vice-versa) in Node.js, but it's not recommended because:
-
Stream Behavior: Readable and writable streams have distinct behaviors. A readable stream emits data that can be consumed, whereas a writable stream receives data to process. Swapping them might cause unexpected results, errors, or simply do nothing if the stream isn't used correctly.
-
Compatibility Issues: Some operations depend on specific stream types. For example, trying to write to a readable stream or read from a writable stream could result in runtime errors or program malfunction since the intended stream behavior is different.
-
Design Consistency: Keeping streams properly defined ensures that your code is easier to read, debug, and maintain. It's best to follow the convention that stdin is writable and stdout/stderr are readable, which aligns with how processes and streams are designed to work in Node.js.
Example of potential issues:
-
If you mistakenly treat stdout (which should be readable) as writable, you'd get an error when trying to write to it.
-
Similarly, if you pass a readable stream (like stdout) to something expecting writable input (like stdin), it would do nothing since stdout cannot accept writes.
Overall doing this is strongly discouraged and should always use streams as intended ((readable for output, writable for input) to avoid unexpected behaviors and erros.
I can edit the docs to reflect this discovery.
|
Please edit the first commit message to be a valid commit, such as |
Ed-roro
changed the title
doc/api/child_process.md
Outdated
| @@ -1061,6 +1061,15 @@ pipes between the parent and child. The value is one of the following: | |||
| corresponds to the index in the `stdio` array. The stream must have an | |||
| underlying descriptor (file streams do not start until the `'open'` event has | |||
| occurred). | |||
| **NOTE:** While it is technically possible to pass `stdin` as a writable or | |||
| `stdout`/`stderr` as readable under `options.stdio`, it is not recommended. | |||
There was a problem hiding this comment.
nit: I don't think you need to need to explicitly state that this is under options.stdio, as this whole section is referring to that optional already
|
@Ed-roro you need to update the commit message to follow guidelines, not the PR title. |
@RedYetiDev Ah that's my mistake 😅. Should i do so for the other commits as well? |
Ed-roro
force-pushed
the
update-child_process-doc
branch
from
e48014d
to
99a2351
Compare
|
No need to do the other comments, only the first one matters |
|
@RedYetiDev First commit message changed to |
RedYetiDev
added
the
author ready
aduh95
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 1c2eecd |
PR-URL: #55322 Fixes: #15714 Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#55322 Fixes: nodejs#15714 Reviewed-By: Matteo Collina <[email protected]>
This is a PR for a old issue in hope for it to be resolved and closed. This will be my first contribution to node 😊
Fixes: #15714