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

Scheduling timers during tick with timeout of 0 forcing timeout of 1 #166

Closed
SimenB opened this issue May 8, 2018 · 9 comments
Closed

Comments

@SimenB
Copy link
Member

SimenB commented May 8, 2018

See the following code: https://github.com/sinonjs/lolex/blob/63b192998893a3a22203d7b0741864e2fc1b91ce/src/lolex-src.js#L219

Jest has a test that running fake timers bail if the infinitely recurse, it looks like this:

it.only('throws before allowing infinite recursion', () => {
  const global = {Date, clearTimeout, process, setTimeout};
  const timers = new FakeTimers({
    global,
    maxLoops: 100,
    moduleMocker,
    timerConfig,
  });
  timers.useFakeTimers();

  global.setTimeout(function infinitelyRecursingCallback() {
    global.setTimeout(infinitelyRecursingCallback, 0);
  }, 0);

  expect(() => {
    timers.advanceTimersByTime(50);
  }).toThrow(
    new Error(
      "Ran 100 timers, and there are still more! Assuming we've hit an " +
        'infinite recursion and bailing out...',
    ),
  );
})

timers.advanceTimersByTime is implemented using clock.tick.

However, this doesn't throw anymore, and I think it's because of the code line I linked above.

My question is: is it on purpose that when parseInt(timer.delay) is 0 (thus falsy) that you should care about duringTick?

@benjamingr
Copy link
Member

Web timers delay subsequent nested timer calls. Although the spec uses a different quota:

Timers can be nested; after five such nested timers, however, the interval is forced to be at least four milliseconds.

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

Cool! I'll just remove that test from Jest then, as it tried to assert the wrong behavior. Thanks for the quick response!

@SimenB SimenB closed this as completed May 8, 2018
@benjamingr
Copy link
Member

Note that we do have a config.loopLimit which you can use to avoid infinite recursion with timers if you'd like to preserve the bailing test.

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

loopLimit is only respected in runAll, not any of the other functions (such as clock.tick which is used here).

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

@benjamingr a question popped up - that's the HTML spec, is it implemented the same in Node?

@benjamingr
Copy link
Member

@SimenB yes, pretty much - though if you'd like more "official clarification" of what we can commit to for timers I can bring it up internally in Node. We don't actually have a spec the way web timers do but try to stay compatible.

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

Nah, as long as it matches, and it's a goal to match in the future (so it won't drift more or less arbitrarily) that's more than enough for me 🙂

In general I wouldn't mind the node docs explicitly stating at least where their implementation deviates from web specs, but that's not timer specific.

@benjamingr
Copy link
Member

In general I wouldn't mind the node docs explicitly stating at least where their implementation deviates from web specs, but that's not timer specific.

Well, timers are pretty much the only API node has that looks like a web API and isn't - the other stuff either has different naming and methods (EventTarget vs EventEmitter) or is actually the same (like URL).

@SimenB
Copy link
Member Author

SimenB commented May 8, 2018

Ah, fair enough!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants