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

handle_wrap: expose {un}refed check to JS #5834

Merged
merged 1 commit into from
Apr 7, 2016

Conversation

Fishrock123
Copy link
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?

Affected core subsystem(s)

handle_wrap

Description of change

Give tools a consistent way to check if a handle is unrefed that works for all of our APIs that expose handles. E.g. would be very helpful to see which handles are umrefed in @AndreasMadsen's https://github.com/AndreasMadsen/dprof

(Some APIs, like timers, do not normally expose handles and are not addressed at that level here)

Refs: #5828
Refs: #5827

R= @trevnorris
cc @thlorenz

@Fishrock123 Fishrock123 added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 21, 2016
@@ -69,6 +81,7 @@ HandleWrap::HandleWrap(Environment* env,
HandleScope scope(env->isolate());
Wrap(object, this);
env->handle_wrap_queue()->PushBack(this);
env->SetMethod(object, "isRefed", HandleWrap::IsRefed);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk if it's better to have this be isUnrefed?

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably should be isUnrefed(), but isRefed() seems a lot simpler to think about.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think isRefed() sounds better ("is this holding the event loop open?").

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the way it's done. This has to be declared in every child class: https://github.com/nodejs/node/blob/v5.9.0/src/tcp_wrap.cc#L87

Otherwise you're setting an object property on an already constructed object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that kinda sucks, alright.

@Fishrock123
Copy link
Contributor Author

(does this needs tests..?)

bool refed = (wrap->flags_ & kUnref) == 0;
args.GetReturnValue().Set(Boolean::New(env->isolate(), refed));
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also: is it ok to return nothing of the handle is not alive?

Copy link
Contributor

Choose a reason for hiding this comment

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

returning nothing == returning undefined. but the JS signature returns bool. The question this is trying to answer is "Can this handle hold open the event loop?" So if !IsAlive() means should return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

So basically this can be boiled down to:

bool refed = IsAlive(wrap) && (wrap->flags_ & kUnref) == 0;
args.GetReturnValue().Set(Boolean::New(env->isolate(), refed);

@thlorenz
Copy link
Contributor

Yes, I think this needs tests. Ideally you'd pass tests similar to the ones in the PRs you referenced.
At that point we can also safely let this PR supersede those.

@mscdex
Copy link
Contributor

mscdex commented Mar 21, 2016

Yes, tests are needed.

@Fishrock123
Copy link
Contributor Author

Does anyone have advice on how I can make a more generic test for this rather than testing everywhere that we expose a handle?

@thlorenz
Copy link
Contributor

@Fishrock123 I guess you could test on C++ level (do we have C++ level tests?) and just create a handle and interact with it directly.
However I also believe that you should test this from a bit higher level for each case as well, at least for timers, sockets, pipes, etc.

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

Does it have to be a method? Is it possible to have it as a read-only property? .refed

A quick test case would be worthwhile also.

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 21, 2016
@Fishrock123
Copy link
Contributor Author

@jasnell I guess this makes it explicit that you cannot just set it, but I could possibly change it otherwise.

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@Fishrock123 ... that makes sense. The additional () isn't too difficult to type ;-). LGTM with test cases added.

@trevnorris
Copy link
Contributor

The implementation is incomplete. As noted in #5834 (comment) the method needs to be placed on the FunctionTemplate of the child class. This is tedious, but haven't found a better way to automate propagation of these JS methods to the C++ child classes. @bnoordhuis had an idea a while back, but can't remember what it was. Ben, you recall anything about this?

The test is simple enough:

const assert = require('assert');
const net = require('net');

const server = net.createServer(() => {}).listen(common.PORT);
server.unref();
assert.equal(false, server._handle.isRefed());

Remember this won't cover timer cases. To make the API parallel may need to implement a JS version that can be called on the JS timer handle instances.

@AndreasMadsen
Copy link
Member

@Fishrock123 thanks for doing this. It would also be useful for just checking what keeps a process alive at any given time.

@Fishrock123
Copy link
Contributor Author

Updated with a test, some things:

I realized that some _handles we expose directly extend AsyncWrap instead of HandleWrap, and this currently does not cover those. To cover those that would mean moving the c++ check into AsyncWrap I think, and I'm not sure if that is possible/acceptable.

I'm not sure if it is possible to test spawn a net.Socket that uses a Pipe handle without making a child process, so I've just gone ahead and directly made a Pipe.

TTY's cannot be re-refed for some reason. The method does not exist in c++, and does not work if it is added.

@Fishrock123
Copy link
Contributor Author

@Fishrock123
Copy link
Contributor Author

Hmm, getting this on Windows vs2015 and vcbt2015:

not ok 86 test-handle-wrap-isrefed.js
# events.js:155
# throw er; // Unhandled 'error' event
# ^
# 
# Error: read EISDIR
# at exports._errnoException (util.js:893:11)
# at TTY.onread (net.js:552:26)

Anyone know how to fix this test for that?

  const assert = makeAssert('isRefed() not working on tty_wrap');
  const ReadStream = require('tty').ReadStream;
  const tty = new ReadStream();
  assert(Object.getPrototypeOf(tty._handle).hasOwnProperty('isRefed'), true);
  assert(tty._handle.isRefed(), true);
  tty.unref();
  assert(tty._handle.isRefed(), false);

@cjihrig
Copy link
Contributor

cjihrig commented Mar 22, 2016

Do you need to pass a fd to the ReadStream() constructor?

Once the CI is happy, LGTM.

assert(timer._handle.isRefed(), false);
timer.ref();
assert(timer._handle.isRefed(), true);
timer.unref();
Copy link
Contributor

Choose a reason for hiding this comment

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

This depends on some non-obvious implementation details. If the timer is unref'd then the JS object will be moved to a TimerWrap that also isn't ref'd, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See

node/lib/timers.js

Lines 460 to 485 in d3a7534

Timeout.prototype.unref = function() {
if (this._handle) {
this._handle.unref();
} else if (typeof this._onTimeout === 'function') {
var now = TimerWrap.now();
if (!this._idleStart) this._idleStart = now;
var delay = this._idleStart + this._idleTimeout - now;
if (delay < 0) delay = 0;
// Prevent running cb again when unref() is called during the same cb
if (this._called && !this._repeat) {
exports.unenroll(this);
return;
}
var handle = reuse(this);
this._handle = handle || new TimerWrap();
this._handle.owner = this;
this._handle[kOnTimeout] = unrefdHandle;
this._handle.start(delay, 0);
this._handle.domain = this.domain;
this._handle.unref();
}
return this;
};

timer.unref() assigns a ._handle for that specific timer, and is unlikely to change in the short term.

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize that. What I failed to say is that a note of this should be placed in timer_wrap.cc where the method is added so future generations can immediately see why this non-obvious implementation detail actually works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where exactly do you mean? Isn't the same thing for the un(ref) methods on timer_wrap?

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is it may be non-obvious why setTimeout(()=>{}, 100)._handle === undefined but setTimeout(()=>{}, 100).unref()._handle links to a Timer instance. So using isRefed() is conditional to the state of timer. So not having a uniform way to access that data from the public object instance could be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but the regular timers implementation will also benefit from this. e.g.

node/lib/timers.js

Lines 210 to 214 in cf94929

if (list._unrefed === true) {
delete unrefedLists[msecs];
} else {
delete refedLists[msecs];
}

So I'm not sure what I would say in handle_wrap

@Fishrock123
Copy link
Contributor Author

Ci with fd 0 for TTY: https://ci.nodejs.org/job/node-test-pull-request/2030/

@Fishrock123
Copy link
Contributor Author

@cjihrig still the same thing on those windows builds/platforms..

@cjihrig
Copy link
Contributor

cjihrig commented Mar 22, 2016

Crap. The only real use of tty.ReadStream I found was in src/node.js.

@Fishrock123
Copy link
Contributor Author

maybe cc @bnoordhuis

@Fishrock123 Fishrock123 merged commit 7d8882b into nodejs:master Apr 7, 2016
@Fishrock123
Copy link
Contributor Author

@nodejs/lts think we can get this in the next minor LTS release?

@jasnell
Copy link
Member

jasnell commented Apr 7, 2016

Should be possible. It's just not yet clear when that next minor LTS release will be.

@Fishrock123
Copy link
Contributor Author

OK in hindsight this behavior regarding isAlive() is a huge mistake and this should not ship as is.

This needs to be accurate enough for timers to use too, and currently it isn't with either the following patches:

diff --git a/lib/timers.js b/lib/timers.js
index dc2506e..3a57d86 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -133,8 +133,6 @@ function insert(item, unrefed) {
     list = new TimersList(msecs, unrefed);
     L.init(list);
     list._timer._list = list;
-
-    if (unrefed === true) list._timer.unref();
     list._timer.start(msecs, 0);

     lists[msecs] = list;
@@ -149,7 +147,7 @@ function TimersList(msecs, unrefed) {
   this._idleNext = null; // Create the list with the linkedlist properties to
   this._idlePrev = null; // prevent any unnecessary hidden class changes.
   this._timer = new TimerWrap();
-  this._unrefed = unrefed;
+  if (unrefed === true) this._timer.unref();
   this.msecs = msecs;
 }

@@ -207,7 +205,7 @@ function listOnTimeout() {
   debug('%d list empty', msecs);
   assert(L.isEmpty(list));
   this.close();
-  if (list._unrefed === true) {
+  if (list._timer.isRefed() === false) {
     delete unrefedLists[msecs];
   } else {
     delete refedLists[msecs];
diff --git a/lib/timers.js b/lib/timers.js
index dc2506e..cdc0d1c 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -133,8 +133,6 @@ function insert(item, unrefed) {
     list = new TimersList(msecs, unrefed);
     L.init(list);
     list._timer._list = list;
-
-    if (unrefed === true) list._timer.unref();
     list._timer.start(msecs, 0);

     lists[msecs] = list;
@@ -149,7 +147,7 @@ function TimersList(msecs, unrefed) {
   this._idleNext = null; // Create the list with the linkedlist properties to
   this._idlePrev = null; // prevent any unnecessary hidden class changes.
   this._timer = new TimerWrap();
-  this._unrefed = unrefed;
+  if (unrefed === true) this._timer.unref();
   this.msecs = msecs;
 }

@@ -207,10 +205,10 @@ function listOnTimeout() {
   debug('%d list empty', msecs);
   assert(L.isEmpty(list));
   this.close();
-  if (list._unrefed === true) {
-    delete unrefedLists[msecs];
-  } else {
+  if (list._timer.isRefed() === true) {
     delete refedLists[msecs];
+  } else {
+    delete unrefedLists[msecs];
   }
 }

@Fishrock123
Copy link
Contributor Author

So... it does work if I close after the delete (duh), but the behavior is still pretty confusing..

Will probably remove the isAlive() check. (Which can be exposed separately if necessary)

@MylesBorins
Copy link
Contributor

@Fishrock123 you may want to chime into the v6 thread before the rc is cut if you don't think this should land

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 25, 2016
This fixes my perceived usability issues with 7d8882b. Which, at the
time of writing, has not landed in any release except v6 RCs. This
should not be considered a breaking change due to that.

It is useful if you have a handle, even if it has been closed, to be
able to inspect whether that handle was unrefed or not. As such, this
renames the method accordingly. If people need to check a handle's
aliveness, that is a separate API we should consider exposing.

Refs: nodejs#5834
PR-URL: nodejs#6204
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 26, 2016
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 26, 2016
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 26, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
This allows third-party tools to check whether or not a handle that
can be unreferenced is unreferenced at a particular time.
Notably, this should be helpful for inspection via AsyncWrap.

Also, this is useful even to node's internals, particularly timers.

Refs: #5828
Refs: #5827
PR-URL: #5834
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
evanlucas pushed a commit that referenced this pull request May 17, 2016
This fixes my perceived usability issues with 7d8882b. Which, at the
time of writing, has not landed in any release except v6 RCs. This
should not be considered a breaking change due to that.

It is useful if you have a handle, even if it has been closed, to be
able to inspect whether that handle was unrefed or not. As such, this
renames the method accordingly. If people need to check a handle's
aliveness, that is a separate API we should consider exposing.

Refs: #5834
PR-URL: #6204
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
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++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants