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.

Sign up for GitHub

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
Closed

tty: add ref() so process.stdin.ref() etc. work #7360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a0024e
tty: add ref() so process.stdin.ref() etc. work
insightfuls Jul 19, 2016
5dcc6a9
test: move tty-wrap isrefed test to pseudo-tty/
insightfuls Jul 19, 2016
18d9be2
test: test tty-wrap handle isrefed properly
insightfuls Jul 19, 2016
c81f710
test: improve failure messages in isrefed tests
insightfuls Feb 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/tty_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ void TTYWrap::Initialize(Local<Object> target,

env->SetProtoMethod(t, "close", HandleWrap::Close);
env->SetProtoMethod(t, "unref", HandleWrap::Unref);
env->SetProtoMethod(t, "ref", HandleWrap::Ref);
env->SetProtoMethod(t, "hasRef", HandleWrap::HasRef);

StreamWrap::AddMethods(env, t, StreamBase::kFlagNoShutdown);
Expand Down
5 changes: 5 additions & 0 deletions test/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ On how to run tests in this direcotry, see
<td>Yes</td>
<td>Various tests that are able to be run in parallel.</td>
</tr>
<tr>
<td>pseudo-tty</td>
<td>Yes</td>
<td>Tests that require stdin/stdout/stderr to be a TTY.</td>
</tr>
<tr>
<td>pummel</td>
<td>No</td>
Expand Down
33 changes: 0 additions & 33 deletions test/parallel/test-handle-wrap-isrefed-tty.js
View file
Open in desktop

This file was deleted.

129 changes: 79 additions & 50 deletions test/parallel/test-handle-wrap-isrefed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,99 +3,128 @@
const common = require('../common');
const strictEqual = require('assert').strictEqual;

function makeAssert(message) {
return function(actual, expected) {
strictEqual(actual, expected, message);
};
}


// child_process
{
const assert = makeAssert('hasRef() not working on process_wrap');
const spawn = require('child_process').spawn;
const cmd = common.isWindows ? 'rundll32' : 'ls';
const cp = spawn(cmd);
assert(Object.getPrototypeOf(cp._handle).hasOwnProperty('hasRef'), true);
assert(cp._handle.hasRef(), true);
strictEqual(Object.getPrototypeOf(cp._handle).hasOwnProperty('hasRef'),
true, 'process_wrap: hasRef() missing');
strictEqual(cp._handle.hasRef(),
true, 'process_wrap: not initially refed');
cp.unref();
assert(cp._handle.hasRef(), false);
strictEqual(cp._handle.hasRef(),
false, 'process_wrap: unref() ineffective');
cp.ref();
assert(cp._handle.hasRef(), true);
cp._handle.close(common.mustCall(() => assert(cp._handle.hasRef(), false)));
strictEqual(cp._handle.hasRef(),
true, 'process_wrap: ref() ineffective');
cp._handle.close(common.mustCall(() =>
strictEqual(cp._handle.hasRef(),
false, 'process_wrap: not unrefed on close')));
}


// dgram
// dgram ipv4
{
const assert = makeAssert('hasRef() not working on udp_wrap');
const dgram = require('dgram');

const sock4 = dgram.createSocket('udp4');
assert(Object.getPrototypeOf(sock4._handle).hasOwnProperty('hasRef'), true);
assert(sock4._handle.hasRef(), true);
strictEqual(Object.getPrototypeOf(sock4._handle).hasOwnProperty('hasRef'),
true, 'udp_wrap: ipv4: hasRef() missing');
strictEqual(sock4._handle.hasRef(),
true, 'udp_wrap: ipv4: not initially refed');
sock4.unref();
assert(sock4._handle.hasRef(), false);
strictEqual(sock4._handle.hasRef(),
false, 'udp_wrap: ipv4: unref() ineffective');
sock4.ref();
assert(sock4._handle.hasRef(), true);
sock4._handle.close(
common.mustCall(() => assert(sock4._handle.hasRef(), false)));
strictEqual(sock4._handle.hasRef(),
true, 'udp_wrap: ipv4: ref() ineffective');
sock4._handle.close(common.mustCall(() =>
strictEqual(sock4._handle.hasRef(),
false, 'udp_wrap: ipv4: not unrefed on close')));
}


// dgram ipv6
{
const dgram = require('dgram');
const sock6 = dgram.createSocket('udp6');
assert(Object.getPrototypeOf(sock6._handle).hasOwnProperty('hasRef'), true);
assert(sock6._handle.hasRef(), true);
strictEqual(Object.getPrototypeOf(sock6._handle).hasOwnProperty('hasRef'),
true, 'udp_wrap: ipv6: hasRef() missing');
strictEqual(sock6._handle.hasRef(),
true, 'udp_wrap: ipv6: not initially refed');
sock6.unref();
assert(sock6._handle.hasRef(), false);
strictEqual(sock6._handle.hasRef(),
false, 'udp_wrap: ipv6: unref() ineffective');
sock6.ref();
assert(sock6._handle.hasRef(), true);
sock6._handle.close(
common.mustCall(() => assert(sock6._handle.hasRef(), false)));
strictEqual(sock6._handle.hasRef(),
true, 'udp_wrap: ipv6: ref() ineffective');
sock6._handle.close(common.mustCall(() =>
strictEqual(sock6._handle.hasRef(),
false, 'udp_wrap: ipv6: not unrefed on close')));
}


// pipe
{
const assert = makeAssert('hasRef() not working on pipe_wrap');
const Pipe = process.binding('pipe_wrap').Pipe;
const handle = new Pipe();
assert(Object.getPrototypeOf(handle).hasOwnProperty('hasRef'), true);
assert(handle.hasRef(), true);
strictEqual(Object.getPrototypeOf(handle).hasOwnProperty('hasRef'),
true, 'pipe_wrap: hasRef() missing');
strictEqual(handle.hasRef(),
true, 'pipe_wrap: not initially refed');
handle.unref();
assert(handle.hasRef(), false);
strictEqual(handle.hasRef(),
false, 'pipe_wrap: unref() ineffective');
handle.ref();
assert(handle.hasRef(), true);
handle.close(common.mustCall(() => assert(handle.hasRef(), false)));
strictEqual(handle.hasRef(),
true, 'pipe_wrap: ref() ineffective');
handle.close(common.mustCall(() =>
strictEqual(handle.hasRef(),
false, 'pipe_wrap: not unrefed on close')));
}


// tcp
{
const assert = makeAssert('hasRef() not working on tcp_wrap');
const net = require('net');
const server = net.createServer(() => {}).listen(0);
assert(Object.getPrototypeOf(server._handle).hasOwnProperty('hasRef'), true);
assert(server._handle.hasRef(), true);
assert(server._unref, false);
strictEqual(Object.getPrototypeOf(server._handle).hasOwnProperty('hasRef'),
true, 'tcp_wrap: hasRef() missing');
strictEqual(server._handle.hasRef(),
true, 'tcp_wrap: not initially refed');
strictEqual(server._unref,
false, 'tcp_wrap: _unref initially incorrect');
server.unref();
assert(server._handle.hasRef(), false);
assert(server._unref, true);
strictEqual(server._handle.hasRef(),
false, 'tcp_wrap: unref() ineffective');
strictEqual(server._unref,
true, 'tcp_wrap: _unref not updated on unref()');
server.ref();
assert(server._handle.hasRef(), true);
assert(server._unref, false);
server._handle.close(
common.mustCall(() => assert(server._handle.hasRef(), false)));
strictEqual(server._handle.hasRef(),
true, 'tcp_wrap: ref() ineffective');
strictEqual(server._unref,
false, 'tcp_wrap: _unref not updated on ref()');
server._handle.close(common.mustCall(() =>
strictEqual(server._handle.hasRef(),
false, 'tcp_wrap: not unrefed on close')));
}


// timers
{
const assert = makeAssert('hasRef() not working on timer_wrap');
const timer = setTimeout(() => {}, 500);
timer.unref();
assert(Object.getPrototypeOf(timer._handle).hasOwnProperty('hasRef'), true);
assert(timer._handle.hasRef(), false);
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 :-)

timer.ref();
assert(timer._handle.hasRef(), true);
timer._handle.close(
common.mustCall(() => assert(timer._handle.hasRef(), false)));
strictEqual(timer._handle.hasRef(),
true, 'timer_wrap: ref() ineffective');
timer._handle.close(common.mustCall(() =>
strictEqual(timer._handle.hasRef(),
false, 'timer_wrap: not unrefed on close')));
}


// see also test/pseudo-tty/test-handle-wrap-isrefed-tty.js
27 changes: 27 additions & 0 deletions test/pseudo-tty/ref_keeps_node_running.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';
require('../common');

const { TTY, isTTY } = process.binding('tty_wrap');
const strictEqual = require('assert').strictEqual;

strictEqual(isTTY(0), true, 'fd 0 is not a TTY');

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

function isHandleActive(handle) {
return process._getActiveHandles().some((active) => active === handle);
}

strictEqual(isHandleActive(handle), true, 'TTY handle not initially active');

handle.unref();

strictEqual(isHandleActive(handle), false, 'TTY handle active after unref()');

handle.ref();

strictEqual(isHandleActive(handle), true, 'TTY handle inactive after ref()');

handle.unref();
Empty file added test/pseudo-tty/ref_keeps_node_running.out
View file
Open in desktop
Empty file.
23 changes: 23 additions & 0 deletions test/pseudo-tty/test-handle-wrap-isrefed-tty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

// see also test/parallel/test-handle-wrap-isrefed.js

const common = require('../common');
const strictEqual = require('assert').strictEqual;
const ReadStream = require('tty').ReadStream;
const tty = new ReadStream(0);
const isTTY = process.binding('tty_wrap').isTTY;
strictEqual(isTTY(0), true, 'tty_wrap: stdin is not a TTY');
strictEqual(Object.getPrototypeOf(tty._handle).hasOwnProperty('hasRef'),
true, 'tty_wrap: hasRef() missing');
strictEqual(tty._handle.hasRef(),
true, 'tty_wrap: not initially refed');
tty.unref();
strictEqual(tty._handle.hasRef(),
false, 'tty_wrap: unref() ineffective');
tty.ref();
strictEqual(tty._handle.hasRef(),
true, 'tty_wrap: ref() ineffective');
tty._handle.close(common.mustCall(() =>
strictEqual(tty._handle.hasRef(),
false, 'tty_wrap: not unrefed on close')));
Empty file added test/pseudo-tty/test-handle-wrap-isrefed-tty.out
View file
Open in desktop
Empty file.