timers: use HandleWrap::Unrefed

[Fishrock123]

Checklist

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

Affected core subsystem(s)

timers

Description of change

So, there a couple things going on here:

• I originally wanted to do this, I don't like relying on a JS property much.
• This breaks down if someone {un}refs the handle from C++, is that actually an issue?

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

[bnoordhuis]

LGTM but the commit log should say timers: use HandleWrap::HasRef(), no? Also, the CI had issues.

[Fishrock123]

I think the CI went down for a security update, new CI: https://ci.nodejs.org/job/node-test-pull-request/2604/

Edit: CI is green.

[Fishrock123]

also cc @misterdjules

[misterdjules]

The changes themselves look like they wouldn't introduce any major regression. What's the motivation for this change? What's the problem about relying on the _unrefed property (besides the fact that its name could be better, see below)?

Also, as you noted:

This breaks down if someone {un}refs the handle from C++, is that actually an issue?

While I don't know if using ref/unref on timers without using Timeout.prototype.unref is a significant use case, this problem seems to be the symptom of a broader issue with the current timers implementation: there are two different types of timers that are referred to with the same name.

These two different types are:

1. unrefed external timers, created with e.g setTimeout(...).unref()
2. unrefed internal timers, created with _unrefActive

The two lists of timers lists, unrefedLists and refedLists really are internalTimersLists and timersLists. Before this change, we could tell the difference between an internal timer instance and an external one by checking the _unrefed property on a timeout's list, and external unrefed timers where removed from the list anyway, so they wouldn't be processed by listOnTimeout.

However with this change, the lines become even less clear between an internal timer and an unrefed "external" or "user" timer, which is why we run into the problem you mentioned above.

It seems to me that we would gain a lot in implementation clarity by refactoring the timers implementation to make the difference between internal timers and external unrefed timers clearer by e.g adding the concept of an "internal" timer to the implementation. I believe it would fix the issue mentioned above.

I hope that helps.

[Fishrock123]

What's the problem about relying on the _unrefed property (besides the fact that its name could be better, see below)?

There isn't one, but it's an unideal hack.

While I don't know if using ref/unref on timers without using Timeout.prototype.unref is a significant use case, this problem seems to be the symptom of a broader issue with the current timers implementation: there are two different types of timers that are referred to with the same name.

I don't disagree but it isn't easy to fix and is out of scope. :(

Also consider un/refing one of the internal lists will effect many times, not just one.

Before this change, we could tell the difference between an internal timer instance and an external one by checking the _unrefed property on a timeout's list

You can just check the handle like this PR, what use is it to us?

However with this change, the lines become even less clear between an internal timer and an unrefed "external" or "user" timer, which is why we run into the problem you mentioned above.

How so?

It seems to me that we would gain a lot in implementation clarity by refactoring the timers implementation to make the difference between internal timers and external unrefed timers clearer by e.g adding the concept of an "internal" timer to the implementation. I believe it would fix the issue mentioned above.

I'm not sure about that.

I've considered exposing setTimeoutWeak() and setIntervalWeak() as proxies to _unrefActive() -- in an ideal world setTimeout().unref() would do that, but it's not possible since you may be doing that later than timer creation and as such have to do a linear-time walk and insert into the millisecond list.

It would be possible to check if it is in the same tick, probably, but you've still got a problem then because the backing handle may or may not be shared and it's somewhat hard to tell without mucking with internals.

That wouldn't a problem if no-one touched unrefed timer handles in the ecosystem but I doubt that is the case, and still doesn't resolve unrefing at a later time. (Yay undeterministic APIs 😞)

---

Scoped, does this change seem fine to you? I should probably open the issue for the other thing but I'm not sure of a reasonable way to go about it, it's not exactly simple. :(

[misterdjules]

Before this change, we could tell the difference between an internal timer instance and an external one by checking the _unrefed property on a timeout's list

You can just check the handle like this PR, what use is it to us?

My point was that with this change, by just checking the handle of a timer TimerWrap instance, one cannot tell if it's an unrefed external timer, or an unrefed internal timer. With _unrefed the _unrefed property on TimersList instances, which really means _internal, one can tell the difference.

Scoped, does this change seem fine to you?

It seems like it might break at least one edge case, and it doesn't seem to be solving any problem. What's the motivation for this change?

[Fishrock123]

It seems like it might break at least one edge case, and it doesn't seem to be solving any problem. What's the motivation for this change?

Cleanup. Keep in mind there wasn't a good way to tell what the previous shared internal handle was either from just looking at the handle, so I'm not sure how real that concern is?

[misterdjules]

Cleanup.

The code after the change doesn't look cleaner to me, because it seems we lose a very important piece of data which is a marker that identifies that the TimersList to which the timer belongs is the list of internal timers.

We're lucky currently to have external unrefed timers to be moved out of their TimersList, and so most if not all things that are exposed to users don't break but it seems to be a slippery slope. The fact that this change breaks as you mentioned if someone unrefs the handle without using the Timeout.prototype.unref API is to me a clear indicator of this, even if this may not have any impact on most if not all users now.

Keep in mind there wasn't a good way to tell what the previous shared internal handle was either from just looking at the handle, so I'm not sure how real that concern is?

Absolutely, I didn't mean to say the contrary. I updated my previous comment to hopefully clear the confusion. My concern is different, hopefully I managed to present it in a way that is clearer this time.

[Fishrock123]

The code after the change doesn't look cleaner to me, because it seems we lose a very important piece of data which is a marker that identifies that the TimersList to which the timer belongs is the list of internal timers.

Consider that you also haven't been able to know if a Referenced TimerWrap was an internal active() backing one, or one from Timeout#unref()#ref(). Non-issue in my books.

The fact that this change breaks as you mentioned if someone unrefs the handle without using the Timeout.prototype.unref API is to me a clear indicator of this, even if this may not have any impact on most if not all users now.

You don't unref the handle itself from Timeout.prototype.unref either... This will only break if you uv_{un}ref the hidden, internal handle. (Including for non-internal timers. If you unref that handle from C++ you will also have problems, but that seems like a bad thing to worry about anyways as there will likely be problems from doing that? (I.e. it is unsafe to begin with))

[misterdjules]

Consider that you also haven't been able to know if a Referenced TimerWrap was an internal active() backing one, or one from Timeout#unref()#ref(). Non-issue in my books.

I don't think using limitations of other aspects of the implementation as a reference for what we want to achieve is going to help us improve that implementation. It seems to me that it's another argument in favor of trying to not conflate TimerWrap instances' internal handles' "refedness" with the their internal nature.

You don't unref the handle itself from Timeout.prototype.unref either...

I don't understand what you mean. Do you mean that the handle itself is not "unrefered" when Timeout.prototype.unref is called? If so, that is not true. Or do you mean that I said that the problem with your change was surfacing when calling Timeout.prototype.unref? If so, that is not what I wrote, so I feel that I must be missing something.

This will only break if you uv_{un}ref the hidden, internal handle. (Including for non-internal timers. If you unref that handle from C++ you will also have problems, but that seems like a bad thing to worry about anyways as there will likely be problems from doing that? (I.e. it is unsafe to begin with))

I agree, and I have not suggested otherwise.

What I've been trying to convey in my comments is that removing the _unrefed property from the implementation in lib/timers.js by itself doesn't seem like an improvement to me. The immediate negative impact of this change is small, but without a larger refactoring of how internal timers are implemented, this seems like a change in the wrong direction, because it seems like it conflates the internal nature of some timers with whether their internal handles are refed/unrefed.

I would instead like to explore other approaches such as:

1. Using a separate kOnTimeout callback for internal and external timers, similar to the previous distinction between listOnTimeout and unrefTimeout, but with both functions sharing most of their implementation.
2. Keeping a reference to the list of TimersList instances from a TimersList instance.

There are probably other approaches to this problem, but the key to me would be to have the internal nature of timers and their "internal handle's refedness" being two orthogonal concepts.

[Fishrock123]

Ah, yes. I would tend to agree.

I'll take a look at those options when I get the chance.

I original considered having a ref back to the lists list on the TimersList but I didn't do that for reason(s) which I do not recall.

[jasnell]

Ping @Fishrock123 ... still want to pursue this?

[Fishrock123]

@jasnell No; but this is a reminder for me to look into doing what @misterdjules described

[BridgeAR]

Closing due to long inactivity.