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

tty: remove NODE_TTY_UNSAFE_ASYNC #12057

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@addaleax
Member

addaleax commented Mar 27, 2017

Rebased version of #10393 which has more or less stalled, plus addressing my own comment there.

/cc @Fishrock123 @nodejs/ctc

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

@addaleax addaleax referenced this pull request Mar 27, 2017

Closed

tty: remove NODE_TTY_UNSAFE_ASYNC #10393

3 of 3 tasks complete

@addaleax addaleax added this to the 8.0.0 milestone Mar 27, 2017

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Mar 27, 2017

Member

I'll update the original today if desired

Member

Fishrock123 commented Mar 27, 2017

I'll update the original today if desired

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 27, 2017

Member

@Fishrock123 Do whatever makes the most sense to you, I just did this because I’d like to see it land in time for 8.0.0

Member

addaleax commented Mar 27, 2017

@Fishrock123 Do whatever makes the most sense to you, I just did this because I’d like to see it land in time for 8.0.0

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Mar 29, 2017

Member

ok so maybe I don't have enough time / interest... carry on I guess

Member

Fishrock123 commented Mar 29, 2017

ok so maybe I don't have enough time / interest... carry on I guess

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Mar 29, 2017

Member

I don't think it makes any sense to block only on fd 0-2 though...

Plus that changes the behavior in yet another way 😬

Member

Fishrock123 commented Mar 29, 2017

I don't think it makes any sense to block only on fd 0-2 though...

Plus that changes the behavior in yet another way 😬

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Mar 29, 2017

Member

If anyone doesn't want to block on their own FD, they can just call the method?

Member

Fishrock123 commented Mar 29, 2017

If anyone doesn't want to block on their own FD, they can just call the method?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 29, 2017

Member

I don't think it makes any sense to block only on fd 0-2 though...

By that logic all Node.js streams should be blocking by default… these problems are about stdio, and that will cover the vast majority of all TTY streams.

Also, there’s similar code in lib/net.js for new net.Socket() already.

If anyone doesn't want to block on their own FD, they can just call the method?

Then we should add something for that to the public API.

Member

addaleax commented Mar 29, 2017

I don't think it makes any sense to block only on fd 0-2 though...

By that logic all Node.js streams should be blocking by default… these problems are about stdio, and that will cover the vast majority of all TTY streams.

Also, there’s similar code in lib/net.js for new net.Socket() already.

If anyone doesn't want to block on their own FD, they can just call the method?

Then we should add something for that to the public API.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Mar 29, 2017

Member

By that logic all Node.js streams should be blocking by default… these problems are about stdio, and that will cover the vast majority of all TTY streams.

if you don't block to TTYs the same silly (esp interleaving) problems manifest. Was there any issue with it behaving as it has been? I am -1 on changing that to be async by default in some cases.

Member

Fishrock123 commented Mar 29, 2017

By that logic all Node.js streams should be blocking by default… these problems are about stdio, and that will cover the vast majority of all TTY streams.

if you don't block to TTYs the same silly (esp interleaving) problems manifest. Was there any issue with it behaving as it has been? I am -1 on changing that to be async by default in some cases.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 29, 2017

Member

if you don't block to TTYs the same silly (esp interleaving) problems manifest.

Again, that’s true for any stream if you open it multiple times? Also, what’s special about stdio is that the API (and console.* in particular) encourages you to act like the writes are synchronous…

I’m dropping 889239b24f1237e69c5b4fdcc1f5df10586a94ec from this PR though, let’s save this discussion for something less time-constrained…

Member

addaleax commented Mar 29, 2017

if you don't block to TTYs the same silly (esp interleaving) problems manifest.

Again, that’s true for any stream if you open it multiple times? Also, what’s special about stdio is that the API (and console.* in particular) encourages you to act like the writes are synchronous…

I’m dropping 889239b24f1237e69c5b4fdcc1f5df10586a94ec from this PR though, let’s save this discussion for something less time-constrained…

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 31, 2017

Member

Landed in 1b63fa1

Member

addaleax commented Mar 31, 2017

Landed in 1b63fa1

@addaleax addaleax closed this Mar 31, 2017

@addaleax addaleax deleted the addaleax:tty-async branch Mar 31, 2017

addaleax added a commit that referenced this pull request Mar 31, 2017

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

PR-URL: #12057
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@Trott Trott removed the ctc-review label Apr 3, 2017

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment