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: add ref() so process.stdin.ref() etc. work #7360

Closed
wants to merge 4 commits into from

Conversation

insightfuls
Copy link
Contributor

@insightfuls insightfuls commented Jun 22, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

tty

Description of change

tty.ReadStream and tty.WriteStream are documented to be net.Socket subclasses, so should support unref() and ref(), but only unref() worked. I ran into this problem when following advice at http://stackoverflow.com/questions/26004519/why-doesnt-my-node-js-process-terminate-once-all-listeners-have-been-removed to get Node to exit when I stop listening to stdin (which seems to be delayed until a newline is received otherwise). If I changed my mind and wanted to start listening again prior to exiting, it wasn't possible as ref() wasn't implemented (net.Socket.ref threw when trying to call tty_wrap.ref which didn't exist). This fixes it up trivially by implementing tty_wrap.ref().

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 22, 2016
@mscdex mscdex added tty Issues and PRs related to the tty subsystem. process Issues and PRs related to the process subsystem. labels Jun 22, 2016
@Fishrock123
Copy link
Contributor

IIRC when I last tried this it didn't actually undo the weak reference..

@Fishrock123 Fishrock123 self-assigned this Jun 24, 2016
@Fishrock123 Fishrock123 removed the process Issues and PRs related to the process subsystem. label Jun 24, 2016
@Fishrock123
Copy link
Contributor

@insightfuls
Copy link
Contributor Author

insightfuls commented Jun 24, 2016

It works for me with this test script:

"use strict";
process.stdout.write("Keep Node open [y/N]?: ");
process.stdin.on("readable", function() {
    var data = process.stdin.read();
    if (data === null) {
        return;
    }
    if (data[0] === "Y".charCodeAt(0) || data[0] === "y".charCodeAt(0)) {
        process.stdin.ref();
    } else {
        process.stdin.unref();
        process.stdout.write("There will be a 5-second delay for you to change your mind.\n");
        setTimeout(function(){}, 5000);
    }
    process.stdout.write("Keep Node open [y/N]?: ");
});
process.on("exit", function(){ process.stdout.write("\n") });

AFAICT, I can't work something like this into the automated tests, but if there's a way to do so, I can try; just point me in the right direction.

Re the CI, is there something that requires my action? I realise it's not passing, but I'm not sure what all the failures are. I do know one test was failing before I made my changes, and I rebased before submitting the PR, somewhat blindly following the instructions in the Contributing document, as I'm somewhat a Git n00b. Should I rebase my branch onto a commit where all the tests are passing? Or is there something else I should do? There seem to also be linting problems showing up in Jenkins which I doubt have anything to do with this change, but I'm not sure how to get more detailed information to even find out. I'm happy to do whatever I can, but I'll need a little guidance to know what to do.

@Fishrock123
Copy link
Contributor

Ok, there is a way to make the test work and I did verify that it works correctly.

Put this is test/pseudo-tty/ -- you'll also need a file of the same name with the extension .out, it can just have a single newline in it. that is ok.

'use strict';
require('../common');

const TTY = process.binding('tty_wrap').TTY;

const handle = new TTY(0);
handle.readStart();

handle.unref();
handle.ref();
handle.unref();

@insightfuls
Copy link
Contributor Author

Thanks. That was the start I needed. Looking at the code you provided, though, it only truly tests unref(): ref() could be a no-op and the test would still pass (unless it did something like throw, as it used to, but testing it doesn't throw is covered by the test I already provided). I've reworked it to use an unref()'d timeout to check that ref() does indeed keep Node running, so works as expected. If it doesn't, the timeout won't keep Node open either so there will be no output. Of course this also tests unref() works, because otherwise Node would hang and not exit at all.

make -j4 test is working for me; though one test in the suite seems to fail non-deterministically (ouch), it is nothing to do with this change (something to do with watching the filesystem).

Is there anything more I need to do here?

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jul 11, 2016
Refs: nodejs#7360
PR-URL: nodejs#7613
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jul 11, 2016
test-tty-wrap hasn’t worked since StreamBase was introduced, I think.
test-tty-stdout-end also happens to works with PipeWrap-s.

Refs: nodejs#7360
PR-URL: nodejs#7613
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
var handle = new TTY(1);

handle.unref();
handle.ref(); // Should not throw.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this test - it will always be skipped and is therefore not useful. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(pseudo-tty/ tests are runs as party of normal test runs)

@Fishrock123
Copy link
Contributor

evanlucas pushed a commit that referenced this pull request Jul 15, 2016
Refs: #7360
PR-URL: #7613
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas pushed a commit that referenced this pull request Jul 15, 2016
test-tty-wrap hasn’t worked since StreamBase was introduced, I think.
test-tty-stdout-end also happens to works with PipeWrap-s.

Refs: #7360
PR-URL: #7613
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 18, 2016

@insightfuls Looks like lots of errors:

not ok 1162 pseudo-tty/ref_and_unref
# /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node[62514]: ../src/async-wrap-inl.h:109:v8::Local<v8::Value> node::AsyncWrap::MakeCallback(const v8::Local<v8::String>, int, v8::Local<v8::Value> *): Assertion `cb_v->IsFunction()' failed.
# 1: node::Abort() [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 2: node::RunMicrotasks(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 3: node::StreamBase::EmitData(long, v8::Local<v8::Object>, v8::Local<v8::Object>) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 4: node::StreamWrap::OnReadImpl(long, uv_buf_t const*, uv_handle_type, void*) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 5: node::StreamWrap::OnReadCommon(uv_stream_s*, long, uv_buf_t const*, uv_handle_type) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 6: uv__stream_eof [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 7: uv__stream_io [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 8: uv__io_poll [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 9: uv_run [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 10: node::Start(int, char**) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
# 11: start [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]

@cjihrig Any idea? I'm not really sure why MakeCallback would fail from this.

@insightfuls Maybe it would better to use Colin's suggestion for testing this.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 18, 2016

That's strange. Nothing really jumps out, and it passes locally on my mac.

@Fishrock123
Copy link
Contributor

Same, it also passes locally for me.

@addaleax
Copy link
Member

addaleax commented Jul 18, 2016

I’d assume the reason it fails on CI is that standard input ends immediately there, and so the TTY handle tries to call .onread() to communicate that.

I am able to reproduce the failure using echo -n | ./node test/pseudo-tty/ref_and_unref.js, and adding a dummy .onread() should fix it:

diff --git a/test/pseudo-tty/ref_and_unref.js b/test/pseudo-tty/ref_and_unref.js
index 0faad3c..2131546 100644
--- a/test/pseudo-tty/ref_and_unref.js
+++ b/test/pseudo-tty/ref_and_unref.js
@@ -4,6 +4,7 @@ require('../common');
 const TTY = process.binding('tty_wrap').TTY;

 const handle = new TTY(0);
+handle.onread = () => {};
 handle.readStart();

 handle.unref();

@insightfuls insightfuls force-pushed the tty-ref branch 2 times, most recently from f28da90 to f875576 Compare July 19, 2016 05:42
@insightfuls
Copy link
Contributor Author

Ugh. echo -n | make test also fails. Turns out the Python test harness only puts a pty on stdout and stderr, not stdin, meaning in CI it wouldn't be testing a TTY at all. I've fixed that up.

I've updated my test case. I've opted for the internal timer approach, as I feel that tests precisely what I want it to test. I don't believe it will be flakey because it's not truly about time, it's only about giving the event loop an opportunity to tick over. But if any of you folk disagree, just see out the debate to get some consensus, and I'll be happy to follow it!

Additionally, though, the active handle aspect is, I believe, tested by 'test-handle-wrap-isrefed-tty', which I found in 'parallel', and wouldn't have been working, because it spawned a subprocess with piped stdio, so was really testing pipe-wrap not tty-wrap. I've moved it to pseudo-tty and removed the spawning so it truly tests TTYs now. I've followed the pattern from #7613, doing the rename in a separate commit from fixing up the test.

Please see updated branch!

@cjihrig
Copy link
Contributor

cjihrig commented Jul 19, 2016

I'm still -1 on using a timer that isn't really needed. If a ref'ed handle doesn't keep the event loop open, then Node is completely broken. Thoughts from @nodejs/testing?

@Fishrock123
Copy link
Contributor

'test-handle-wrap-isrefed-tty',

Crap. I forgot about that one when I fixed the others recently...

Turns out the Python test harness only puts a pty on stdout and stderr, not stdin, meaning in CI it wouldn't be testing a TTY at all. I've fixed that up.

hmmm... I forgot about that. It should be possible to make it also get a fake stdin.

@Fishrock123
Copy link
Contributor

CI Again: https://ci.nodejs.org/job/node-test-pull-request/3335/

I like this, but I'm fine switching to @cjihrig's way.

@insightfuls
Copy link
Contributor Author

I've added a commit that adjusts all the refed tests to call strictEqual directly. See what you think.

strictEqual(Object.getPrototypeOf(timer._handle).hasOwnProperty('hasRef'),
true, 'timer_wrap: hasRef() missing');
strictEqual(timer._handle.hasRef(),
false, 'timer_wrap: unref() ineffective');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: There was no unref before this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes there was; it's just a line further up.

Apparently timers don't have _handle defined prior to unref() being called, so we cannot check hasRef() exists, nor assert an initially refed state for them. Instead we check the existence of hasRef() and the unrefed state after already calling unref().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool. Thanks for clarifying :-)

@thefourtheye
Copy link
Contributor

Why test/pseudo-tty/ref_keeps_node_running.out and test/pseudo-tty/test-handle-wrap-isrefed-tty.out are necessary?

@addaleax
Copy link
Member

@thefourtheye The test runner expects them, test/pseudo-tty is like test/message and asserts against the script output.

@thefourtheye
Copy link
Contributor

Oh okay. Thanks @addaleax :-)

@insightfuls
Copy link
Contributor Author

(Just fixed up the author of the commit.)

@addaleax
Copy link
Member

addaleax commented Feb 25, 2017

(Just fixed up the author of the commit.)

@insightfuls Are you sure you didn’t mean the committer of the commit? It might be splitting hairs but I’d like to be sure which email you actually wanted since you explicitly brought it up (…@users.noreply.github.com or dev@…?).

edit: this is the last thing I’d like to clear up before landing this ;)

@insightfuls
Copy link
Contributor Author

It doesn't really matter, but the github one is what I intended, and I figured it is probably more sensible if it's at least consistent for the related commits.

@addaleax
Copy link
Member

Landed in 3c92200 🎉 Thanks for the PR!

@addaleax addaleax closed this Feb 28, 2017
addaleax pushed a commit that referenced this pull request Feb 28, 2017
Also squashed from:
* test: move tty-wrap isrefed test to pseudo-tty/
* test: test tty-wrap handle isrefed properly
* test: improve failure messages in isrefed tests

PR-URL: #7360
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@insightfuls
Copy link
Contributor Author

Thanks a lot!

addaleax pushed a commit that referenced this pull request Mar 5, 2017
Also squashed from:
* test: move tty-wrap isrefed test to pseudo-tty/
* test: test tty-wrap handle isrefed properly
* test: improve failure messages in isrefed tests

PR-URL: #7360
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell.gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
evanlucas added a commit that referenced this pull request Mar 8, 2017
Notable changes:

* doc: add `Daijiro Wachi` to collaborators (Daijiro Wachi) #11676
* tty: add ref() so process.stdin.ref() etc. work (Ben Schmidt) #7360
* util: fix inspecting symbol key in string (Ali BARIN) #11672

PR-URL: #11745
evanlucas added a commit that referenced this pull request Mar 8, 2017
Notable changes:

* doc: add `Daijiro Wachi` to collaborators (Daijiro Wachi) #11676
* tty: add ref() so process.stdin.ref() etc. work (Ben Schmidt) #7360
* util: fix inspecting symbol key in string (Ali BARIN) #11672

PR-URL: #11745
@Fishrock123
Copy link
Contributor

In hindsight, this shouldn't have this been either:
a. in a minor?
b. not mentioned under "notable changes"?

imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 16, 2017
    Notable changes:

    * doc: add `Daijiro Wachi` to collaborators (Daijiro Wachi) nodejs/node#11676
    * tty: add ref() so process.stdin.ref() etc. work (Ben Schmidt) nodejs/node#7360
    * util: fix inspecting symbol key in string (Ali BARIN) nodejs/node#11672

    PR-URL: nodejs/node#11745

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
Notable changes:

* doc: add `Daijiro Wachi` to collaborators (Daijiro Wachi) nodejs#11676
* tty: add ref() so process.stdin.ref() etc. work (Ben Schmidt) nodejs#7360
* util: fix inspecting symbol key in string (Ali BARIN) nodejs#11672

PR-URL: nodejs#11745
@MylesBorins
Copy link
Contributor

Should this be backported?
@Fishrock123 you mention that this should might have been better as a minor... do you think it should land that was into LTS (if it lands)

@MylesBorins
Copy link
Contributor

ping

@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@MylesBorins
Copy link
Contributor

ping re: backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. confirmed-bug Issues with confirmed bugs. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.