Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DEP0096] DeprecationWarning: timers.unenroll() question #20261

Closed
dougwilson opened this issue Apr 24, 2018 · 13 comments
Closed

[DEP0096] DeprecationWarning: timers.unenroll() question #20261

dougwilson opened this issue Apr 24, 2018 · 13 comments
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@dougwilson
Copy link
Member

  • 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
Copy link
Member

jasnell commented Apr 24, 2018

/cc @Fishrock123

@jasnell jasnell added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Apr 24, 2018
@Fishrock123
Copy link
Contributor

Fishrock123 commented Apr 24, 2018

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
Copy link
Contributor

Fishrock123 commented Apr 24, 2018

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

@dougwilson
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

Fishrock123 commented Apr 24, 2018

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
Copy link
Member Author

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

@dougwilson
Copy link
Member Author

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
Copy link
Contributor

Fishrock123 commented Apr 25, 2018

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
Copy link
Contributor

Ok, PR up at #20298

Fishrock123 added a commit to Fishrock123/node that referenced this issue May 8, 2018
Originally added in
bb5575a
discussions such as
nodejs#20261
show the usefulness of this API to the Node.js ecosystem.

PR-URL: nodejs#20298
ryzokuken pushed a commit that referenced this issue May 10, 2018
Originally added in
bb5575a
discussions such as
#20261
show the usefulness of this API to the Node.js ecosystem.

PR-URL: #20298

Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
targos pushed a commit that referenced this issue May 12, 2018
Originally added in
bb5575a
discussions such as
#20261
show the usefulness of this API to the Node.js ecosystem.

PR-URL: #20298

Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 17, 2018

#20298 landed and was released in 11.0.0.

@dougwilson Can this issue be closed? Or is there more to be done here?

@dougwilson
Copy link
Member Author

Yes, all resolved, thank you.

@apurv195
Copy link

Just update the mysql library to latest version

npm install mysql@2.16.0 --save

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

No branches or pull requests

5 participants