-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: describe when stdout/err is sync #10884
Conversation
568bc90
to
0dd41e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this, it should be really helpful.
I think we typically use doc:
as the subsystem for documentation-only updates, btw.
doc/api/console.md
Outdated
it writes to [`process.stdout`][] and [`process.stderr`][] which differ from | ||
other Node.js streams. See the [note on process I/O][] for more information. | ||
|
||
[note on process I/O]: process.html#process_a_note_on_process_i_o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to the other link definitions at the bottom of the file?
doc/api/process.md
Outdated
To check if Node.js is being run in a TTY context, read the `isTTY` property | ||
on `process.stderr`, `process.stdout`, or `process.stdin`: | ||
Note: `process.stderr` differs from other Node.js streams in important ways, | ||
see [XXX][] for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a link to the notes section, too? (ditto for the XXX
below)
2. They cannot be closed ([`end()`][] will throw). | ||
3. They will never emit the [`'finish'`][] event. | ||
4. Writes may be synchronous depending on the what the stream is connected to | ||
and whether the system is Windows or Unix: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to mention that synchronous
writes correspond to blocking behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we don't do this in any other place where we wrap system calls synchronously: https://nodejs.org/api/fs.html#fs_fs_writefilesync_file_data_options for example, not sure why its so necessary here, but I'm willing.
Whether a sync call blocks or not depends on the O/S, the write buffer sizes, whether whatever process on the other side of the pipe/pty/whatever is reading data, etc. We can't just say that sync calls block, because that isn't true.
How about
If the write blocks, the synchronous call won't return until it unblocks.
If not, perhaps you can provide some text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@addaleax If you are commenting top-to-bottom, you wouldn't notice, but this statement exists already, just a couple lines down:
Warning: Synchronous writes block the event loop until the write has completed.
0dd41e1
to
daabdae
Compare
@addaleax PTAL |
daabdae
to
e658c30
Compare
@Fishrock123 I'd appreciate a review by you, too, when you have a moment. |
doc/api/process.md
Outdated
under high system load, pipes that are not being read at the receiving end, or | ||
with slow terminals or file systems, its possible for the event loop to be | ||
blocked often enough and long enough to have severe negative performance | ||
impacts. This may not e a problem when writing to an interactive terminal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: may not be
doc/api/process.md
Outdated
blocked often enough and long enough to have severe negative performance | ||
impacts. This may not e a problem when writing to an interactive terminal | ||
session, but consider this particularly careful when doing production logging to | ||
the process output streams. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This writeup is much more detailed than I hoped for, thanks!
e658c30
to
6db4711
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments but the overall direction is good @sam-github.
doc/api/console.md
Outdated
* A global `console` instance configured to write to `stdout` and `stderr`. | ||
Because this object is global, it can be used without calling | ||
* A global `console` instance configured to write to [`process.stdout`][] and | ||
[`process.stderr`][]. This object is global, it can be used without calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, and it
?
doc/api/console.md
Outdated
`require('console')`. | ||
|
||
***Warning***: The global console object's methods may be synchronous, because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the opposite is more true: they may be asynchronous. This differers more from Node.js streams but would better reflect reality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, browsers are synchronous for consoles too IIRC so async is even more different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
Warning: The global console object's methods are neither consistently synchronous like the browser APIs they resemble, nor are they consistently asynchronous like all other Node.js streams. See the note ... etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better yeah, wording reads a bit weird to me but idk how to improve it atm. Something regarding the neither
.
doc/api/process.md
Outdated
The `process.exit()` method instructs Node.js to terminate the process as | ||
quickly as possible with the specified exit `code`. If the `code` is omitted, | ||
The `process.exit()` method instructs Node.js to terminate the process | ||
synchronously with the specified exit `code`. If the `code` is omitted, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add something like "the exit is immediate except that synchronous code may still be run from the exit
event"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comment about exit event
2. They cannot be closed ([`end()`][] will throw). | ||
3. They will never emit the [`'finish'`][] event. | ||
4. Writes may be synchronous depending on the what the stream is connected to | ||
and whether the system is Windows or Unix: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
doc/api/process.md
Outdated
4. Writes may be synchronous depending on the what the stream is connected to | ||
and whether the system is Windows or Unix: | ||
- Files: *synchronous* on Windows and Linux | ||
- Terminals/TTYs: *asynchronous* on Windows, *synchronous* on Unix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TTYs (Terminals):
might be better
The `process.stderr` and `process.stdout` streams are blocking when outputting | ||
to TTYs (terminals) on OS X as a workaround for the operating system's small, | ||
1kb buffer size. This is to prevent interleaving between `stdout` and `stderr`. | ||
These behaviours are partly for historical reasons, as changing them would |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. They are the way they are because of factors outside the node process.
Ideally they would all be synchronous I think. That is entirely impossible to implement Windows consoles* and also has other problems with pipes. (But Windows pipes have to block regardless.)
*There is no end-of-message notification IIRC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fishrock123 Your statement above is exactly why the it is partly for historical reasons. In your opinion pipes would be sync on Unix, but they aren't. How would you care to explain this other than "for historical reasons... backwards compat"? Also, note that I covered your point of view in the sentence below the one you comment on: "expected by some users", and that I even elaborated on why its expected with a whole paragraph. Would you like to propose some other explaination of why unix and windows are the exact opposites of each other for consoles and pipes? If one is the right way, the other is the wrong way and we have to say something about why this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Pipes on both Windows and Unix are wrong but neither is entirely our fault.
Blocking in a pipe means the process may stall forever if the downstream consumer stalls. That is not good and why pipes are (should be) async. @piscisaureus or @bnoordhuis may have more info.
It is my impression that making pipes on windows does not work correctly. See nodejs/node-v0.x-archive#3584 and 20176a9, although after reading some of the comments I am less confident about that. Maybe we could make pipes async on windows... again?
Note of course, that being async in a pipe has most of the caveats that being async in a terminal does, so it's not really good either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you care to explain this other than "for historical reasons... backwards compat"?
Would you like to propose some other explaination of why unix and windows are the exact opposites of each other for consoles and pipes? If one is the right way, the other is the wrong way and we have to say something about why this is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again:
Maybe we could make pipes async on windows... again?
I don't actually have an answer at the current time. I suppose the wording is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a doc PR, I'm not about to change how the system works in it, but I'm 💯 on aligning Windows and *nix platforms.
doc/api/process.md
Outdated
Synchronous writes avoid problems such as output written with `console.log()` or | ||
`console.write()` being unexpectedly interleaved, or not written at all if | ||
`process.exit()` is called before an asynchronous write completes. See | ||
[`process.exit()`][] for more discussion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more... discussion information?
doc/api/process.md
Outdated
[`process.exit()`][] for more discussion. | ||
|
||
***Warning***: Synchronous writes block the event loop until the write has | ||
completed. This can be near instantaneous in the case of output to a file, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the case of output to a file due to write-back caching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You think our users will know what write-back caching is? Why do you think that referencing that one kind of caching will be helpful to node users? That kind of caching is only one of the things happening deep under the hood of an I/O scheduler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm well I think it would be nice to mention something at least that people can do more research into.
session, but consider this particularly careful when doing production logging to | ||
the process output streams. | ||
|
||
To check if a stream is connected to a [TTY][] context, check the `isTTY` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to isTTY
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried, but the TTY docs don't describe it as a property, its only mentioned in the text, so that's the most specific link until the TTY docs get reworked. https://nodejs.org/api/tty.html#tty_tty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, ok
6db4711
to
85f0d2d
Compare
6f7ef5d
to
35dfd8a
Compare
@Fishrock123 addressed most comments, have a couple open questions for you, PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh, seems good enough to me, definitely better than it was
process.stdout, process.stderr, and console.log() and console.error() which use the process streams, are usually synchronous. Warn about this, and clearly describe the conditions under which they are synchronous. Fix: nodejs#10617 PR-URL: nodejs#10884 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
35dfd8a
to
5ba00af
Compare
Thanks for the review, @Fishrock123 @italoacasas doc-only change, would be nice to have in 7.x, but not a disaster if it is not |
process.stdout, process.stderr, and console.log() and console.error() which use the process streams, are usually synchronous. Warn about this, and clearly describe the conditions under which they are synchronous. Fix: nodejs#10617 PR-URL: nodejs#10884 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
process.stdout, process.stderr, and console.log() and console.error() which use the process streams, are usually synchronous. Warn about this, and clearly describe the conditions under which they are synchronous. Fix: #10617 PR-URL: #10884 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
process.stdout, process.stderr, and console.log() and console.error() which use the process streams, are usually synchronous. Warn about this, and clearly describe the conditions under which they are synchronous. Fix: #10617 PR-URL: #10884 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
needs a backport PR in order to land on v4 |
process.stdout, process.stderr, and console.log() and console.error() which use the process streams, are usually synchronous. Warn about this, and clearly describe the conditions under which they are synchronous. Fix: #10617 PR-URL: #10884 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
process.stdout, process.stderr, and console.log() and console.error()
which use the process streams, are usually synchronous. Warn about this,
and clearly describe the conditions under which they are synchronous.
Fix: #10617
Checklist
Affected core subsystem(s)
doc,console,process