(does this needs tests..?)

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.

Yes, tests are needed.

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

@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.

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 I guess this makes it explicit that you cannot just set it, but I could possibly change it otherwise.

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

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.

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

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.

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

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);

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

Once the CI is happy, LGTM.

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

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

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

maybe cc @bnoordhuis

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

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

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];
   }
 }

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)

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

For posterity, landing this requires this commit chain:

• 7d8882b (this PR's commit)
• f938ef7 src: fix -Wunused-variable compiler warning #6129
• 9bb5a5e handle_wrap: IsRefed -> Unrefed, no isAlive check #6204
• cad1a62 src: use libuv's refcounting directly  #6395
• a58d483 src: use libuv's refcounting directly  #6395
• 4282405 src: use libuv's refcounting directly  #6395
• 96a9439 Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check" #6546
• fe4837a Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check" #6546
• 2c480bd src,tools: rewrite check-imports and fix linter errors #6105