tty: remove NODE_TTY_UNSAFE_ASYNC#10393

Fishrock123 · 2016-12-21T20:41:24Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tty

Description of change

Nothing but trouble can ever come from it.

Edit: I should mention that anyone can still use setBlocking(false) to get this behavior back if they really desire.

Nothing but trouble can ever come from it.

cjihrig

Personally, LGTM. Not sure how much it's used, if at all.

jasnell

This should go through a proper deprecation cycle

Fishrock123 · 2016-12-23T17:26:52Z

We should just deprecate it in v7, if anyone is actually going to yell about this I'd prefer to know now rather than in 6 months.

jasnell · 2016-12-24T06:57:44Z

@nodejs/ctc ... please weigh in on this!

Trott · 2016-12-24T07:16:53Z

This should go through a proper deprecation cycle

@jasnell Did your deprecation policy ever land?

If we don't have a deprecation policy, let's get one.

If we have a deprecation policy, we should follow it. And if we feel like there are situations where it should be ignored, then we should change it. It shouldn't be treated as an advisory document. It should be treated as a contract with Node.js users.

jasnell · 2016-12-24T07:27:37Z

Nope, not yet, but we need to get going on it.

ChALkeR · 2016-12-25T02:40:10Z

No traces of NODE_TTY_UNSAFE_ASYNC in js/coffee/ts code from npm, as of 2016-11-24.

Also, https://github.com/search?l=JavaScript&q=NODE_TTY_UNSAFE_ASYNC&type=Code doesn't show any real uses.

addaleax · 2016-12-25T18:26:37Z

There is virtually no difference in guaranteed behaviour here, so removing the env var barely qualifies as semver-major in my eyes.

jasnell · 2016-12-26T18:43:06Z

given @ChALkeR's search results, I'm +1 on getting the deprecation into v7 and removing in v8.

targos · 2016-12-26T18:48:11Z

I don't think usage of runtime env variables in JS files is a good metric. It's the kind of options that you will more likely find in deployment scripts or config files.

That said, I'm also +1 on removing in v8.

Fishrock123 · 2017-01-03T14:57:32Z

There is virtually no difference in guaranteed behaviour here, so removing the env var barely qualifies as semver-major in my eyes.

We could "remove" it in a minor, but it would have to be a no-op.

addaleax · 2017-01-03T18:40:55Z

We could "remove" it in a minor, but it would have to be a no-op.

I’m not sure I understand… is there a difference between making the flag a no-op and just removing any behaviour that was behind it?

Fishrock123 · 2017-01-05T15:32:37Z

I’m not sure I understand… is there a difference between making the flag a no-op and just removing any behaviour that was behind it?

@addaleax Yeah, if we don't parse the flag node will exit if it is passed. :/

targos · 2017-01-05T15:36:27Z

But that's not a flag. It's just an environment variable.

Fishrock123 · 2017-01-05T15:49:00Z

Oh, of course. Whoops >_<

evanlucas · 2017-01-31T11:55:40Z

I'm +1 for removing it, I just am unsure if it should be removed in v7.x as opposed to waiting for v8.x. To be on the safe side, I think we should wait until v8.x to remove, but possibly add a runtime deprecation to it in v7.x? We definitely need a way to communicate changes that we want to make like this to the ecosystem without disruption. This one is particularly difficult due to this being something that would probably be more used in services more so than in packages (imo).

mhdawson · 2017-01-31T20:45:49Z

I'm +1 on getting the deprecation into v7 and removing in v8.

bnoordhuis · 2017-02-01T13:19:26Z

+1 on removing.

There is virtually no difference in guaranteed behaviour here, so removing the env var barely qualifies as semver-major in my eyes.

Agreed.

Won't block this

jasnell · 2017-02-01T16:45:53Z

I'd still prefer for this to go through a proper deprecation cycle but I'll switch my objection to a -0.

misterdjules · 2017-02-07T19:09:48Z

I would also prefer this to go through a proper deprecation cycle, so I'm -1 on this change.

As @evanlucas said better than I could:

We definitely need a way to communicate changes that we want to make like this to the ecosystem without disruption. This one is particularly difficult due to this being something that would probably be more used in services more so than in packages (imo).

addaleax · 2017-02-07T19:34:01Z

Honestly, adding a runtime deprecation would be so much more disruptive than just dropping this feature completely…

And I’m not sure it’s clear to everyone, but just in case, I’ll expand on my comment above:
Node will attempt synchronous writes with or without the env var being present, and whether that succeeds or it has to fallback to writing asynchronously virtually always depends on external factors (e.g. performance of the terminal emulator); which means that one can’t really get reliable behaviour out of this feature anyways.

addaleax · 2017-02-07T19:34:34Z

lib/tty.js

 // even though it was originally intended to change in v1.0.2 (Libuv 1.2.1).
 // Ref: https://github.com/nodejs/node/pull/1771#issuecomment-119351671
-this._handle.setBlocking(process.env.NODE_TTY_UNSAFE_ASYNC!=='1');
+this._handle.setBlocking(true);


@Fishrock123 Why do we do this here, for all TTY streams? Shouldn’t we just do it for process.stderr/process.stdin?

bump @Fishrock123 … actually came here to comment exactly what I had written above and saw I already did that 😄

rvagg · 2017-02-08T03:31:21Z

I agree with @addaleax' last comment, so +1 from me, however I would prefer more if we just did a proper deprecation cycle and I don't see how that could be more disruptive, we just add a check for the env var being set to '1' and use the deprecation message output and proceed as normal. That would be easy enough in 7.x and we could remove in 8.x, so an expedited deprecation cycle really.

bnoordhuis

(Just reaffirming my +1 from further up so it shows in the sidebar. PR needs a rebase though.)

jasnell · 2017-03-22T22:10:57Z

@Fishrock123 ... how do you want to proceed on this one? Either way it needs a rebase

addaleax · 2017-03-27T04:35:17Z

picked this up @ #12057

jasnell · 2017-03-27T16:28:11Z

Will go ahead and close this in favor of the new PR

tty: remove NODE_TTY_UNSAFE_ASYNC
91d27e5
Nothing but trouble can ever come from it.

Fishrock123 added semver-major PRs that contain breaking changes and should be released in the next major version. tty Issues and PRs related to the tty subsystem. labels Dec 21, 2016

Fishrock123 added this to the 8.0.0 milestone Dec 21, 2016

cjihrig approved these changes Dec 21, 2016
View reviewed changes

addaleax approved these changes Dec 22, 2016
View reviewed changes

italoacasas approved these changes Dec 22, 2016
View reviewed changes

jasnell previously requested changes Dec 23, 2016
View reviewed changes

jasnell added the ctc-review label Dec 24, 2016

silverwind approved these changes Jan 18, 2017
View reviewed changes

Trott mentioned this pull request Jan 31, 2017
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-02-01 nodejs/CTC#65
Closed

Trott mentioned this pull request Feb 7, 2017
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-02-08 nodejs/CTC#68
Closed

addaleax reviewed Feb 7, 2017
View reviewed changes

This was referenced Feb 13, 2017
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-02-15 nodejs/CTC#70
Closed
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-02-22 nodejs/CTC#71
Closed

Trott mentioned this pull request Feb 27, 2017
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-01 nodejs/CTC#74
Closed

bnoordhuis approved these changes Feb 27, 2017
View reviewed changes

Trott mentioned this pull request Mar 6, 2017
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-08 nodejs/CTC#77
Closed

This was referenced Mar 13, 2017
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-15 nodejs/CTC#79
Closed
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-22 nodejs/CTC#82
Closed

Trott mentioned this pull request Mar 26, 2017
Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-29 nodejs/CTC#85
Closed

addaleax mentioned this pull request Mar 27, 2017
tty: remove NODE_TTY_UNSAFE_ASYNC #12057
Closed

jasnell closed this Mar 27, 2017

Trott removed the ctc-review label Apr 3, 2017

Uh oh!

tty: remove NODE_TTY_UNSAFE_ASYNC#10393

tty: remove NODE_TTY_UNSAFE_ASYNC #10393

Uh oh!

Conversation

Fishrock123 commented Dec 21, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Dec 23, 2016

Uh oh!

jasnell commented Dec 24, 2016

Uh oh!

Trott commented Dec 24, 2016

Uh oh!

jasnell commented Dec 24, 2016

Uh oh!

ChALkeR commented Dec 25, 2016

Uh oh!

addaleax commented Dec 25, 2016

Uh oh!

jasnell commented Dec 26, 2016

Uh oh!

targos commented Dec 26, 2016

Uh oh!

Fishrock123 commented Jan 3, 2017

Uh oh!

addaleax commented Jan 3, 2017

Uh oh!

Fishrock123 commented Jan 5, 2017

Uh oh!

targos commented Jan 5, 2017

Uh oh!

Fishrock123 commented Jan 5, 2017

Uh oh!

evanlucas commented Jan 31, 2017

Uh oh!

mhdawson commented Jan 31, 2017

Uh oh!

bnoordhuis commented Feb 1, 2017

Uh oh!

jasnell commented Feb 1, 2017

Uh oh!

misterdjules commented Feb 7, 2017

Uh oh!

addaleax commented Feb 7, 2017

Uh oh!

addaleaxFeb 7, 2017

Choose a reason for hiding this comment

Uh oh!

addaleaxMar 6, 2017

Choose a reason for hiding this comment

Uh oh!

rvagg commented Feb 8, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 22, 2017

Uh oh!

addaleax commented Mar 27, 2017

Uh oh!

jasnell commented Mar 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

Fishrock123 commented Dec 21, 2016•
edited
Loading