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

tty.isatty(1) and process.stdout.isTTY in worker_threads worker return different values #28386

Closed
1999 opened this issue Jun 22, 2019 · 12 comments
Labels
tty Issues and PRs related to the tty subsystem. worker Issues and PRs related to Worker support.

Comments

@1999
Copy link

1999 commented Jun 22, 2019

  • Version: 12.4.0
  • Platform: Darwin C02WJ0U4HTDG 17.7.0 Darwin Kernel Version 17.7.0: Wed Apr 24 21:17:24 PDT 2019; root:xnu-4570.71.45~1/RELEASE_X86_64 x86_64
  • Subsystem: tty

I found out recently that in a worker which is run with worker_threads, tty.isatty(1) returns True whereas process.stdout.isTTY returns False.

This led to a weird issue in mocha - a popular library for testing: mochajs/mocha#3955

@richardlau
Copy link
Member

See #26946 (comment)

@1999
Copy link
Author

1999 commented Jun 22, 2019

Thanks for the link, interesting. But it doesn't say anything about tty.isatty API, does it?

@addaleax
Copy link
Member

@1999 This still seems like expected behaviour – fd’s are per-process, so fd 1 is going to be a TTY in one thread if it is a TTY for any thread. The problem is that in Workers, process.stdout does not refer to fd 1.

@addaleax addaleax added tty Issues and PRs related to the tty subsystem. worker Issues and PRs related to Worker support. labels Jun 22, 2019
@1999
Copy link
Author

1999 commented Jun 22, 2019

@addaleax I didn't know that tty.isatty(fd) is talking about the whole process. Maybe it's worth it to add a note about it do the documentation?

@bnoordhuis
Copy link
Member

You're welcome to open a documentation pull request but where would you add it? There are tens if not hundreds of core methods that accept a file descriptor. It'd be incongruent to single out tty.isatty().

Its behavior also isn't the least bit surprising to me but maybe that's just me.

@1999
Copy link
Author

1999 commented Jun 24, 2019

For me as a Node.JS developer, it's not clear if I should use tty.isatty(fd) or process.stdout.isTTY - they look the same from the documentation and also the documentation doesn't explicitly say that one is about the process even when we create a thread within this process and another is talking about a thread.

@addaleax
Copy link
Member

I’m having the same issue as Ben here – I don’t see a good place in the documentation that isn’t “everywhere”…

For me as a Node.JS developer, it's not clear if I should use tty.isatty(fd) or process.stdout.isTTY

We could document that tty.isatty(fd) should be used when dealing with a file descriptor, and process.stdout.isTTY when dealing with process.stdout, but that seems somewhat redundant? But that’s kind of the answer to the question of which function to use…

@devsnek
Copy link
Member

devsnek commented Jun 24, 2019

i think the question is more like (@1999 correct me if i'm wrong) when should someone be dealing with fd 1 vs when should someone be dealing with process.stdout.

@1999
Copy link
Author

1999 commented Jun 25, 2019

Exactly.

@addaleax
Copy link
Member

I would think of the fd-handling functions as low-level primitives that are there for when you really need them, but generally dealing with the corresponding JS objects makes more sense when available (e.g. 99 % of the time using fs.createReadStream() makes more sense then manually reading data using fs.read()).

In the case of isatty(), I think the use cases are very limited. I could see somebody using it for detecting TTY-ness for non-stdio fd’s (e.g. for native openpty() or tcgetattr()/tcsetattr() wrappers), or maybe when the goal is to intentionally test the per-process state, including when process.stdout has been overridden or when you’re in a Worker. But here too, I think 99 % of the time you’d want to use the convenience JS objects.

Is that helpful? Is that something that we could or should document in some way?

@devsnek
Copy link
Member

devsnek commented Jun 25, 2019

it's also worth noting that accessing process.std{out, in, err} can change their modes and that node will start processing data (which may or may not be a good thing depending on your use case)

@Fishrock123
Copy link
Contributor

Related to this, we may want to properly publicly (think, docs) expose process.stdout.fd?

HarshithaKP added a commit to HarshithaKP/node that referenced this issue Jan 10, 2020
HarshithaKP added a commit to HarshithaKP/node that referenced this issue Jan 17, 2020
codebytere pushed a commit that referenced this issue Feb 17, 2020
Fixes: #28386
Refs: #31292
Refs: nodejs/help#2136

PR-URL: #31395
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this issue Mar 14, 2020
Fixes: #28386
Refs: #31292
Refs: nodejs/help#2136

PR-URL: #31395
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this issue Mar 17, 2020
Fixes: #28386
Refs: #31292
Refs: nodejs/help#2136

PR-URL: #31395
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tty Issues and PRs related to the tty subsystem. worker Issues and PRs related to Worker support.
Projects
None yet
6 participants