Usually use while (true) { for this pattern in Dart.

Sure, changed.

(That's usually what I'd write too; the for (;;) was preserved from the old _fireTimersWhile.)

I'd make an internal helper _runNextTimer([Duration? until]) that doesn't take a duration relative to now, but takes the actual end time (duration since initialTime).

That's the time (_elapsingTo) that you already have here, and it's the first thing that runNextTimer computes internally anyway.
It avoids repeatedly doing _elapsingTo - _elapsed.

So:

  final elapsingTo = _elapsingTo = _elapsed + duration;
  while (_runNextTimer(elapsingTo)) {}

Then the public function would be:

bool runNextTimer({Duration? timeout]) {
   if (timeout == null) return runNextTimer();
   var timeoutTime = _elapsed + timeout;
   if (_runNextTimer(timeoutTime)) {
     return true;
   } else {
     _elapsed = timeoutTime;
     return false;
   }
}

(I suggest advancing time by at least the [timeout]. If not, it should be renamed to something else, like before. A timeout only applies if time has actually advanced.)

I'd make an internal helper _runNextTimer([Duration? until]) that doesn't take a duration relative to now, but takes the actual end time (duration since initialTime).

Sure, done.

(Though the tests informed me that that final elapsingTo = … version of this call site isn't quite right — the "timers expiring due to elapseBlocking" test shows this loop needs to re-read the _elapsingTo property because it may have changed.)

That's the time (_elapsingTo) that you already have here, and it's the first thing that runNextTimer computes internally anyway.
It avoids repeatedly doing _elapsingTo - _elapsed.

Yeah, those repeated subtractions did feel a bit silly.

This also prompts the thought that perhaps it'd even be good to go a bit further in this direction: perhaps those repeated subtractions are a sign that the actual end time should be what's in the public interface, too.

In particular, just like the two internal callers here, my external use case (gnprice/zulip-flutter@1ad0a6f#diff-03f5c714d04c27b23d2efc576efe97e08095be869e3021d017434bbd88205944R33) also starts by computing the desired end time, and repeatedly subtracts elapsed:

  const timeout = Duration(hours: 1);
  final absoluteTimeout = async.elapsed + timeout;
  while (async.runNextTimer(timeout: absoluteTimeout - async.elapsed)) {

I would still advance the time by timeout before returning false.
Otherwise the name should be different. A timeout triggering means that the time has passed.

I can see that flushTimers doesn't do that, but that's because it throws an error if all (non-periodic) timers are not flushed before the timeout is up, and presumably the test fails then, so advancing the time doesn't matter.
That is, the timeout parameter to runNextTimer is not the same as the one to flushTimers. The latter means "it's an error if we reach it", the former is just "only try to go this far".

If we change the call in flushTimers to the internal _runNextTimer, and it's only the public runNextTimer that advance time on a false result, then we won't change the behavior of flushTimers.

I would still advance the time by timeout before returning false.
Otherwise the name should be different. A timeout triggering means that the time has passed.
[…]
That is, the timeout parameter to runNextTimer is not the same as the one to flushTimers. The latter means "it's an error if we reach it", the former is just "only try to go this far".

Hmm, yeah, I agree — "timeout" isn't the right word to describe this behavior.

I think the "only try to go this far" behavior is the one I'd prefer to expose, though. In particular it's easy for a caller to use that behavior to implement the "error if we reach this" behavior, the same way flushTimers does; but if the method has already advanced time, then the caller can't undo that.

(The flushTimers implementation uses _timers.isEmpty, but an external caller can say .pendingTimers.isEmpty. That's inefficient because currently pendingTimers allocates a List, but there's no reason it should be inefficient to ask that question; I think the inefficiency there is just a case of what you pointed out at #85 (comment) above.)

So I'd prefer to find a better name for this parameter, rather than change the behavior to match the timeout name. Perhaps within? Or before?

(Just a comment: So much of this code would be easier to read (and more efficient) if _timers was a priority queue. And if it tracked the number of periodic timers on the side, this check would just be _timers.length == _periodicTimerCount. All this repeated iteration only works because it's for testing only, and there aren't that many timers in one test.)

Agreed.

I'd code-quote true and false. Generally try to code-quote literals when I refer to language-semantic values, like these, void and null, "done" or 42, to distinguish them from plain text containg the same symbols as English words or numbers.

Makes sense, thanks. Done.

That is, I'd start the internal bool _runNextTimer(Duration? absoluteTimeout) {
here instead.

Mention that the current microtask queue is flushed whether or not there is a timer in range.

Sure, done.

Sure, changed.

(That's usually what I'd write too; the for (;;) was preserved from the old _fireTimersWhile.)

Agreed.

Makes sense, thanks. Done.

Sure, done.

I would still advance the time by timeout before returning false.
Otherwise the name should be different. A timeout triggering means that the time has passed.
[…]
That is, the timeout parameter to runNextTimer is not the same as the one to flushTimers. The latter means "it's an error if we reach it", the former is just "only try to go this far".

Hmm, yeah, I agree — "timeout" isn't the right word to describe this behavior.

I think the "only try to go this far" behavior is the one I'd prefer to expose, though. In particular it's easy for a caller to use that behavior to implement the "error if we reach this" behavior, the same way flushTimers does; but if the method has already advanced time, then the caller can't undo that.

(The flushTimers implementation uses _timers.isEmpty, but an external caller can say .pendingTimers.isEmpty. That's inefficient because currently pendingTimers allocates a List, but there's no reason it should be inefficient to ask that question; I think the inefficiency there is just a case of what you pointed out at #85 (comment) above.)

So I'd prefer to find a better name for this parameter, rather than change the behavior to match the timeout name. Perhaps within? Or before?

I'd make an internal helper _runNextTimer([Duration? until]) that doesn't take a duration relative to now, but takes the actual end time (duration since initialTime).

Sure, done.

(Though the tests informed me that that final elapsingTo = … version of this call site isn't quite right — the "timers expiring due to elapseBlocking" test shows this loop needs to re-read the _elapsingTo property because it may have changed.)

That's the time (_elapsingTo) that you already have here, and it's the first thing that runNextTimer computes internally anyway.
It avoids repeatedly doing _elapsingTo - _elapsed.

Yeah, those repeated subtractions did feel a bit silly.

This also prompts the thought that perhaps it'd even be good to go a bit further in this direction: perhaps those repeated subtractions are a sign that the actual end time should be what's in the public interface, too.

In particular, just like the two internal callers here, my external use case (gnprice/zulip-flutter@1ad0a6f#diff-03f5c714d04c27b23d2efc576efe97e08095be869e3021d017434bbd88205944R33) also starts by computing the desired end time, and repeatedly subtracts elapsed:

  const timeout = Duration(hours: 1);
  final absoluteTimeout = async.elapsed + timeout;
  while (async.runNextTimer(timeout: absoluteTimeout - async.elapsed)) {

Adapted from the suggestion at #85 (review) .

For this class of use case:

if a test wants to simulate other asynchronous non-timer events between timer events, for example simulating UI updates or port communication.

my thinking would be that the convenient way to handle those is to create timers that trigger them. By putting them all on the same timeline measured with Duration, that seems like it makes for a relatively easy-to-think-about way of controlling the relative order of those events and of any timer events that are also happening.

For UI updates, in the case of Flutter with package:flutter_test, I believe those are already triggered by a timer.

As I see it the key things that make this API valuable, in a way that can't be substituted for by just scheduling more timers, are that the test can have its own code run after each timer (however frequent or infrequent those might be on the fake-time timeline), and that it can break out of the loop at will. The use case I have myself, described at #84 and demo'ed at gnprice/zulip-flutter@1ad0a6f , is to do those things to check if an error has been recorded, and if so then to throw and stop running timers.

Agree. If someone wants to simulate events, they still have to simulate when the event occurs, and then it might as well be treated as a timer event. (That also ensures that all prior due timers get handled first.)

Here we could run through all the timers, find the maximal _nextCall among them (or among the non-periodic ones if we ignore periodic timers), starting with "now", and if we don't find any later timers, we're done.

If the latest timer is after absoluteTimeout, then cap it at absolute timeout and remember that we did that.

The just keep runNextTimer until that time.
If we stopped because of the timeout, handle that.

while (true) {
  var timers = timers;
  if (!flushPeriodicTimers) timers = timers.where((t) => !t.isPeridioc);
  var lastEvent = maxBy(timers, (t) => t._nextCall);
  if (lastEvent == null) break;
  var lastEventTime = lastEvent._nextCall;
  var timesOut = false;
  if (lastEventTime > absoluteTimeout) {
    timesOut = true;
    lastEventTime = absoluteTimeout;
  }
  while (runNextEvent(until: lastEventTime)) {}
  if (timesOut) {
    throw StateError('Exceeded timeout $timeout while flushing timers');
  }
}

That seems more efficient than doing a complete scan of the timers after each event.

(Technically we don't know if all later timers were cancelled before we reached the timeout.)

We could do that but it wouldn't change the asymptotics — we'd still be scanning through all the timers each time in order to find the next one to run. That's the minBy call in runNextTimer, and in the existing code in the loop body in _fireTimersWhile.

If we want to optimize this algorithmically, I think the way to do it would be to change the data structures. We could… actually what I was about to write is the same thing you suggested in this comment above: #85 (comment)

That's probably best kept in a separate PR, though.

Ack. Only so much we can do locally can do when every later access is a linear scan too.
This should be fine.

Agree. If someone wants to simulate events, they still have to simulate when the event occurs, and then it might as well be treated as a timer event. (That also ensures that all prior due timers get handled first.)

Is this code re-entrancy safe? (Likely not, code rarely is if it's not designed for it.)

That is, if someone calls runNextTimer from a microtask or timer event callback, so it's inside a running event loop, will something break?
(Should we check for it and maybe throw a state error?)

Hmm, good question.

In fact, when I stared at this code today to try to pin down an answer, I discovered a bug in the existing code:

• #88
• Fully update internal state before invoking periodic-timer callback #89

It's actually normal and useful, though, that elapse gets called from microtasks within flushTimers (and so that runNextTimer gets called from microtasks within runNextTimer).

Specifically, we write tests like

  test('thing gets handled', () => awaitFakeAsync((async) async {
    await scheduleThing(delay: const Duration(seconds: 1));
    async.elapse(const Duration(seconds: 1));
    checkThingWasHandled();
  }));

where awaitFakeAsync is the helper described in #84. So that async.elapse call comes from a microtask, fired by the flushTimers call in the implementation of awaitFakeAsync (with current fake_async), or the runNextTimer call in my draft revision of awaitFakeAsync (using this PR).

Fortunately I think runNextTimer's internal assumptions are perfectly safe with re-entrancy:

• After the first flushMicrotasks call, the only new fact it can assume is that the microtask queue is empty. And flushMicrotasks ensures that before it returns, regardless of what the microtasks do. So if a microtask calls runNextTimer, things are no different from just calling runNextTimer twice in succession.
• Conversely, on entering the flushMicrotasks call at the end, the only post-condition that remains for runNextTimer to establish is that the microtask queue is empty (as that flushMicrotasks call is the very last thing it does). If a microtask calls runNextTimer, that call will do its own flushMicrotasks, so it's just as if it had happened after the outer call finished.
• Finally, the same thing is true of when the timer callback gets fired, with that timer._fire() call just before flushMicrotasks…
  • … except that when the timer is periodic, FakeTimer._fire updates _nextCall only after calling the timer callback. That sounds like a re-entrancy bug, and indeed it is: #88. Starting to write this paragraph is what led me to discover that, and then I took a detour to confirm that it is and write up a PR to fix it (Fully update internal state before invoking periodic-timer callback #89).

Beyond runNextTimer itself, this library's two event loops that call it (in elapse and flushTimers) are also safe with re-entrancy, by the same reasoning that I give in #89 to argue that that fix is complete.

The possibility of re-entrancy does call for a bit of care, though, when looking at a call site of runNextTimer and thinking about its behavior. Here's a paragraph I'll add to the doc:

  /// If microtasks or timer callbacks make their own calls to methods on this
  /// [FakeAsync], then a call to this method may indirectly cause more timers
  /// to run beyond the timer it runs directly, and may cause [elapsed] to
  /// advance beyond [until].  Any such timers are ignored in the return value.

In terms of the use cases I put in the doc:

• to verify invariants between each timer event: For this use case, one would avoid calling runNextTimer except at the call site that will verify the invariants (and would avoid calling elapse and flushTimers entirely).
• to end the test early in case of an error (which is the use case I actually have): For this use case I think this wrinkle is perfectly fine. If the test body (e.g., the callback passed to awaitFakeAsync) has already completed with an error, then it won't be making any more FakeAsync method calls; and only the test should be seeing the FakeAsync instance in the first place.