elapse inside periodic timer immediately calls the same timer

[gnprice]

If the callback of a periodic timer calls elapse or flushTimers, the same periodic timer immediately gets called again — even if its next scheduled call is far in the future and there are other timers scheduled sooner.

The reason is that _nextCall gets updated only after invoking the timer's callback, instead of before:

    if (isPeriodic) {
      // ignore: avoid_dynamic_calls
      _callback(this);
      _nextCall += duration;

Fortunately the fix is easy: just swap those two statements. I'll send a PR.

Discovered this when studying the code to answer @lrhn's question at dart-archive/fake_async#85 (comment) about re-entrancy.

[lrhn]

To be precise, the problem is not that it immediately calls the timer if it had been due again after the elapse, but that it immediately runs it at the same tick again, because the state was not updated before running user code.

Dart periodic timers are under-specified. The native overs try to be isochronic, which isn't always possible and why we have the tick counter, the web timers do not, they just reschedule duration later when they are done running. I don't remember if we try to correct for that.

The change here makes the simulated timers act like native timers, and the elapse acts like a large lag spike happening while running the callback, so it's reasonable behaviour.
The internal state is updated to a consistent state (this timer has triggered, it's no longer due) before we run user code that can change state.

(It is a little worrisome if elapse runs other events inside a timer callback. It really should wait until the callback returns to continue running the event loop. That would probably break since tests, though.)

[gnprice]

To be precise, the problem is not that it immediately calls the timer if it had been due again after the elapse, but that it immediately runs it at the same tick again, because the state was not updated before running user code.

The tick as reported by [Timer.tick] was actually already getting correctly updated — the _tick++; line came before invoking the callback:

    _tick++;
    if (isPeriodic) {
      // ignore: avoid_dynamic_calls
      _callback(this);
      _nextCall += duration;

and that's the field that [FakeTimer.tick] reads.

I'm not sure I fully understand the next paragraph:

Dart periodic timers are under-specified. The native overs try to be isochronic, which isn't always possible and why we have the tick counter, the web timers do not, they just reschedule duration later when they are done running. I don't remember if we try to correct for that.

but it sounds like you're saying the old behavior (before dart-archive/fake_async#89 fixed this issue #2319) was within the range of behavior that the real timers can sometimes have, due to the fact that providing ideal timer behavior is impossible (at least for a general-purpose application platform).

In any case I think we're all agreed that the new behavior post-#89 is preferable.

At a high level the reason I'd give for that is: generally a fake implementation of something, for use in tests, should by default behave in an idealized way, with the behavior that a real implementation would have under ideal conditions (or even that the real implementation would ideally have under ideal conditions). That's good for predictability, not only in the sense of determinism (as opposed to flakiness) but also in the sense that it makes it easy to look at the test source code and reason through what will happen.

Of course it's often important to test how code behaves under non-ideal conditions (for example if timers get fired late because the system was busy)… but that should be something a test chooses explicitly. After all, to really have an effective test it'll usually be necessary anyway to specify some details of just how the conditions are non-ideal. Here for example, a test can use elapseBlocking to simulate a lag spike and cause simulated timers to fire late.

[lrhn]

a fake implementation of something, for use in tests, should by default behave in an idealized way, with the behavior that a real implementation would have under ideal conditions (or even that the real implementation would ideally have under ideal conditions)

That's indeed, as you say, one use of testing, which removes focus from the fake implementation by making it as predictable and controllable as possible for the test scaffolding.

The other use-case you mention would be stress-testing, to have an implementation that can span the full range of valid behaviors, so that code making non-guaranteed assumptions gets caught doing so.
If the fake timing used in testing behaves "better" than production, you haven't actually tested what will happen in production.

Both have value. The FakeAsync here is probably mainly of the former kind ("controllable and predictable").

[gnprice]

Yeah, both kinds are important.

FakeAsync as it stands (and as it stood before dart-archive/fake_async#89) doesn't really provide stress-testing of timer behavior. Just having timers fire early when elapse or flushTimers are called re-entrantly from a timer callback isn't a useful degree of stress-testing on its own — not only is that a fairly narrow situation, but it's one that can only arise with the active involvement of test code (since non-test code doesn't have a FakeAsync instance it could call elapse or flushTimers on).

If someone's writing a stress test of how some code responds to timers, then they'll want to exercise, as you say, the full range of valid timer behaviors. That means timers firing late as well as early; and non-periodic timers being affected as well as periodic timers; and applying without regard to test code happening to have a re-entrant timer callback. Probably someone doing that will want to start by forking FakeAsync, or perhaps rewriting it from scratch.

In short, a stress-testing implementation is useful only if it actually stress-tests, systematically exercising at least a wide range of possible behavior. A fake implementation that, like FakeAsync, isn't specifically aimed at stress-testing should by default provide idealized behavior.

[lrhn]

A better behavior might be that nothing fires while elapsing, when it's done inside an existing event callback.
Instead that should just increase time, and then events would fire when you got back to the event loop.
The FakeAsync design isn't really geared for the fake events themselves being used to elapse time. (But rewriting would likely break some tests.)