Skip to content
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: make _read throw ERR_TTY_WRITABLE_NOT_READABLE #21654

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Jul 4, 2018

This change avoid an 'read ENOTCONN' error introduced by libuv 1.20.0
when trying to read from a TTY WriteStream. Instead, we are throwing
a more actionable ERR_TTY_WRITABLE_NOT_READABLE.

This changes fixes a regression introduced by libuv 1.20.0.
Calling .resume() on stdout and stderr started to error because of that
change.

See: ae2b5bc
Fixes: #21203

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the tty Issues and PRs related to the tty subsystem. label Jul 4, 2018
@mcollina
Copy link
Member Author

mcollina commented Jul 4, 2018

@addaleax
Copy link
Member

addaleax commented Jul 4, 2018

It seems like a regression to me that WriteStream is no longer usable as a readable stream since libuv 1.20.0 – this worked before. This particular PR just silences the error, it doesn’t fix behaviour.

It also seems like a bug that we have separate classes for tty.ReadStream and tty.WriteStream in general, TTYs should always be both readable and writable.

@mcollina
Copy link
Member Author

mcollina commented Jul 4, 2018

It seems like a regression to me that WriteStream is no longer usable as a readable stream since libuv 1.20.0 – this worked before. This particular PR just silences the error, it doesn’t fix behaviour.

Oh, I didn't know that we could read from stdout. I find this code really confusing:

process.stdout.setEncoding('utf8')
process.stdout.on('data', process._rawDebug)

The latest libuv docs is very explicit that we can either have a readable or writable TTY handle, but not both (see http://docs.libuv.org/en/v1.x/tty.html#c.uv_tty_init). I think it worked in the past by accident and not by design.

An alternative is to emit a better-to-read error in this case. I planned to do that anyway as a semver-major change after this lands, but I can do that now and backport to node 10 anyway - but it would need TSC approval.

I'm open to suggestions.

@addaleax
Copy link
Member

addaleax commented Jul 4, 2018

The latest libuv docs is very explicit that we can either have a readable or writable TTY handle, but not both (see http://docs.libuv.org/en/v1.x/tty.html#c.uv_tty_init). I think it worked in the past by accident and not by design.

I think that’s a libuv bug then, both in the code and the documentation? Otherwise it’s a purely artificial restriction…

An alternative is to emit a better-to-read error in this case. I planned to do that anyway as a semver-major change after this lands, but I can do that now and backport to node 10 anyway - but it would need TSC approval.

If we are committed to making things not work, then this is the best route to go imo

@mcollina
Copy link
Member Author

mcollina commented Jul 4, 2018

I think that’s a libuv bug then, both in the code and the documentation? Otherwise it’s a purely artificial restriction…

@nodejs/libuv what do you think?

@cjihrig
Copy link
Contributor

cjihrig commented Jul 4, 2018

Copying this from #21203 (comment) just for context:

The specific commit in libuv responsible for this is libuv/libuv@8f9ba2a. Specifically, the return -ENOTCONN; when uv_read_start() is called on a stream that is not readable.

This breakage was not intended, but this is arguably a bug in Node (or the application code) as the socket's readable property is false.

I think it makes sense that the stream's flags should be respected. That is what is causing the issue related to TTYs.

It also seems like a bug that we have separate classes for tty.ReadStream and tty.WriteStream in general, TTYs should always be both readable and writable.

That makes sense to me. A lot of the TTY code in both Node and libuv hasn't been touched in a long time. Unless there is some historical context I'm unaware of, maybe we should revisit it in both projects. I think it would at least require updates in uv_tty_init() to support flags instead of a readable boolean.

@santigimeno
Copy link
Member

It seems that before this very old commit it was both writable and readable. /cc @bnoordhuis as he may know why it was changed

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 4, 2018

It also seems like a bug that we have separate classes for tty.ReadStream and tty.WriteStream in general, TTYs should always be both readable and writable.

IIRC this is not the case on Windows?

@AyushG3112
Copy link
Contributor

A question: will this make the main process unable to listen on the stdout and stderr of a child process?

@jasnell
Copy link
Member

jasnell commented Jul 12, 2018

Is this ready to land or no?

@addaleax
Copy link
Member

I’d prefer not to land something that silences the error coming from what is imo a real regression.

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Jul 12, 2018
This change avoid an 'read ENOTCONN' error introduced by libuv 1.20.0
when trying to read from a TTY WriteStream. Instead, we are throwing
a more actionable ERR_TTY_WRITABLE_NOT_READABLE.

Fixes: nodejs#21203
@mcollina mcollina added semver-major PRs that contain breaking changes and should be released in the next major version. and removed wip Issues and PRs that are still a work in progress. labels Jul 20, 2018
@mcollina mcollina changed the title tty: make _read a noop as WriteStream is not readable tty: make _read throw ERR_TTY_WRITABLE_NOT_READABLE Jul 20, 2018
@mcollina
Copy link
Member Author

@mcollina
Copy link
Member Author

@addaleax please have a look. I've introduced the new error code, and tagged the PR as semver-major

@jasnell @Leko @cjihrig @BridgeAR you should review this again.

@nodejs/tsc we might want to backport this to Node 10.x even if is semver-major, mainly because the error code that we currently throw is not helpful.

@mcollina
Copy link
Member Author

@nodejs/tsc this needs another signoff before landing.

@Fishrock123
Copy link
Contributor

My 2c:

If the breaking libuv landed in a release already, we should release this as a patch before / if we cannot revert / soft-revert the libuv change.

Anything custom would be better than the raw error IMO, and since it already broke, this just seems like doing due diligence to follow up.

@gireeshpunathil
Copy link
Member

Anything custom would be better than the raw error IMO

@Fishrock123 - If libuv establishes that they are not reverting the changes, this is the best thing to do; but it makes sense to wait for their final assertion?

@Trott
Copy link
Member

Trott commented Aug 13, 2018

@mcollina Can this now be removed from the TSC agenda or is there something the TSC needs to do here?

@mcollina
Copy link
Member Author

@Trott I won't be able to be at the meeting. However very little progress is happening on the libuv side and users are still being presented with a very hard to debug error. I think we should land this asap, and ship it in Node 10 before LTS. If we could revert this, I'd be enthusiastic. Note that we might want to backport this down to Node 8 if we ever want to update libuv.
Considering that this is technically semver-major, what does @nodejs/lts think?

I'm ok to wait another week and call a vote on the follow-up meeting.

@jasnell
Copy link
Member

jasnell commented Aug 14, 2018

I agree with @mcollina

@mcollina
Copy link
Member Author

@nodejs/lts @nodejs/tsc @addaleax I think we should land this, as no progress has been made on the libuv side.

@addaleax
Copy link
Member

@mcollina Okay, feel free to do so, I won’t block this. I still think it’s a bad direction to go in, though.

@mcollina
Copy link
Member Author

@addaleax I agree. However the current error is worse than this, and it does not look like it is going to be solved anytime soon.

@mcollina
Copy link
Member Author

@mcollina
Copy link
Member Author

@mcollina
Copy link
Member Author

CI passed but didn't report correctly. Landing.

@mcollina
Copy link
Member Author

Landed in 91eec00

@mcollina mcollina closed this Aug 22, 2018
@mcollina mcollina deleted the fix-21203 branch August 22, 2018 14:42
mcollina added a commit that referenced this pull request Aug 22, 2018
This change avoid an 'read ENOTCONN' error introduced by libuv 1.20.0
when trying to read from a TTY WriteStream. Instead, we are throwing
a more actionable ERR_TTY_WRITABLE_NOT_READABLE.

Fixes: #21203

PR-URL: #21654
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 17, 2018
addaleax added a commit to addaleax/node that referenced this pull request Sep 24, 2018
addaleax added a commit that referenced this pull request Oct 4, 2018
This reverts commit 91eec00.

Refs: #21654
Refs: #21203

PR-URL: #23053
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Oct 5, 2018
This reverts commit 91eec00.

Refs: #21654
Refs: #21203

PR-URL: #23053
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
This reverts commit 91eec00.

Refs: #21654
Refs: #21203

PR-URL: #23053
Reviewed-By: James M Snell <jasnell@gmail.com>
@shrikrishnaholla
Copy link

Just FYI for the TSC and rest of the node maintainers on impact with a breaking change like this without sufficiently clear communication (please advise if there's a better forum to post this in):

The referenced libuv change was backported to node-8 and landed with Nov 28th security release. And that broke our production (even as builds succeeded in CIs). It has taken us longer to drill down on this as the root cause because we initially assumed it was docker-node's fault.

I don't know what could have been done better. It seems like there should have been something that should have gone differently, whether it was a clearer error message, or a patch in libuv, or a regression notice... Regression in node was the last thing we expected, since we hadn't switched major versions and expectation with semver is to not have breaking changes in patch/minor-upgrades.

@mcollina
Copy link
Member Author

@shrikrishnaholla I'm 100% sure that libuv was not updated on the 28th Nov release: https://nodejs.org/en/blog/release/v8.14.0/. That might have happened in another LTS release, but not that one. I think it has been https://nodejs.org/en/blog/release/v8.13.0/.

I don't think this specific PR was backported to any line, as we do not backport expected semver-major changes. However the libuv version has been backported without the fix for this unexpected change, which lives in #23053.

Can you open a new issue? We should likely backport part of #23053 to Node 8.

@mcollina
Copy link
Member Author

mcollina commented Dec 24, 2018

cc @addaleax @MylesBorins @nodejs/release @nodejs/lts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting callback for on 'data' event of stderr causes error on v10.0.0