-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
stdout, stderr and stdin are all Duplex streams but documentation states otherwise #9201
Comments
FWIW the reason for this is that the |
I'd say this is probably just a bug in the inheritance chain of Duplex streams prior to v6.8.0. |
We currently don’t actually check whether the FDs 0, 1, 2 are readable/writable, and there’s no reason why they can’t be both (e.g. for TTYs or in child processes spawned by node), so keeping it to a generic (This also means that the |
Yeah, I wouldn’t count the Windows console as a real TTY. ;) |
@addaleax I don't know enough about node or linux internals but I didn't think a process could write to its own stdin. |
If you’re e.g. just running
When you specify I guess it’s weird that you can write to |
Actually, I'm not sure if writing to var spawn = require('child_process').spawn;
var args = ['-e', 'process.stdin.write("ok\\n")'];
var proc = spawn(process.execPath, args, { stdio: ['pipe'] });
proc.stdin.pipe(process.stdout); I'll file an issue. EDIT: Fixed copy/paste error in example. |
@bnoordhuis @addaleax Can you give a semi real example where this might actually be used ? |
@yonjah I’ve never seen something like that in the wild, no (well, at least not done intentionally 😄). I’d say it’s more that there’s no real reason to treat the stdio FDs different from other FDs. |
@addaleax So maybe it's a good ides to have them as a unidirection If someone has an actual need to write to stdin I guess they can always create a custom stream using the file descriptor. Though I don't know if this is actually possible with the current |
@yonjah we write to stdin in unit tests. |
@yonjah I think I generally agree with you, but more in a “maybe Node should have done this differently from the beginning” kind of way. To break this now does not sound like it’s worth the breakage to me, even if there is only a very small amount of it. |
@addaleax didn't your PR fix this? |
@Fishrock123 The inheritance one? No, and I would say that it is correct to have stdio streams as Duplexes (and judging from #9206 @bnoordhuis seems to agree), so I’d say this is really a docs-only issue |
…n states otherwise This is a fix for nodejs#9201
Thanks, landed in 1005b1d |
stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes #9201 PR-URL: #11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
stdout, stderr and stdin are all Duplex streams but documentation states otherwise Fixes nodejs/node#9201 PR-URL: nodejs/node#11194 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Documentation define stdin as readable stream and stdout, stderr as writable streams -
https://nodejs.org/api/process.html
But the actual Implementation is of a Duplex stream -
Using a unidirectional stream seems like the obvious choice but if this is not possible for some reason the documentation should be fixed.
This is a bit more annoying with node 4.x (I checked on v4.6.1) since Duplex stream do not extend Writable ones so this will actually evaluate to false -
Documentation doesn't mention stream type for v4.x but if the type cannot be changed from Duplex maybe it should mention that since this can cause some confusion
The text was updated successfully, but these errors were encountered: