doc: describe when stdout/err is sync #10884

sam-github · 2017-01-18T21:25:12Z

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

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc,console,process

addaleax

Thanks for doing this, it should be really helpful.

I think we typically use doc: as the subsystem for documentation-only updates, btw.

addaleax · 2017-01-19T07:51:27Z

doc/api/console.md

Can you move this to the other link definitions at the bottom of the file?

addaleax · 2017-01-19T07:52:25Z

doc/api/process.md

Should this be a link to the notes section, too? (ditto for the XXX below)

addaleax · 2017-01-19T07:55:31Z

doc/api/process.md

It might be good to mention that synchronous writes correspond to blocking behaviour?

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?

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

sam-github · 2017-01-19T17:39:37Z

@addaleax PTAL

sam-github · 2017-01-19T17:48:24Z

@Fishrock123 I'd appreciate a review by you, too, when you have a moment.

addaleax · 2017-01-19T17:59:16Z

doc/api/process.md

typo: may not be

addaleax · 2017-01-19T17:59:41Z

doc/api/process.md

This writeup is much more detailed than I hoped for, thanks!

sam-github · 2017-01-25T20:48:47Z

@Fishrock123 ?

Fishrock123

Left some comments but the overall direction is good @sam-github.

Fishrock123 · 2017-02-06T21:09:35Z

doc/api/console.md

, and it ?

Fishrock123 · 2017-02-06T21:11:35Z

doc/api/console.md

I think the opposite is more true: they may be asynchronous. This differers more from Node.js streams but would better reflect reality.

Also, browsers are synchronous for consoles too IIRC so async is even more different.

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

Better yeah, wording reads a bit weird to me but idk how to improve it atm. Something regarding the neither.

Fishrock123 · 2017-02-06T21:14:58Z

doc/api/process.md

Maybe we should add something like "the exit is immediate except that synchronous code may still be run from the exit event"?

added comment about exit event

Fishrock123 · 2017-02-06T21:20:14Z

doc/api/process.md

Fishrock123 · 2017-02-06T21:22:19Z

doc/api/process.md

TTYs (Terminals): might be better

Fishrock123 · 2017-02-06T21:25:25Z

doc/api/process.md

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

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

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.

@Fishrock123

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

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.

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.

Fishrock123 · 2017-02-06T21:25:45Z

doc/api/process.md

more... ~~discussion~~ information?

Fishrock123 · 2017-02-06T21:26:22Z

doc/api/process.md

in the case of output to a file due to write-back caching.

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.

Hmmm well I think it would be nice to mention something at least that people can do more research into.

Fishrock123 · 2017-02-06T21:26:59Z

doc/api/process.md

Link to isTTY?

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

sam-github · 2017-02-14T20:52:21Z

@Fishrock123 addressed most comments, have a couple open questions for you, PTAL

Fishrock123

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

sam-github · 2017-02-16T17:15:26Z

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

jasnell · 2017-03-07T16:06:27Z

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

sam-github added console Issues and PRs related to the console subsystem. doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Jan 18, 2017

nodejs-github-bot added doc Issues and PRs related to the documentations. lts-watch-v6.x labels Jan 18, 2017

sam-github force-pushed the doc-stdio-syncness branch from 568bc90 to 0dd41e1 Compare January 18, 2017 21:30

nodejs-github-bot added the lts-watch-v6.x label Jan 18, 2017

addaleax reviewed Jan 19, 2017

View reviewed changes

jasnell approved these changes Jan 19, 2017

View reviewed changes

sam-github force-pushed the doc-stdio-syncness branch from 0dd41e1 to daabdae Compare January 19, 2017 17:39

nodejs-github-bot added dont-land-on-v7.x and removed lts-watch-v6.x labels Jan 19, 2017

sam-github force-pushed the doc-stdio-syncness branch from daabdae to e658c30 Compare January 19, 2017 17:47

nodejs-github-bot added lts-watch-v6.x and removed dont-land-on-v7.x labels Jan 19, 2017

sam-github requested a review from Fishrock123 January 19, 2017 17:47

addaleax approved these changes Jan 19, 2017

View reviewed changes

sam-github force-pushed the doc-stdio-syncness branch from e658c30 to 6db4711 Compare January 19, 2017 19:30

nodejs-github-bot added the lts-watch-v6.x label Jan 19, 2017

sam-github changed the title ~~process,console: describe when stdout/err is sync~~ doc: describe when stdout/err is sync Jan 19, 2017

Fishrock123 suggested changes Feb 6, 2017

View reviewed changes

Fishrock123 self-assigned this Feb 6, 2017

sam-github force-pushed the doc-stdio-syncness branch from 6db4711 to 85f0d2d Compare February 14, 2017 20:39

nodejs-github-bot removed the lts-watch-v6.x label Feb 14, 2017

sam-github force-pushed the doc-stdio-syncness branch 3 times, most recently from 6f7ef5d to 35dfd8a Compare February 14, 2017 20:51

Fishrock123 approved these changes Feb 14, 2017

View reviewed changes

sam-github force-pushed the doc-stdio-syncness branch from 35dfd8a to 5ba00af Compare February 16, 2017 17:14

sam-github merged commit 5ba00af into nodejs:master Feb 16, 2017

Fishrock123 mentioned this pull request Feb 23, 2017

node blocks when piping to standard output. Git for Bash on Windows 10, Cygwin too. #11514

Closed

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

jasnell added the dont-land-on-v4.x label Mar 7, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

sam-github deleted the doc-stdio-syncness branch April 17, 2017 20:36

Uh oh!

doc: describe when stdout/err is sync #10884

doc: describe when stdout/err is sync #10884

Conversation

sam-github commented Jan 18, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Jan 19, 2017

Uh oh!

sam-github commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Jan 25, 2017

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github Feb 6, 2017 •

edited

Loading

Fishrock123 Feb 6, 2017 •

edited

Loading

Fishrock123 Feb 6, 2017 •

edited

Loading