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

doc: explain process.stdout in worker_threads #31292

Closed
wants to merge 1 commit into from

Conversation

HarshithaKP
Copy link
Member

fixes: #28386

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tty Issues and PRs related to the tty subsystem. labels Jan 10, 2020
@@ -289,6 +289,10 @@ The `tty.isatty()` method returns `true` if the given `fd` is associated with
a TTY and `false` if it is not, including whenever `fd` is not a non-negative
integer.

Within `worker_threads`, this API will return `false` for process.stdout.
This is because the `process.stdout` of the worker is a pipe into the parent,
not actually a TTY.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not accurate:

> void new Worker(`console.log('isatty(1)', require('tty').isatty(1))`, {eval:true})
undefined
> isatty(1) true

tty.isatty() is generally not concerned with what thread it’s being called on, as fds are a process-wide resource.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax , I am describing fd 1, instead process.stdout.fd

void new Worker(`console.log('isatty(process.stdout.fd)', require('tty').isatty(process.stdout.fd))`, {eval:true})
undefined
satty(process.stdout.fd) false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: I am describing fd 1 -> I am NOT describing fd 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.stdout.fd is undefined in a worker thread. (Should it be?)

> void new Worker('console.log("fd:", process.stdout.fd)', {eval: true})
undefined
> fd: undefined

@addaleax addaleax added the worker Issues and PRs related to Worker support. label Jan 10, 2020
@Trott
Copy link
Member

Trott commented Jan 11, 2020

@HarshithaKP Feel free to re-open (or request that it be re-opened if the interface won't let you do it) if you plan on continuing to work on this. It seems to me that the proper place for the documentation would be inside the worker_threads documentation and/or in process documentation.

@Trott Trott closed this Jan 11, 2020
@HarshithaKP
Copy link
Member Author

@Trott , why did you close this ?

This API (isatty) exhibits uncommon behaviour in certain contexts and that's what I am documenting. According to me such document should be near the API itself.

@Trott Trott reopened this Jan 13, 2020
@Trott
Copy link
Member

Trott commented Jan 13, 2020

@Trott , why did you close this ?

Because this doesn't seem like the right approach and I thought (wrongly?) that seemed to be pretty clear. (We have over 200 open PRs so I also thought maybe it's not a bad thing to be a little more efficient about closing ones that may be stalled. They're easy enough to re-open.)

This API (isatty) exhibits uncommon behaviour in certain contexts

I don't think it exhibits uncommon behavior. If process.stdout.fd is undefined, then isatty(process.stdout.fd) should return false. Which is what it does. Unless I'm missing something (and I may indeed be missing something important), the issue here isn't anything unusual happening with isatty(). The issue is that process.stdout.fd in a worker thread may not be what a user assumes/expects. But that should be documented in the process and/or worker_thread documentation. I suppose a carefully-worded and thoughtfully-placed note about this might not be entirely out-of-place in the isatty() docs. But that sort of thing can get out of hand quickly. We don't want to include a note about this in every API that takes an fd.

@Trott
Copy link
Member

Trott commented Jan 13, 2020

The issue is that process.stdout.fd in a worker thread may not be what a user assumes/expects.

And now that I think more about it, is that a bug?

@Trott
Copy link
Member

Trott commented Jan 13, 2020

So, to summarize:

In a main thread, process.stdout.fd is (usually) 1.

In a worker thread, process.stdout.fd will (usually) be undefined.

Which leads us to the questions:

  • process.stdout.fd is not defined in the docs (as far I know, at least). Therefore, should users be able to rely on it?

  • Is the missing process.stdout.fd in worker threads a bug? Should process.stdout.fd be 1 by default?

@HarshithaKP
Copy link
Member Author

@Trott ,

But that sort of thing can get out of hand quickly. We don’t want to include a note about this in every API that takes an fd.

acknowledged.

Which leads us to the questions

I don't know. cc @addaleax

@Trott
Copy link
Member

Trott commented Jan 13, 2020

Separate issue: The isatty() doc says that fd must be a number and indicates it is required. This issue here shows that is not accurate. Perhaps we need to update the docs saying that certain (all?) non-number values will be coerced (if that's true) to a number?

@Trott
Copy link
Member

Trott commented Jan 13, 2020

Perhaps we need to update the docs saying that certain (all?) non-number values will be coerced (if that's true) to a number?

Looks like it does not coerce and simply returns false. (Only messed around a little in the REPL, didn't look at the code so I could still be wrong.) Whatever the actual case it, it should be documented, I imagine.

@addaleax
Copy link
Member

  • process.stdout.fd is not defined in the docs (as far I know, at least). Therefore, should users be able to rely on it?

Yes. I think there’s an open issue about documenting these, and removing them is not a viable option at any point in Node’s future anyway, I imagine.

  • Is the missing process.stdout.fd in worker threads a bug? Should process.stdout.fd be 1 by default?

No. process.stdout in worker threads is not associated with a file descriptor. The .fd property should be undefined.

@HarshithaKP
Copy link
Member Author

@addaleax and @Trott , That essentially boils down to documenting process.stdout.fd and its status in worker threads, leaving the doc for this api(isatty) as is. Please let me know.
And if so can I do it in this PR itself ?

@addaleax
Copy link
Member

@HarshithaKP I’d open a new PR, since it”s unrelated to the contents of this PR.

@HarshithaKP
Copy link
Member Author

Closing this as this is unrelated to the contents of PR.

HarshithaKP added a commit to HarshithaKP/node that referenced this pull request Jan 17, 2020
addaleax pushed a commit that referenced this pull request Jan 20, 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 pull request 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 pull request 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 pull request 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
doc Issues and PRs related to the documentations. tty Issues and PRs related to the tty subsystem. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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