dgram: tracking unrefdness of dgram sockets

[thlorenz]

Affected core subsystem

dgram

Description of change

• initializing _unref to false when the socket is created
• updating that value each time either unref() or ref() is invoked
  on it
• this mirrors the implementation in lib/net.js
• adding test to check unrefdness tracking

This PR goes together with the same change applied to child_process ( #5827 )

R= @Fishrock123
R= @trevnorris

[mscdex]

I'm curious, why add this and the child_process changes if it's a private variable? Is it actually checked for somewhere else in the code base?

[thlorenz]

@mscdex This is useful for tools that show pending async actions in the process and need to show if they are keeping the process running or not.
As mentioned in the PR message in lib/net.js we are currently tracking this property so it makes sense to do so for other async activity handles as well.

[cjihrig]

LGTM. Again, would substitute assert.strictEqual() for assert.ok().

[thlorenz]

@cjihrig as in assert.strictEqual(sock._unref, true) for instance?
I looked at similar tests (with truthy assertions) and saw ok being used a lot.
However if we're trying to change that in general I can adapt the tests.

[cjihrig]

Yes, like that. I think it's a little more readable, with the state toggling back and forth. ok() also only checks truthiness, instead of strict Boolean values.

[Fishrock123]

Been thinking about this sort of thing for a while, I think it would be best for every handle to have this sort of property in c++, accessible to JS, if possible.

[thlorenz]

@Fishrock123 until then we should still get this in IMO, so we are aligned with the info that's exposed by lib/net.js. We can remove all _unref tracking once we are doing this for handles directly.

[Fishrock123]

@thlorenz Problem is, we might not be able to remove the _unref for like 6 months or even a year or more.

[thlorenz]

@Fishrock123 that is a valid concern, but the same is true for net.js.
This and the related PR just make things consistent which is better IMO. It's not like the concept of tracking _unref is introduced here.
Also it may be more than 6 months until we get the change to do this on the handle directly and until then the tools have nothing to go by if we don't expose this info via an _unref property.

[vkurchatkin]

-1. If it's useful, it should be public

[mscdex]

I think I am -1 on this as well. We should not be adding underscore-prefixed properties that we are encouraging end users to use as public/documented properties.

Either don't add it at all or remove the underscore and document it.

[thlorenz]

-1. If it's useful, it should be public

Agreed, just trying to match the implementation in net.js which has _unrefd as well.
However we could make this public ... in the meantime I'd like to see if @Fishrock123's #5834 solves this in a better way.

[jasnell]

@nodejs/ctc ... we've had one LGTM and a couple -1's on this. Can we get additional review?

[cjihrig]

Is this trumped by #5834 and #6204?

[jasnell]

Not sure, @Fishrock123 ?

[indutny]

-1, sorry

[jasnell]

Given the -1's and the discussion, then, I'm closing this. Can reopen if necessary.