tty: set the handle to blocking mode #6816

Fishrock123 · 2016-05-17T17:00:53Z

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

tty, process

Description of change

See discussion at:

cc @nodejs/ctc @isaacs @ksc @saghul etc

Note: we still have more investigation to do but this is looking like it is probably the correct thing to do.

indutny · 2016-05-17T17:03:22Z

I have a deja vu here... Is it like 3rd time we re-enable it?

indutny · 2016-05-17T17:04:12Z

Am I right that running node script.js | some-prog-that-does-not-read-stdin will make script.js eventually hang on writes?

addaleax · 2016-05-17T17:06:15Z

@indutny This should only affect ttys, so, no, I don’t think so.

indutny · 2016-05-17T17:07:49Z

@addaleax oh, right! Thank you. LGTM then, I have no issues with this.

mscdex · 2016-05-17T17:08:25Z

Shouldn't there at least be an official way to change the behavior (in case someone needs/wants async)?

Fishrock123 · 2016-05-17T17:11:13Z

I have a deja vu here... Is it like 3rd time we re-enable it?

Haha, not quite. ;)

@mscdex Official? Idk. But you can still do tty._handle.setBlocking(false) afaik.

That being said, I can't think of a reason you'd need async in a TTY? Maybe if only one stream is to a TTY?

mscdex · 2016-05-17T17:20:46Z

@Fishrock123 I meant a way without accessing an underscored property.

saghul · 2016-05-17T17:56:30Z

LGTM. (fingers crossed so we don't open a can of worms).

/cc @bnoordhuis I believe this whole thing make some sense in your brain, I'd love to see your yay or nay here.

Builds on nodejs#6816, adds API for explicitly setting non-blocking mode without relying on `_`-prefixed internal property. Adds documentation.

jasnell · 2016-05-17T18:50:18Z

@mscdex ... like this? jasnell@4982550

jasnell · 2016-05-17T19:19:52Z

This PR should include a documentation update that (a) describes the default behavior of tty.ReadStream and (b) demonstrates a supported way of making it unblocking. I'm not comfortable with this landing until at least (a) is added.

Fishrock123 · 2016-05-17T19:44:04Z

@jasnell So, there are a couple issues with your API proposal.

The biggest is that actually creating TTY streams isn't particularly supported:

In normal circumstances, process.stdout will be the only tty.WriteStream instance ever created (and only when isatty(1) is true).

In fact, I've tried to make tty streams before and found it's virtually impossible. You can only make one if the fd is a tty and it is not already in use, which is basically never. I actually don't know of a case when you could ever make one manually without severe hackery.

However, exposing a setBlocking method publicly also causes some concerns:

The existing problem that stdio streams do not have consistent APIs depending on the stream type is yet again amplified (very difficult to fix)
It doesn't work for pipes, but putting this functionality into net.Socket publicly is harmful because of TCP connections.
I'm not sure unblocking is ever valuable for this, in any manor that isn't strictly theoretical.
- Same goes for pipes, where blocking only has theoretical niceties.

describes the default behavior of tty.ReadStream

I doubt that is useful due to the above, or the correct place to put it, since no-one will go looking for the TTY module from console.log() or process.std{in|out|err}.

I will add a note to where the existing note is for file piping though: https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_process_stderr

Fishrock123 · 2016-05-17T20:03:36Z

Updated with docs note.

addaleax · 2016-05-17T20:06:25Z

doc/api/process.md

+  - (Although disks are fast and operating systems normally employ write-back
+    caching so this is very uncommon.)
+4. Writes __will__ block by default if output is going to a TTY (a terminal) or
+Node.js is running on windows.


nit: It’s usually stylized as Windows in the docs.

addaleax · 2016-05-17T20:06:41Z

LGTM

mscdex · 2016-05-17T20:12:25Z

@jasnell Something like that would be ideal of course, but even if there was a way to simply change the behavior once on startup. I'm not sure what that would look like or if enough people would be interested in supporting something like that though ...

jasnell · 2016-05-17T20:51:40Z

@mscdex ... I'm definitely for it (either API or command line switch) but given the discussion I doubt we'd get consensus to add it so I'm going to just drop it.

jasnell · 2016-05-17T20:52:33Z

I would say that this looks like a semver-major change but I know there will be disagreement with that.

addaleax · 2016-05-17T20:54:23Z

@jasnell Could you give an example of what you imagine might break because of this change?

Fishrock123 · 2016-05-17T20:55:13Z

I can imagine a lot of things that are already breaking without this change. ;)

Fishrock123 · 2016-05-17T20:55:56Z

@jasnell @mscdex I'd be open to it if there is actually a reason to do it, but no reasons are appearing...?

jasnell · 2016-05-17T20:58:21Z

@addaleax ... not off hand but it is a change in the established default behavior, which would technically qualify it as a semver-major. Note that I didn't add the label ;-) ... I'm quite certain that I'm in the minority.

Fishrock123 · 2016-05-17T21:04:26Z

but it is a change in the established default behavior,

Sorta. It gets tricky here because on OS X it appears to have previously actually blocked when it wasn't supposed to so the majority of the CLI users never noticed this because it never changed on their platform sooner. So... the previous behavior is actually more established there, and due to previous versions of node.js, also across the board I think. Additionally, I'm unsure if console.log() blocks in the browser but I would not be particularly surprised if it did.

mscdex · 2016-05-17T21:19:41Z

I'm more inclined to say semver-major too since the non-blocking behavior in general has existed for quite a long time now in node, so I don't view this change as a "oops we just introduced a bug, let's fix it quickly" kind of situation. shrug

addaleax · 2016-05-17T21:26:08Z

As far as I can tell, the only visible difference would be in timing, no? And unlike with pipes, for ttys there is (in 99.99 % of practically occurring situations) an active consumer on the other end, so writing won’t block indefinitely because of this.

Trott · 2016-07-15T20:52:33Z

doc/api/process.md

-redirected to a file (although disks are fast and operating systems normally
-employ write-back caching so it should be a very rare occurrence indeed.)
+Note: `process.stderr` and `process.stdout` differ from other Node.js streams
+in a couple ways:


Same nits as above.

Trott · 2016-07-15T20:54:16Z

Left some nits on the documentation prose, but nothing that I think must happen before landing. (In other words: Don't interpret my nits as me objecting to this landing.)

MylesBorins · 2016-07-15T21:57:59Z

@Fishrock123 is this something that should be included in v4.5.0?

saghul · 2016-07-16T07:07:17Z

@kzc A note to remember: when stdio was made blocking on macOS that was meant to restore the behavior aas in v4.x On other Unix systems it never blocked. We are changing that now. Is it a good or a bad thing? I no longer know.

kzc · 2016-07-16T13:27:35Z

@saghul I know the history well. The blocking on Mac was initially inadvertently introduced as result of a bug pre-node 4.x when trying to re-open the tty - ironically to not block stdio - and somehow this blocking stdio behavior on Mac became grandfathered as the status quo.

Whether this PR is good or bad, it just addresses that inconsistency on UNIX platforms. #6816 (comment) sums up my view. Blocking stdio tty alone is not enough to fix the many long standing problems with node stdio. Something like #6773 is still needed to resolve truncated piped stdout/stderr and some similar fix is still required to flush piped stdio in the event of an uncaught exception. Once node is in the process of an abrupt exit and the event loop is no longer running (although not destroyed) there's zero downside to flushing stdout and stderr as users would reasonably expect.

As I don't watch these node issue/PR threads any longer please use my @ handle if you want to get any other feedback.

saghul · 2016-07-17T07:26:14Z

@kzc Sure, I know we are currently not in a very good place. Thanks for your efforts so far.

Fishrock123 · 2016-07-18T08:33:47Z

@thealphanerd Give it's potentially debatable if this is a major, I'm going to say no.

Fishrock123 · 2016-07-18T08:35:09Z

I consider it a non-breaking bugfix given that:

There are no shortage of complaints about the current behavior
There is no evidence this could cause problems

Refs: nodejs#1771 Refs: nodejs#6456 Refs: nodejs#6773 Refs: nodejs#7743 PR-URL: nodejs#6816

Fishrock123 · 2016-07-18T08:39:28Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/3324/

@jasnell Could you take a look at this again and let us know if you are comfortable with it?

thefourtheye · 2016-07-18T12:58:42Z

doc/api/process.md

+Note: `process.stderr` and `process.stdout` differ from other Node.js streams
+in several ways:
+1. They cannot be closed ([`end()`][] will throw).
+2. They never emit the [`'finish'`][] event.


We don't need the additional square brackets for links I guess.

Huh? These should use links defined at the bottom of the file...?

If we have defined them at the bottom the empty square brackets are not necessary I think

jasnell · 2016-07-20T16:17:19Z

LGTM but we'll need to message this properly. While I believe there is consensus around landing this as a bug fix, it is technically a fairly significant change in behavior that should technically be tagged as a semver-major. There should be a release candidate cycle for any release that this goes out in just to be on the safe side.

Fishrock123 · 2016-07-21T08:59:27Z

If it must land in a semver-major, it could.

I do not think it is reasonable to see this as an API change however, so I think it may be worth our while to get this in before v6 goes LTS.

addaleax · 2016-08-08T13:34:41Z

This still LGTM, and I’m still +1 on not seeing this as a semver-major because afaict, this doesn’t necessarily represent a visible behavioural change.

Fishrock123 · 2016-08-08T18:14:45Z

Guess we're clear to land... or as clear as it will get.

@jasnell how do you propose we message it?

jasnell · 2016-08-08T18:19:21Z

Twitter PSA at the very least (I can do that in just a minute). Even tho this will land as a patch, we need to make sure it's called out explicitly in the release notes. Beyond that, I'm not sure what else.

jasnell · 2016-08-08T18:20:13Z

I would definitely recommend that we do a release candidate for whatever release this lands in.

addaleax · 2016-08-10T20:01:15Z

I’ll be landing this later today if there are no objections and nobody else does before me, this has been lying around long enough.

edit: One more CI: https://ci.nodejs.org/job/node-test-commit/4501/

addaleax · 2016-08-10T22:32:38Z

Landed in ab3306a

Refs: #1771 Refs: #6456 Refs: #6773 Refs: #7743 PR-URL: #6816 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label May 17, 2016

jasnell added a commit to jasnell/node that referenced this pull request May 17, 2016

tty: set blocking by default, api for unblocking

4982550

Builds on nodejs#6816, adds API for explicitly setting non-blocking mode without relying on `_`-prefixed internal property. Adds documentation.

Fishrock123 added the process Issues and PRs related to the process subsystem. label May 17, 2016

Fishrock123 force-pushed the stdio-TTY-blocking branch from c3bfcb0 to 5645984 Compare May 17, 2016 20:03

addaleax reviewed May 17, 2016
View reviewed changes

Fishrock123 force-pushed the stdio-TTY-blocking branch from 5645984 to 8a33b16 Compare May 17, 2016 20:06

Fishrock123 force-pushed the stdio-TTY-blocking branch from 8a33b16 to 2504cf5 Compare May 17, 2016 20:08

Trott reviewed Jul 15, 2016
View reviewed changes

tty: set the handle to blocking mode

09cc780

Refs: nodejs#1771 Refs: nodejs#6456 Refs: nodejs#6773 Refs: nodejs#7743 PR-URL: nodejs#6816

Fishrock123 force-pushed the stdio-TTY-blocking branch from 1f05b96 to 09cc780 Compare July 18, 2016 08:39

thefourtheye reviewed Jul 18, 2016
View reviewed changes

addaleax mentioned this pull request Aug 10, 2016

console.log hangs on long lines #8050

Closed

addaleax closed this Aug 10, 2016

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

MylesBorins added the dont-land-on-v4.x label Sep 30, 2016

sambthompson mentioned this pull request May 2, 2017

File descriptors not closed properly on OSX #12776

Closed

tty: set the handle to blocking mode #6816

tty: set the handle to blocking mode #6816

Conversation

Fishrock123 commented May 17, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

indutny commented May 17, 2016

indutny commented May 17, 2016

addaleax commented May 17, 2016

indutny commented May 17, 2016

mscdex commented May 17, 2016

Fishrock123 commented May 17, 2016

mscdex commented May 17, 2016

saghul commented May 17, 2016

jasnell commented May 17, 2016

jasnell commented May 17, 2016

Fishrock123 commented May 17, 2016 • edited Loading

Fishrock123 commented May 17, 2016

addaleax May 17, 2016

Choose a reason for hiding this comment

addaleax commented May 17, 2016

mscdex commented May 17, 2016

jasnell commented May 17, 2016

jasnell commented May 17, 2016

addaleax commented May 17, 2016 • edited Loading

Fishrock123 commented May 17, 2016

Fishrock123 commented May 17, 2016

jasnell commented May 17, 2016

Fishrock123 commented May 17, 2016

mscdex commented May 17, 2016 • edited Loading

addaleax commented May 17, 2016

Trott Jul 15, 2016

Choose a reason for hiding this comment

Trott commented Jul 15, 2016

MylesBorins commented Jul 15, 2016

saghul commented Jul 16, 2016

kzc commented Jul 16, 2016 • edited Loading

saghul commented Jul 17, 2016

Fishrock123 commented Jul 18, 2016

Fishrock123 commented Jul 18, 2016

Fishrock123 commented Jul 18, 2016

thefourtheye Jul 18, 2016

Choose a reason for hiding this comment

Fishrock123 Jul 18, 2016

Choose a reason for hiding this comment

thefourtheye Jul 18, 2016

Choose a reason for hiding this comment

jasnell commented Jul 20, 2016

Fishrock123 commented Jul 21, 2016 • edited Loading

addaleax commented Aug 8, 2016

Fishrock123 commented Aug 8, 2016

jasnell commented Aug 8, 2016

jasnell commented Aug 8, 2016

addaleax commented Aug 10, 2016 • edited Loading

addaleax commented Aug 10, 2016

Fishrock123 commented May 17, 2016 •

edited

Loading

Fishrock123 commented May 17, 2016 •

edited

Loading

addaleax commented May 17, 2016 •

edited

Loading

mscdex commented May 17, 2016 •

edited

Loading

kzc commented Jul 16, 2016 •

edited

Loading

Fishrock123 commented Jul 21, 2016 •

edited

Loading

addaleax commented Aug 10, 2016 •

edited

Loading