[DEP0096] DeprecationWarning: timers.unenroll() question

[dougwilson]

• Version: v10.0.0
• Platform: Windows 20 64-but

I have a question on this deprecation. I am using Timers.active to start a passive timer on a socket and Timers.unenroll to stop it. I see it says I need to change unenroll to clearTimeout but nothing about Timers.active. Will clearTimeout unenroll the timer created from Timers.active ?

[jasnell]

/cc @Fishrock123

[Fishrock123]

It will work, yes, but you should probably move to using setTimeout. (The old API causes more issues than it solves perf-wise.)

Also, Timers.active doesn't really initialize everything properly. (That's what enroll() was for.)

Timers.active() is not yet deprecated as it is useful to refresh existing timeouts (even those made by setTimeout in-place without allocating new resources.

In the future I want to expose this internal function which does that in a better way but I wasn't sure what the end API should be: https://github.com/nodejs/node/blob/master/lib/internal/timers.js#L85-L96

I could do that sooner I suppose, as the API seems fine to me. I guess browsers might also be interested in this so maybe in the distant future refreshTimeout() would be better, but I am not going to bother with trying to do web standards stuff.

[Fishrock123]

Maybe the deprecation should also mention that if you are using Timers.active in that way you should probably move away from it...?

[dougwilson]

Maybe. I'm fine with moving, just not sure how to make the move. This came out from the "mysql" module. Is there like a guide I can read to understand the changes I need to make?

[dougwilson]

I'm just basically enrolling an object to a timer and I extend the timeout every time a data chunk arrives so it is basically an inactivity timeout.

[Fishrock123]

This older code:

class ThingWithTimer {
  constructor () {
    Timers.enroll(this)
  }

  start ()  {
   Timers.active(this)
  }

  refresh () {
    Timers.active(this)
  }
  
  done ()  {
    Timers.unenroll(this)
  }
}

Becomes something like:

class ThingWithTimer {
  constructor () {
    this.timer = null
  }

  start () {
    this.timer = setTimeout()
  }

  refresh () {
    // Refresh timeout in-place
    Timers.active(this.timer) // Eventually, this.timer.refresh() or refreshTimeout(this.timer)
  }

  done () {
    clearTimeout(this.timer)
  }
}

[dougwilson]

Awesome, will try it out. Is there a way to feature detect when to use this new method vs the old method?

[dougwilson]

Also I saw in the code there is Timeout.prototype[refreshFnSymbol] already. Any reason not to bring it out from behind a symbol in like 10.1? Seems odd to mix the two APIs temporarily. It would also be an easy way to feature detect from my question above: see if there is a refresh method for timers, right?

[Fishrock123]

Is there a way to feature detect when to use this new method vs the old method?

Always. I think you need to be on like Node 0.8 for the full API to be realistically worse for perf. The APIs in the example exist and function correctly on all versions of Node.

---

Any reason not to bring it out from behind a symbol in like 10.1?

From the above, I wasn't sure if something like refreshTimeout(timer) would be a better API. Browsers could make use of something like that (although as stated I don't really want a part in that).

Such an approach could also work better with #19683. (Although something like a getTimerFromId would also work.)

Ehhh, I'll open a PR to make it public. :)

[Fishrock123]

Ok, PR up at #20298