tty: implement ReadStream._final to avoid ENOTCONN on .end() #22900

mcollina · 2018-09-17T11:23:50Z

Checklist

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

nodejs-github-bot · 2018-09-17T11:23:51Z

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/906/pipeline

addaleax · 2018-09-17T13:33:34Z

Is this also a change that came with the libuv TTY changes?

My gut feeling would be that ignoring ENOTCONN during shutdown of any socket would also be a valid fix? Is there ever any benefit from reporting that error back to the user, if it happens while shutting down?

mcollina · 2018-09-17T14:00:39Z

Is this also a change that came with the libuv TTY changes?

Yes.

My gut feeling would be that ignoring ENOTCONN during shutdown of any socket would also be a valid fix? Is there ever any benefit from reporting that error back to the user, if it happens while shutting down?

I think it's still valid for net.Socket. In this case it is happening because the writable side has never been opened, so it cannot really be shutdown (shutdown only works on writables).

addaleax · 2018-09-17T14:07:12Z

I know why it’s being emitted… I guess my main issue with this approach is that it adds more special handling for different types of streams, when we’ve been putting some effort into reducing exactly that. Conceptually, the TTY code should not contain any stream specifics, imo…

Alternatively, would it make sense to skip the shutdownSocket call in net.js if stream.writable was false when the stream was created?

mcollina · 2018-09-17T14:13:44Z

Alternatively, would it make sense to skip the shutdownSocket call in net.js if stream.writable was false when the stream was created?

I'll do that instead. It's more consistent.

Fixes: nodejs#22814.

mcollina · 2018-09-17T14:26:29Z

I'm not super confident about this change. Specifically because process.stdin.write() would likely emit an ENOTCONN as well. I think we should error in that case. I'm not sure if we should skip it in this one as well because it is totally harmless. However, calling .end() on what is essentially a readable should not be ok according to the new libuv logic.

addaleax

Thanks!

Specifically because process.stdin.write() would likely emit an ENOTCONN as well. I think we should error in that case.

I agree, silently swallowing input data is a different thing.

mcollina · 2018-09-17T14:37:03Z

I'll see what process.stdin.write() does and open a PR in case.

Fishrock123 · 2018-09-17T15:45:03Z

CI seems to fail? I'm not sure about the exact specifics but it sounds reasonable.

mcollina · 2018-09-17T15:56:02Z

Let's try CI: https://ci.nodejs.org/job/node-test-pull-request/17251/

mcollina · 2018-09-17T16:40:28Z

@addaleax handling it within net is not working. I think we should go back to the _final override.

addaleax · 2018-09-17T16:44:29Z

lib/net.js

@@ -359,7 +364,7 @@ Socket.prototype._final = function(cb) {
  debug('_final: not ended, call shutdown()');

  // otherwise, just shutdown, or destroy() if not possible
-  if (!this._handle || !this._handle.shutdown) {
+  if (!this[kShouldShutdown] || !this._handle || !this._handle.shutdown) {


I think the issue might be that this runs destroy() when it previously didn’t.

I can try it too (later), but maybe undoing this line change and adding if (!this[kShouldShutdown]) return cb(); below works?

It doesn't because the writable: false parameter that is passed in to the net.Socket constructor does control other behaviors internally, and it does not mean "this is half-open".
If you want to give it a shot, feel free to push here. Otherwise we revert to the _final solution.

cjihrig · 2018-09-18T13:19:16Z

lib/tty.js

@@ -66,6 +66,7 @@ function ReadStream(fd, options) {
 }
 inherits(ReadStream, net.Socket);

+


Unrelated change?

cjihrig · 2018-09-18T13:20:49Z

lib/net.js

@@ -245,6 +246,10 @@ function Socket(options) {
  options.writable = options.writable || false;
  const { allowHalfOpen } = options;

+  // Needed to avoid to call uv_shutdown during _final


"to call" -> "calling"

addaleax · 2018-09-19T16:25:31Z

I think libuv/libuv@4049879 may have fixed this as well, by making things work again on the libuv side?

mcollina · 2018-09-19T16:28:41Z

I think so. Have you tested it?

addaleax · 2018-09-19T16:30:47Z

@mcollina Just checked, and yes, it does fix ./node -e 'process.stdin.end();' when run on a TTY. 🎉

Would prefer to wait for libuv patch (should be soon enough?)

mcollina · 2018-09-19T16:37:15Z

Let's wait for that.

Fixes: nodejs#22814 Refs: nodejs#22900

addaleax · 2018-09-24T09:04:49Z

I’ve opened a PR with only the test changes here (and attribution to you), so I hope it’s okay to close this. You know what to do if I’m mistaken :)

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

net: do not call uv_shutdown if it did not start as writable

b30b568

Fixes: nodejs#22814.

mcollina force-pushed the fix-22814 branch from cfbf4c0 to b30b568 Compare September 17, 2018 14:24

addaleax previously approved these changes Sep 17, 2018

View reviewed changes

addaleax reviewed Sep 17, 2018

View reviewed changes

cjihrig approved these changes Sep 18, 2018

View reviewed changes

joyeecheung mentioned this pull request Sep 18, 2018

CI failures: 20180918 nodejs/reliability#16

Closed

jasnell approved these changes Sep 19, 2018

View reviewed changes

addaleax pushed a commit to addaleax/node that referenced this pull request Sep 24, 2018

test: add process.stdin.end() TTY regression test

c488603

Fixes: nodejs#22814 Refs: nodejs#22900

addaleax mentioned this pull request Sep 24, 2018

test: add process.stdin.end() TTY regression test #23051

Closed

3 tasks

addaleax closed this Sep 24, 2018

cristianberroeta mentioned this pull request Oct 7, 2018

fix ENOTCONN error in Node v10.1.0 workshopper/adventure#15

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tty: implement ReadStream._final to avoid ENOTCONN on .end() #22900

tty: implement ReadStream._final to avoid ENOTCONN on .end() #22900

mcollina commented Sep 17, 2018

nodejs-github-bot commented Sep 17, 2018

addaleax commented Sep 17, 2018

mcollina commented Sep 17, 2018

addaleax commented Sep 17, 2018

mcollina commented Sep 17, 2018

mcollina commented Sep 17, 2018

addaleax left a comment

mcollina commented Sep 17, 2018

Fishrock123 commented Sep 17, 2018

mcollina commented Sep 17, 2018

mcollina commented Sep 17, 2018

addaleax Sep 17, 2018

mcollina Sep 18, 2018

cjihrig Sep 18, 2018

cjihrig Sep 18, 2018

addaleax commented Sep 19, 2018

mcollina commented Sep 19, 2018

addaleax commented Sep 19, 2018

mcollina commented Sep 19, 2018

addaleax commented Sep 24, 2018

		@@ -66,6 +66,7 @@ function ReadStream(fd, options) {
		}
		inherits(ReadStream, net.Socket);

tty: implement ReadStream._final to avoid ENOTCONN on .end() #22900

tty: implement ReadStream._final to avoid ENOTCONN on .end() #22900

Conversation

mcollina commented Sep 17, 2018

Checklist

nodejs-github-bot commented Sep 17, 2018

addaleax commented Sep 17, 2018

mcollina commented Sep 17, 2018

addaleax commented Sep 17, 2018

mcollina commented Sep 17, 2018

mcollina commented Sep 17, 2018

addaleax left a comment

Choose a reason for hiding this comment

mcollina commented Sep 17, 2018

Fishrock123 commented Sep 17, 2018

mcollina commented Sep 17, 2018

mcollina commented Sep 17, 2018

addaleax Sep 17, 2018

Choose a reason for hiding this comment

mcollina Sep 18, 2018

Choose a reason for hiding this comment

cjihrig Sep 18, 2018

Choose a reason for hiding this comment

cjihrig Sep 18, 2018

Choose a reason for hiding this comment

addaleax commented Sep 19, 2018

mcollina commented Sep 19, 2018

addaleax commented Sep 19, 2018

mcollina commented Sep 19, 2018

addaleax commented Sep 24, 2018