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

stdout, stderr and stdin are all Duplex streams but documentation states otherwise #9201

Closed
yonjah opened this issue Oct 20, 2016 · 16 comments
Closed
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. process Issues and PRs related to the process subsystem.

Comments

@yonjah
Copy link

yonjah commented Oct 20, 2016

  • Version: 6.x latest (tested on 6.9.1, 6.8.1)
  • Platform: Linux 3.10.0-327.28.3.el7.x86_64

Documentation define stdin as readable stream and stdout, stderr as writable streams -

https://nodejs.org/api/process.html

The process.stderr property returns a Writable stream equivalent to or associated with stderr (fd 2).
...
The process.stdin property returns a Readable stream equivalent to or associated with stdin (fd 0).
...
The process.stdout property returns a Writable stream equivalent to or associated with stdout (fd 1).

But the actual Implementation is of a Duplex stream -

const Stream = require('stream');
['stdin', 'stdout', 'stderr'].forEach((prop) => {
    ['Duplex', 'Writable', 'Readable'].forEach((instance) => {
        console.log(prop, `is`, instance, process[prop] instanceof Stream[instance]);
    });
});

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 -

process.stdout instanceof Stream.Writable

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

@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Oct 20, 2016
@mscdex
Copy link
Contributor

mscdex commented Oct 20, 2016

FWIW the reason for this is that the process.stdin/stdout/stderr are actually net.Socket objects, which are Duplex streams. You can see this by inspecting process.stdin.constructor for example.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Oct 20, 2016

I'd say this is probably just a bug in the inheritance chain of Duplex streams prior to v6.8.0.

@addaleax
Copy link
Member

Using a unidirectional stream seems like the obvious choice but if this is not possible for some reason the documentation should be fixed.

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 socket is probably okay.

(This also means that the .readable and .writable properties of stdio streams may be lying. Sigh.)

@vkurchatkin vkurchatkin added the doc Issues and PRs related to the documentations. label Oct 20, 2016
@princejwesley
Copy link
Contributor

@addaleax I doubt that, In the REPL case (#9189), underlying Input stream is read-only in windows.

@addaleax
Copy link
Member

Yeah, I wouldn’t count the Windows console as a real TTY. ;)

@yonjah
Copy link
Author

yonjah commented Oct 20, 2016

@addaleax I don't know enough about node or linux internals but I didn't think a process could write to its own stdin.
I understand that with child process you want to write into the child.stdin but from the child process,stdin should still only be readable no ?

@addaleax
Copy link
Member

addaleax commented Oct 20, 2016

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 node some-file.js in a POSIX terminal, the FDs 0, 1 and 2 will be indistinguishable references to the TTY – you can read from and write to any of them, each time with the same effect (which is one of the reasons why stderr and stdout can interleave in your terminal, btw). And r/w to any of them is technically supported on Node’s side, too, because the TTY streams inherit from net.Socket. As you may guess, this breaks down as soon as you redirect stdout to a file, for example.

I understand that with child process you want to write into the child.stdin but from the child process,stdin should still only be readable no ?

When you specify pipe to child_process.spawn on POSIX, you get a socket pair (there’s been a thread in nodejs/help with a bit more detail).

I guess it’s weird that you can write to stdin, but it may start to make sense once you think about it – Node doesn’t really give you any way to specify whether the child process should see the readable end or the writable end of a pipe.

@bnoordhuis
Copy link
Member

bnoordhuis commented Oct 20, 2016

Actually, I'm not sure if writing to process.stdin still works the way it used to. It looks like commit 9ae1a61 from March last year put a stop to that. It's arguably a kind of regression because a snippet like below used to work in older versions of node:

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.

@yonjah
Copy link
Author

yonjah commented Oct 21, 2016

@bnoordhuis @addaleax Can you give a semi real example where this might actually be used ?

@addaleax
Copy link
Member

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

@yonjah
Copy link
Author

yonjah commented Oct 22, 2016

@addaleax So maybe it's a good ides to have them as a unidirection
streams and throwing an error if someone tries to write to stdin or read from stdout.
So if anyone makes such a mistake it will be caught quickly.

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 net.Socket implementation

@princejwesley
Copy link
Contributor

@yonjah we write to stdin in unit tests.

@addaleax
Copy link
Member

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

@Fishrock123
Copy link
Contributor

@addaleax didn't your PR fix this?

@addaleax
Copy link
Member

addaleax commented Nov 8, 2016

@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

@Fishrock123 Fishrock123 added the good first issue Issues that are suitable for first-time contributors. label Nov 8, 2016
seppevs added a commit to seppevs/node that referenced this issue Mar 27, 2017
@fhinkel
Copy link
Member

fhinkel commented Mar 27, 2017

Thanks, landed in 1005b1d

@fhinkel fhinkel closed this as completed Mar 27, 2017
fhinkel pushed a commit that referenced this issue Mar 27, 2017
stdout, stderr and stdin are all Duplex streams but documentation
states otherwise

Fixes #9201

PR-URL: #11194
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 28, 2017
stdout, stderr and stdin are all Duplex streams but documentation
states otherwise

Fixes #9201

PR-URL: #11194
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 18, 2017
stdout, stderr and stdin are all Duplex streams but documentation
states otherwise

Fixes #9201

PR-URL: #11194
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 19, 2017
stdout, stderr and stdin are all Duplex streams but documentation
states otherwise

Fixes #9201

PR-URL: #11194
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants