-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
timers: setInterval interval includes cb duration #14815
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,11 +163,15 @@ exports._unrefActive = function(item) { | |
// Appends a timer onto the end of an existing timers list, or creates a new | ||
// TimerWrap backed list if one does not already exist for the specified timeout | ||
// duration. | ||
function insert(item, unrefed) { | ||
function insert(item, unrefed, start) { | ||
const msecs = item._idleTimeout; | ||
if (msecs < 0 || msecs === undefined) return; | ||
|
||
item._idleStart = TimerWrap.now(); | ||
if (typeof start === 'number') { | ||
item._idleStart = start; | ||
} else { | ||
item._idleStart = TimerWrap.now(); | ||
} | ||
|
||
const lists = unrefed === true ? unrefedLists : refedLists; | ||
|
||
|
@@ -462,16 +466,17 @@ function ontimeout(timer) { | |
var args = timer._timerArgs; | ||
if (typeof timer._onTimeout !== 'function') | ||
return promiseResolve(timer._onTimeout, args[0]); | ||
const start = TimerWrap.now(); | ||
if (!args) | ||
timer._onTimeout(); | ||
else | ||
Reflect.apply(timer._onTimeout, timer, args); | ||
if (timer._repeat) | ||
rearm(timer); | ||
rearm(timer, start); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is |
||
} | ||
|
||
|
||
function rearm(timer) { | ||
function rearm(timer, start) { | ||
// // Do not re-arm unenroll'd or closed timers. | ||
if (timer._idleTimeout === -1) return; | ||
|
||
|
@@ -480,7 +485,15 @@ function rearm(timer) { | |
timer._handle.start(timer._repeat); | ||
} else { | ||
timer._idleTimeout = timer._repeat; | ||
active(timer); | ||
|
||
const duration = TimerWrap.now() - start; | ||
if (duration >= timer._repeat) { | ||
// If callback duration >= timer._repeat, | ||
// add 1 ms to avoid blocking eventloop | ||
insert(timer, false, start + duration - timer._repeat + 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think just wrapping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrap |
||
} else { | ||
insert(timer, false, start); | ||
} | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const Timer = process.binding('timer_wrap').Timer; | ||
const assert = require('assert'); | ||
|
||
let cntr = 0; | ||
let first, second; | ||
const t = setInterval(() => { | ||
common.busyLoop(50); | ||
cntr++; | ||
if (cntr === 1) { | ||
first = Timer.now(); | ||
} else if (cntr === 2) { | ||
second = Timer.now(); | ||
assert(Math.abs(second - first - 100) < 10); | ||
clearInterval(t); | ||
} | ||
}, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just check for
start === undefined
instead oftypeof
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that should be sufficient.