timers: setInterval interval includes cb duration #14815

zhangzifa · 2017-08-14T02:15:07Z

setInterval callback should be scheduled on the interval

Fixes: #7346

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

zhangzifa · 2017-08-14T02:17:21Z

@Fishrock123 Could you help view this?
@mitsos1os Please check if this fulfills your requirement.

Trott · 2017-08-14T02:56:16Z

test/parallel/test-timers-setInterval-excludes-callback-duration.js

+require('../common');
+const assert = require('assert');
+
+const sleep = function(ms) {


The linter will want this to be either an arrow function or else a function declaration. So either:

const sleep = (ms) => {

...or...

function sleep(ms) {

Trott · 2017-08-14T02:57:07Z

test/parallel/test-timers-setInterval-excludes-callback-duration.js

+  while (new Date().getTime() < st + ms);
+};
+
+var cntr = 0;


Linter will flag this and the next line and want it to be let instead of var.

Trott · 2017-08-14T02:57:27Z

test/parallel/test-timers-setInterval-excludes-callback-duration.js

+
+var cntr = 0;
+var first, second;
+setInterval(function(){


Linter will expect a space before the {

Trott · 2017-08-14T03:01:07Z

Hi, @zhangzifa! Thanks for doing this! I was looking at that issue just yesterday. Glad someone is working to fix the issue!

Trott · 2017-08-14T03:09:59Z

lib/timers.js

  const msecs = item._idleTimeout;
  if (msecs < 0 || msecs === undefined) return;

-  item._idleStart = TimerWrap.now();
+  if (start) {


Because we know that start will always be an integer, this is probably more performant as if (start !== 0).

Trott · 2017-08-14T03:17:27Z

Surprisingly (to me at least), there does not seem to be any benchmarks for setInterval() in the Node.js code base.

zhangzifa · 2017-08-14T03:40:05Z

thanks @Trott for your comments.

benjamingr · 2017-08-14T07:28:08Z

@Trott I recall @mscdex did some interesting work on timers a while back and had benchmarks.

benjamingr

This approve is on the content of the PR.

I'd like a CITGM run on this one - and a CTC member signing off because it's a potentially breaking change. (Looks like Trott has that covered)

bnoordhuis · 2017-08-14T08:17:27Z

How does this behave with an interval of n ms and a callback that takes > n ms?

zhangzifa · 2017-08-14T08:25:26Z

@bnoordhuis
When callback takes > n ms, the real interval is the duration of callback.

tniessen · 2017-08-14T11:40:28Z

test/parallel/test-timers-setInterval-excludes-callback-duration.js

+
+let cntr = 0;
+let first, second;
+setInterval(function() {


You could as well use an arrow function here. (Just a personal preference, feel free to ignore.)

tniessen · 2017-08-14T11:42:14Z

test/parallel/test-timers-setInterval-excludes-callback-duration.js

+  } else if (cntr === 2) {
+    second = new Date();
+    assert(Math.abs(second - first - 200) < 10);
+    process.exit(0);


Is there any advantage of using process.exit(0) instead of clearInterval()?

Fishrock123

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

The change seems fine and looks ok as a patch to me.

I'm not so comfortable with the test, it looks like it should be in /pummel/ - perhaps we can override and hook into TimerWrap.now() or use async-hooks(?) to check invocation order.

Fishrock123 · 2017-08-14T15:58:04Z

lib/timers.js

@@ -465,6 +469,7 @@ function ontimeout(timer) {
  var callback = timer._onTimeout;
  if (typeof callback !== 'function')
    return promiseResolve(callback, args[0]);
+  var start = TimerWrap.now();


const for new values please

Fishrock123 · 2017-08-14T16:06:00Z

When callback takes > n ms, the real interval is the duration of callback.

I think @bnoordhuis might be asking if it will loop infinitely - that is will the next callback be called in the same timerslist iteration? (I'm uncertain offhand but will try to take a look too.)

Edit: assigning myself so I don't forget.

misterdjules · 2017-08-14T18:08:37Z

test/parallel/test-timers-setInterval-excludes-callback-duration.js

+let cntr = 0;
+let first, second;
+setInterval(function() {
+  sleep(100);


Should we use common.busyLoop instead of defining another function that does something similar?

misterdjules · 2017-08-14T18:11:23Z

test/parallel/test-timers-setInterval-excludes-callback-duration.js

+  if (cntr === 1) {
+    first = new Date();
+  } else if (cntr === 2) {
+    second = new Date();


In general I would suggest not using wall-clock time, and instead use a monotonic clock by e.g calling Timer.now().

zhangzifa · 2017-08-15T03:36:39Z

@Fishrock123 & @bnoordhuis

When callback takes > n ms, the real interval is the duration of callback.

I think @bnoordhuis might be asking if it will loop infinitely - that is will the next callback be called in the same timerslist iteration? (I'm uncertain offhand but will try to take a look too.)

Your concern is important, I updated the code a bit to fix it.

andrasq · 2017-08-15T16:12:56Z

Is this breaking? It changes the expected behavior of interval timers, and how they should be reasoned about.

The intended behavior is already possible with a setImmediate and a mutex: the interval function can arrange for the handler to be called and return to schedule the next run still on the same tick, with the mutex preventing overlapping runs. Something like (arg passing omitted):

// invoke handler() on a preset scheduling interval
funcion setExactInterval( handler, intervalMs ) {
  var running = false;
  function runHandler( ) {
    running = true;
    try {
      handler();
      running = false;
    } catch (err) {
      running = false;
      throw err;
    }
  }
  return setInterval(function(){ if (!running) setImmediate(runHandler) }, intervalMs);
}

There is merit in considering an interval function that always fires on the exact scheduled time, but then it should always be on multiples of the interval, even the duration exceeds it. I.e., if the handler runs too long, the next scheduled time should be the earliest time point that was part of the original sequence, and not the first time point of a new, re-anchored or adjusted sequence.

Fishrock123

The test doesn't appear to work locally. New CI: https://ci.nodejs.org/job/node-test-pull-request/9674/

Unfortunately using process.nextTick() causes something wrong with AsyncHooks in the timerwrap.setInterval test. I'm not sure why but perhaps something else is not quite right in the timers code with AsyncHooks?

Fishrock123 · 2017-08-15T16:21:16Z

lib/timers.js

+    if (duration >= timer._repeat) {
+      // If callback duration >= timer._repeat,
+      // add 1 ms to avoid blocking eventloop
+      insert(timer, false, start + duration - timer._repeat + 1);


I think just wrapping insert() in process.nextTick() should be enough. If not, perhaps just adding +1ms will work fine. (Timer granularity is only 1-2ms anyways even on fast machines afaik.)

Wrap insert in process.nextTick and fix the test.

zhangzifa · 2017-08-15T16:59:53Z

hello @andrasq

Is this breaking? It changes the expected behavior of interval timers, and how they should be reasoned about.

I don't know if there is any spec/doc describes the expected behavior of interval timers for Node.js. I only found this for html, which tells setInterval behavior: Schedules a timeout to run handler every timeout milliseconds. Any arguments are passed straight through to the handler.

BTW, this PR does not change the order of different timer with same interval.

Fishrock123 · 2017-08-15T19:25:18Z

lib/timers.js

+      process.nextTick(insert, timer, false, start + duration - timer._repeat);
+    } else {
+      insert(timer, false, start);
+    }


I had meant the entire block here, I don't think the conditional or the +- start is necessary.

In an ideal world, replacing active(timer) with process.nextTick(insert, timer, false, start) would be all that were necessary. Unfortunately that causes a slight continuity break and the existing timers iteration code is unaware that a timer will be scheduled in the same slot but only after this iteration, causing the underlying TimerWrap to be re-created (and thus impacting AsyncHooks tests).

Imo, insert(timer, false, start + 1) is the way to go. (Perhaps with a conditional check still.)

Yes, the conditional check is needed if callback > interval, otherwise, loop will be blocked.

Fishrock123 · 2017-08-15T19:27:43Z

There is merit in considering an interval function that always fires on the exact scheduled time, but then it should always be on multiples of the interval, even the duration exceeds it. I.e., if the handler runs too long, the next scheduled time should be the earliest time point that was part of the original sequence, and not the first time point of a new, re-anchored or adjusted sequence.

I think this should maintain that, but with the caveat of an event turn break. We've maintained some consistency about not firing timers during scheduling in the past and also this prevents infinite loops.

BridgeAR · 2017-09-03T23:56:54Z

Ping @Fishrock123

BridgeAR · 2017-09-13T19:13:31Z

@Fishrock123 PTAL

BridgeAR · 2017-09-19T21:19:49Z

@nodejs/tsc PTAL

mscdex · 2017-09-19T21:30:20Z

Has anyone checked with how browsers handle this? We should align with how they do it.

Dismissing the review due to no response. PTAL

Fishrock123 · 2017-11-23T21:22:22Z

FWIW, the following test appears to "pass" in Chrome, FireFox, and Safari.

I think that indicates that browsers follow the behavior in this PR?

'use strict';

function busyLoop(time) {
  const startTime = Date.now();
  const stopTime = startTime + time;
  while (Date.now() < stopTime) {}
}

let cntr = 0;
let first, second;
const t = setInterval(() => {
  busyLoop(50);
  cntr++;
  if (cntr === 1) {
    first = Date.now();
  } else if (cntr === 2) {
    second = Date.now();
    if (Math.abs(second - first - 100) < 10) {
	  throw new Error('Assertion failed!');
    }
    clearInterval(t);
  }
});

(Still passes if you replace Date.now() with performance.now().)

targos · 2017-11-29T17:38:15Z

+1 on the change. We should check CITGM before merging.

targos · 2017-11-29T17:50:37Z

The HTML standard specs timers here: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers

I'm not sure I read it correctly, but it seems that browsers should behave like Node is behaving without this change (schedule the next task after execution - step 5.3)?

mhdawson · 2017-11-29T17:53:56Z

We discussed in the TSC meeting this week. Behavior in this PR sounds right and since it is closer to browser behavior it is also good from a compatibility front. Main concern is about breaking existing code.

There was an additional question if it should queue if multiple intervals are missed. It would be good to check browser behavior and queue if that is what happens there, but that can be handled in a later PR.

The net is that this can go forward provided we get a CITGM run to validate we don't see unexpected breakage of existing modules.

mhdawson

LGTM

apapirovski · 2017-11-29T17:56:57Z

Behavior in this PR sounds right and since it is closer to browser behavior it is also good from a compatibility front.

Should we check if there's any movement from the browser side to potentially re-implement this to follow the spec? It would be unfortunate to end up in a situation where we have a number of releases where we mirror the browsers only to then have them adjust it and for Node have to go back to doing it the old way.

apapirovski · 2017-11-29T18:07:37Z

Relevant issue on WHATWG: whatwg/html#3151

To sum up: it seems likely that the spec is changed to follow the current browser behaviour.

apapirovski · 2017-11-29T18:08:37Z

Also, Chrome drift correction code: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/Timer.cpp?rcl=e6d900fb6ed08dbd3a048899f38962ee75f4d8d0&l=162

apapirovski · 2017-11-29T18:15:08Z

lib/timers.js

  const msecs = item._idleTimeout;
  if (msecs < 0 || msecs === undefined) return;

-  item._idleStart = TimerWrap.now();
+  if (typeof start === 'number') {


Could this just check for start === undefined instead of typeof?

Yes, that should be sufficient.

Trott · 2017-12-01T05:56:53Z

Should the tsc-agenda flag be removed? @mhdawson

mhdawson · 2017-12-01T21:38:08Z

Removed tsc-agenda, @apapirovski thanks for following up on the spec side. Its good to know that the spec will line up with the direction we are taking.

BridgeAR · 2017-12-06T00:25:44Z

@zhangzifa this seems to be finally in a state it could land. It only needs another rebase.

setInterval callback should be scheduled on the interval Fixes: nodejs#7346

Trott · 2017-12-15T17:36:29Z

@BridgeAR @zhangzifa I rebased and force pushed. The one merge conflict was simple to resolve. PTAL.

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

BridgeAR · 2017-12-15T23:58:22Z

lib/timers.js

  if (!args)
    timer._onTimeout();
  else
    Reflect.apply(timer._onTimeout, timer, args);
  if (timer._repeat)
-    rearm(timer);
+    rearm(timer, start);


Why is TimerWrap.now() not called here? It is the only used spot in the function, so it does not have to be called if timer._repeat is falsy.

BridgeAR · 2017-12-15T23:58:28Z

lib/timers.js

  const msecs = item._idleTimeout;
  if (msecs < 0 || msecs === undefined) return;

-  item._idleStart = TimerWrap.now();
+  if (typeof start === 'number') {


Yes, that should be sufficient.

BridgeAR · 2018-01-05T15:20:40Z

@apapirovski what do you think about landing this right now and just improve the code afterwards in a separate PR?

BridgeAR · 2018-01-12T17:28:55Z

Landed in 1385e1b

I went ahead and landed this without any further changes as I think we can work on it again and improve it later on but it would be good to have this already as is.

setInterval callback should be scheduled on the interval Fixes: nodejs#7346 PR-URL: nodejs#14815 Fixes: nodejs#7346 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

apapirovski · 2018-01-14T03:40:54Z

This should've had another CI run before landing, OS X is consistently failing on CI now. https://ci.nodejs.org/job/node-test-commit-osx/

jasnell · 2018-01-14T03:45:51Z

Ouch. Let's revert if a quick fix cannot be found

Trott · 2018-01-14T03:49:12Z

The test has an arbitrary 10ms window of accuracy after an interval runs twice. A possibly-improved approach (but still imperfect) might be to run an interval more than that--perhaps 10 times? or 16 if you like powers of 2?--and make the check a bit more generous.

Trott · 2018-01-14T03:50:01Z

(But I'm totally +100 on a revert, whether or not a quick fix can be found. Revert, then re-apply with fix is fine by me.)

nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Aug 14, 2017

Trott reviewed Aug 14, 2017

View reviewed changes

benjamingr approved these changes Aug 14, 2017

View reviewed changes

tniessen reviewed Aug 14, 2017

View reviewed changes

Fishrock123 reviewed Aug 14, 2017

View reviewed changes

Fishrock123 self-assigned this Aug 14, 2017

misterdjules reviewed Aug 14, 2017

View reviewed changes

Fishrock123 suggested changes Aug 15, 2017

View reviewed changes

Fishrock123 previously requested changes Aug 15, 2017

View reviewed changes

mhdawson mentioned this pull request Nov 27, 2017

Node.js Foundation Technical Steering Committee (TSC) Meeting 2017-11-29 nodejs/TSC#429

Closed

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017

mhdawson approved these changes Nov 29, 2017

View reviewed changes

apapirovski reviewed Nov 29, 2017

View reviewed changes

mhdawson removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Dec 1, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

timers: setInterval interval includes cb duration

3e47c6c

setInterval callback should be scheduled on the interval Fixes: nodejs#7346

Trott force-pushed the master branch from 761572f to 3e47c6c Compare December 15, 2017 17:34

BridgeAR reviewed Dec 15, 2017

View reviewed changes

BridgeAR closed this Jan 12, 2018

timers: setInterval interval includes cb duration #14815

timers: setInterval interval includes cb duration #14815

Conversation

zhangzifa commented Aug 14, 2017 • edited Loading

Checklist

Affected core subsystem(s)

zhangzifa commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Aug 14, 2017

Choose a reason for hiding this comment

Trott commented Aug 14, 2017 • edited Loading

zhangzifa commented Aug 14, 2017

benjamingr commented Aug 14, 2017

benjamingr left a comment

Choose a reason for hiding this comment

bnoordhuis commented Aug 14, 2017

zhangzifa commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangzifa commented Aug 15, 2017

andrasq commented Aug 15, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangzifa commented Aug 15, 2017

Fishrock123 Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Aug 15, 2017

BridgeAR commented Sep 3, 2017

BridgeAR commented Sep 13, 2017

BridgeAR commented Sep 19, 2017

mscdex commented Sep 19, 2017

Fishrock123 commented Nov 23, 2017 • edited Loading

targos commented Nov 29, 2017

targos commented Nov 29, 2017

mhdawson commented Nov 29, 2017

mhdawson left a comment

Choose a reason for hiding this comment

apapirovski commented Nov 29, 2017

apapirovski commented Nov 29, 2017 • edited Loading

apapirovski commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Dec 1, 2017

mhdawson commented Dec 1, 2017

BridgeAR commented Dec 6, 2017

Trott commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 5, 2018

BridgeAR commented Jan 12, 2018

apapirovski commented Jan 14, 2018

jasnell commented Jan 14, 2018

Trott commented Jan 14, 2018

Trott commented Jan 14, 2018

zhangzifa commented Aug 14, 2017 •

edited

Loading

Trott commented Aug 14, 2017 •

edited

Loading

Fishrock123 left a comment •

edited

Loading

Fishrock123 commented Aug 14, 2017 •

edited

Loading

Fishrock123 Aug 15, 2017 •

edited

Loading

Fishrock123 commented Nov 23, 2017 •

edited

Loading

apapirovski commented Nov 29, 2017 •

edited

Loading