handle_wrap: IsRefed -> Unrefed, no isAlive check #6204

Fishrock123 · 2016-04-14T23:00:37Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

handle_wrap

Description of change

This fixes my perceived usability issues with #5834, which 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, I have renamed the method accordingly. If people need to check a handle's aliveness, that is a separate API we should consider exposing.

cc @trevnorris, @jasnell, @cjihrig

jasnell · 2016-04-15T07:42:37Z

LGTM if @trevnorris and CI are happy

bnoordhuis · 2016-04-15T07:45:51Z

Awkward names, both the old and the new one. What about hasRef?

Fishrock123 · 2016-04-15T14:38:56Z

@bnoordhuis Yeah, but hasRef (& isRefed) implies that it should return false if it is no longer alive.

Maybe UnrefBit, or something?

trevnorris · 2016-04-15T18:14:32Z

src/handle_wrap.cc

  HandleWrap* wrap = Unwrap<HandleWrap>(args.Holder());

-  bool refed = IsAlive(wrap) && (wrap->flags_ & kUnref) == 0;


There's no message body explaining why you removed the IsAlive() check, so i'm not sure of your rational. Seeing checks removed without explanation make me quiver.

Fishrock123 · 2016-04-15T18:48:50Z

@trevnorris I put it into the post comment:

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, I have renamed the method accordingly. If people need to check a handle's aliveness, that is a separate API we should consider exposing.

trevnorris · 2016-04-15T19:54:24Z

@Fishrock123 would you mind putting that into the commit message body? i'm lazy and don't want to have to go look at the PR to find out why a change was made.

Fishrock123 · 2016-04-15T19:56:36Z

Will do.

Helps in implementation of nodejs#6204, where some options passed to `createSecurePair()` are ignored before this patch. These options are very helpful if someone wants to pass `options.servername` or `options.SNICallback` to securepair. PR-URL: nodejs#2441 Reviewed-By: Fedor Indutny <fedor@indutny.com>

jasnell · 2016-04-25T15:41:36Z

ping @Fishrock123 ... if you still want this in v6 it needs to be wrapped up by tomorrow morning.

trevnorris · 2016-04-25T21:48:09Z

LGTM, IF the additional information requested above is added to the git message body.

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 · 2016-04-25T22:21:00Z

Updated with description and metadata. CI before landing: https://ci.nodejs.org/job/node-test-pull-request/2392/

Fishrock123 · 2016-04-25T22:41:15Z

plinux failure looks like a flake: test-debug-port-from-cmdline.js timeout.

Fishrock123 · 2016-04-25T23:25:10Z

Man I'm just having terrible luck with this one. This patch attempting to use this feature crashes test-handle-wrap-close-abort.js..

'./node test/parallel/test-handl…' terminated by signal SIGSEGV (Address boundary error)

diff --git a/lib/timers.js b/lib/timers.js
index dc2506e..9fb247c 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.unrefed() === true) {
     delete unrefedLists[msecs];
   } else {
     delete refedLists[msecs];

This reverts commit 9bb5a5e. Refs: nodejs#6382 Refs: nodejs#6204 Refs: nodejs#5834

This reverts commit 9bb5a5e. Refs: nodejs#6395 Refs: nodejs#6204 Refs: nodejs#6401 Refs: nodejs#6382 Fixes: nodejs#6381 Conflicts: src/handle_wrap.cc test/parallel/test-handle-wrap-isrefed-tty.js test/parallel/test-handle-wrap-isrefed.js

This reverts commit 9bb5a5e. This API is not suitable because it depended on being able to potentially access the handle's flag after the handle was already cleaned up. Since this is not actually possible (obviously, oops) this newer API no longer makes much sense, and the older API is more suitable. API comparison: IsRefed -> Has a strong reference AND is alive. (Deterministic) Unrefed -> Has a weak reference OR is dead. (Less deterministic) Refs: nodejs#6395 Refs: nodejs#6204 Refs: nodejs#6401 Refs: nodejs#6382 Fixes: nodejs#6381 PR-URL: nodejs#6546 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Conflicts: src/handle_wrap.cc test/parallel/test-handle-wrap-isrefed-tty.js test/parallel/test-handle-wrap-isrefed.js

Rename slightly to HasRef() at bnoordhuis’ request. Better reflects what we actually do for this check. Refs: nodejs#6395 Refs: nodejs#6204 Refs: nodejs#6401 Refs: nodejs#6382 Refs: nodejs#6381 PR-URL: nodejs#6546 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

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>

This reverts commit 9bb5a5e. This API is not suitable because it depended on being able to potentially access the handle's flag after the handle was already cleaned up. Since this is not actually possible (obviously, oops) this newer API no longer makes much sense, and the older API is more suitable. API comparison: IsRefed -> Has a strong reference AND is alive. (Deterministic) Unrefed -> Has a weak reference OR is dead. (Less deterministic) Refs: #6395 Refs: #6204 Refs: #6401 Refs: #6382 Fixes: #6381 PR-URL: #6546 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Conflicts: src/handle_wrap.cc test/parallel/test-handle-wrap-isrefed-tty.js test/parallel/test-handle-wrap-isrefed.js

Rename slightly to HasRef() at bnoordhuis’ request. Better reflects what we actually do for this check. Refs: #6395 Refs: #6204 Refs: #6401 Refs: #6382 Refs: #6381 PR-URL: #6546 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

MylesBorins · 2016-06-01T23:37:15Z

@Fishrock123 is there a reason you removed the dont land label for v4.x? This is causing false positives when auditing missing commits from lts

Fishrock123 · 2016-06-02T16:25:24Z

probably because of nodejs/Release#108

MylesBorins · 2016-06-02T16:42:52Z

ahhh my mistake... I got mixed up between this and the revert issues. Moving to lts-watch. Please feel free to do with the tags what you think is most appropriate

MylesBorins · 2016-11-18T20:24:15Z

@Fishrock123 this can be moved to don't land correct?

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 Apr 14, 2016

Fishrock123 added this to the 6.0.0 milestone Apr 14, 2016

trevnorris reviewed Apr 15, 2016
View reviewed changes

cjihrig mentioned this pull request Apr 18, 2016

dgram: tracking unrefdness of dgram sockets #5828

Closed

Fishrock123 force-pushed the better-handle-refed-check branch from f97bd23 to 4eb91b0 Compare April 25, 2016 22:16

Fishrock123 force-pushed the better-handle-refed-check branch from 4eb91b0 to 9bb5a5e Compare April 25, 2016 22:20

Fishrock123 merged commit 9bb5a5e into nodejs:master Apr 25, 2016

This was referenced Apr 25, 2016

timers: use HandleWrap::Unrefed #6381

Closed

Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check" #6382

Closed

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Apr 26, 2016

Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check"

635fb96

This reverts commit 9bb5a5e. Refs: nodejs#6382 Refs: nodejs#6204 Refs: nodejs#5834

Fishrock123 mentioned this pull request Apr 26, 2016

Revert handle wrap refed check #6401

Closed

Fishrock123 added dont-land-on-v5.x labels Apr 26, 2016

Fishrock123 mentioned this pull request May 3, 2016

Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check" #6546

Merged

3 tasks

Fishrock123 removed dont-land-on-v4.x labels May 17, 2016

Fishrock123 mentioned this pull request May 17, 2016

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

Merged

2 tasks

evanlucas mentioned this pull request May 17, 2016

2016-05-17, Version 6.2.0 (Stable) #6810

Merged

Fishrock123 mentioned this pull request May 19, 2016

Discuss backporting HandleWrap#hasRef() in a Minor nodejs/Release#108

Closed

MylesBorins added the dont-land-on-v4.x label Jun 1, 2016

MylesBorins added lts-watch-v4.x and removed dont-land-on-v4.x labels Jun 2, 2016

Fishrock123 added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle_wrap: IsRefed -> Unrefed, no isAlive check #6204

handle_wrap: IsRefed -> Unrefed, no isAlive check #6204

Fishrock123 commented Apr 14, 2016

jasnell commented Apr 15, 2016

bnoordhuis commented Apr 15, 2016 •

edited

Loading

Fishrock123 commented Apr 15, 2016 •

edited

Loading

trevnorris Apr 15, 2016

Fishrock123 commented Apr 15, 2016 •

edited

Loading

trevnorris commented Apr 15, 2016

Fishrock123 commented Apr 15, 2016

jasnell commented Apr 25, 2016

trevnorris commented Apr 25, 2016

Fishrock123 commented Apr 25, 2016

Fishrock123 commented Apr 25, 2016

Fishrock123 commented Apr 25, 2016 •

edited

Loading

MylesBorins commented Jun 1, 2016

Fishrock123 commented Jun 2, 2016

MylesBorins commented Jun 2, 2016

MylesBorins commented Nov 18, 2016

		HandleWrap* wrap = Unwrap<HandleWrap>(args.Holder());

		bool refed = IsAlive(wrap) && (wrap->flags_ & kUnref) == 0;

handle_wrap: IsRefed -> Unrefed, no isAlive check #6204

handle_wrap: IsRefed -> Unrefed, no isAlive check #6204

Conversation

Fishrock123 commented Apr 14, 2016

Checklist

Affected core subsystem(s)

Description of change

jasnell commented Apr 15, 2016

bnoordhuis commented Apr 15, 2016 • edited Loading

Fishrock123 commented Apr 15, 2016 • edited Loading

trevnorris Apr 15, 2016

Choose a reason for hiding this comment

Fishrock123 commented Apr 15, 2016 • edited Loading

trevnorris commented Apr 15, 2016

Fishrock123 commented Apr 15, 2016

jasnell commented Apr 25, 2016

trevnorris commented Apr 25, 2016

Fishrock123 commented Apr 25, 2016

Fishrock123 commented Apr 25, 2016

Fishrock123 commented Apr 25, 2016 • edited Loading

MylesBorins commented Jun 1, 2016

Fishrock123 commented Jun 2, 2016

MylesBorins commented Jun 2, 2016

MylesBorins commented Nov 18, 2016

bnoordhuis commented Apr 15, 2016 •

edited

Loading

Fishrock123 commented Apr 15, 2016 •

edited

Loading

Fishrock123 commented Apr 15, 2016 •

edited

Loading

Fishrock123 commented Apr 25, 2016 •

edited

Loading