Skip to content
New issue

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

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

Already on GitHub? # to your account

timers: expose rearm() #13144

Closed
mscdex opened this issue May 21, 2017 · 8 comments
Closed

timers: expose rearm() #13144

mscdex opened this issue May 21, 2017 · 8 comments
Labels
feature request Issues that request new features to be added to Node.js. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@mscdex
Copy link
Contributor

mscdex commented May 21, 2017

  • Version: all
  • Platform: n/a
  • Subsystem: timers

It would be nice if timers exported rearm() or at least a variant of it suitable for public consumption. One use case for this is to allow for easier and/or more efficient keepalive mechanisms where a timer needs to be reset after a packet is sent/received.

Currently the only two solutions to achieve this are to constantly start/clear a normal Timeout or use an Interval timer and have some extra logic in the Interval callback to determine whether you should actually execute the real callback (and even then you can lose accuracy). Being able to just rearm(timer) simplifies all of this greatly and has much less overhead than clearTimeout() followed by setTimeout() for every packet.

@mscdex mscdex added feature request Issues that request new features to be added to Node.js. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels May 21, 2017
@XadillaX
Copy link
Contributor

@mscdex Could you give some code to show how to use this feature? Assume function rearm() is already finished and you can use this function in your code.

@tniessen
Copy link
Member

tniessen commented May 22, 2017

@XadillaX I think what @mscdex suggests is something like this. Instead of having to write code such as

function someAsyncWork(callback) {
  console.log('Doing something which must not be done more than once per second');
  setTimeout(callback, 50);
}

let timeout;
function doSomethingPeriodically() {
  someAsyncWork((err) => {
    timeout = setTimeout(doSomethingPeriodically, 1000);
  });
}

timeout = setTimeout(doSomethingPeriodically, 1000);

it would be nice to just write

let timeout;
function doSomethingPeriodically() {
  someAsyncWork((err) => {
    timeout.rearm(); // or timers.rearm(timeout)
  });
}

timeout = setTimeout(doSomethingPeriodically, 1000);

@Fishrock123
Copy link
Contributor

Fishrock123 commented May 22, 2017

Not sure I agree with exposing rearm(), but the behavior you are looking for already exists: just call timers.active() on the timer.

Related to #11736

@Fishrock123
Copy link
Contributor

@mscdex does that solve what you are looking for, or?

See https://github.com/nodejs/node/pull/11154/files#diff-e6ef024c3775d787c38487a6309e491dR240 for an example in practice.

@mscdex
Copy link
Contributor Author

mscdex commented May 24, 2017

@Fishrock123 I haven't tested yet, it may be.

@targos
Copy link
Member

targos commented Jan 21, 2018

should this stay open?

@Fishrock123
Copy link
Contributor

My refresh PR solves this better, I think.

Certainly rearm() should not be used to refresh timers.

@Fishrock123
Copy link
Contributor

Timeout#refresh() is now a public API in Node 10 (afaik) - see 46d335c

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
feature request Issues that request new features to be added to Node.js. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

No branches or pull requests

5 participants