Skip to content
This repository has been archived by the owner on Oct 18, 2024. It is now read-only.

Add FakeAsync.runNextTimer #85

Closed
wants to merge 11 commits into from
Closed

Add FakeAsync.runNextTimer #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b22f6e4
Simplify flushPeriodicTimers logic
gnprice Jun 6, 2024
fb5f59c
Rearrange logic around flushPeriodicTimers
gnprice Jun 6, 2024
a78ba91
Add FakeAsync.runNextTimer
gnprice Jun 6, 2024
4916d20
Apply several small comments
gnprice Jul 2, 2024
ae309c9
Internal helper taking absolute time
gnprice Jul 2, 2024
158b5d8
Rename from "timeout" to "within"
gnprice Jul 2, 2024
02188dc
Add more discussion in docs
gnprice Jul 2, 2024
4fd4365
Take an absolute time bound, rather than relative
gnprice Jul 2, 2024
6227b63
Add test cases on not updating elapsed
gnprice Jul 2, 2024
6fb6a8c
Document possible re-entrancy on runNextTimer
gnprice Oct 4, 2024
48f268f
Merge remote-tracking branch 'origin'
gnprice Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## 1.3.2-wip
## 1.4.0-wip

* Add `FakeAsync.runNextTimer`, a single-step analogue
of `FakeAsync.flushTimers`.
* Require Dart 3.3
* Fix bug where a `flushTimers` or `elapse` call from within
the callback of a periodic timer would immediately invoke
Expand Down
74 changes: 49 additions & 25 deletions lib/fake_async.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class FakeAsync {
}

_elapsingTo = _elapsed + duration;
_fireTimersWhile((next) => next._nextCall <= _elapsingTo!);
while (runNextTimer(until: _elapsingTo!)) {}
_elapseTo(_elapsingTo!);
_elapsingTo = null;
}
Expand Down Expand Up @@ -211,39 +211,63 @@ class FakeAsync {
{Duration timeout = const Duration(hours: 1),
bool flushPeriodicTimers = true}) {
final absoluteTimeout = _elapsed + timeout;
_fireTimersWhile((timer) {
if (timer._nextCall > absoluteTimeout) {
// TODO(nweiz): Make this a [TimeoutException].
throw StateError('Exceeded timeout $timeout while flushing timers');
while (true) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// With [flushPeriodicTimers] false, continue firing timers only until
// all remaining timers are periodic *and* every periodic timer has had
// a chance to run against the final value of [_elapsed].
if (!flushPeriodicTimers) {
if (_timers
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

.every((timer) => timer.isPeriodic && timer._nextCall > _elapsed)) {
break;
}
}

if (flushPeriodicTimers) return _timers.isNotEmpty;
if (!runNextTimer(until: absoluteTimeout)) {
if (_timers.isEmpty) break;

// Continue firing timers until the only ones left are periodic *and*
// every periodic timer has had a change to run against the final
// value of [_elapsed].
return _timers
.any((timer) => !timer.isPeriodic || timer._nextCall <= _elapsed);
});
// TODO(nweiz): Make this a [TimeoutException].
throw StateError('Exceeded timeout $timeout while flushing timers');
}
}
}

/// Invoke the callback for each timer until [predicate] returns `false` for
/// the next timer that would be fired.
/// Elapses time to run one timer, if any timer exists.
///
/// Running one timer at a time, rather than just advancing time such as
/// with [elapse] or [flushTimers], can be useful if a test wants to run
/// its own logic between each timer event, for example to verify invariants
/// or to end the test early in case of an error.
Comment on lines +236 to +239
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

///
/// Microtasks are flushed before and after each timer is fired. Before each
/// timer fires, [_elapsed] is updated to the appropriate duration.
void _fireTimersWhile(bool Function(FakeTimer timer) predicate) {
/// Microtasks are flushed before identifying the timer to run,
/// and again after the timer runs.
/// Before the timer runs, [elapsed] is updated to the appropriate value.
///
/// When [until] is non-null, only timers due up until the given time
/// will be considered, in terms of [elapsed].
///
/// Because multiple timers may be due at the same time, a call to this
/// method may leave the time advanced to where other timers are due.
/// Calling `elapse(Duration.zero)` afterwards may trigger more timers.
///
/// 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.
///
/// Returns `true` if a timer was run, `false` otherwise.
bool runNextTimer({Duration? until}) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

flushMicrotasks();
for (;;) {
if (_timers.isEmpty) break;

final timer = minBy(_timers, (FakeTimer timer) => timer._nextCall)!;
if (!predicate(timer)) break;

_elapseTo(timer._nextCall);
timer._fire();
flushMicrotasks();
if (_timers.isEmpty) return false;
final timer = minBy(_timers, (FakeTimer timer) => timer._nextCall)!;
if (until != null && timer._nextCall > until) {
return false;
}

_elapseTo(timer._nextCall);
timer._fire();
flushMicrotasks();
return true;
}

/// Creates a new timer controlled by `this` that fires [callback] after
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: fake_async
version: 1.3.2-wip
version: 1.4.0-wip
description: >-
Fake asynchronous events such as timers and microtasks for deterministic
testing.
Expand Down
117 changes: 117 additions & 0 deletions test/fake_async_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,123 @@ void main() {
});
});

group('runNextTimer', () {
test('should run the earliest timer', () {
FakeAsync().run((async) {
var last = 0;
Timer(const Duration(days: 2), () => last = 2);
Timer(const Duration(days: 1), () => last = 1);
Timer(const Duration(days: 3), () => last = 3);
expect(async.runNextTimer(), true);
expect(last, 1);
});
});

test('should return false if no timers exist', () {
FakeAsync().run((async) {
expect(async.runNextTimer(), false);
});
});

test('should run microtasks before choosing timer', () {
FakeAsync().run((async) {
var last = 0;
Timer(const Duration(days: 2), () => last = 2);
scheduleMicrotask(() => Timer(const Duration(days: 1), () => last = 1));
expect(async.runNextTimer(), true);
expect(last, 1);
expect(async.runNextTimer(), true);
expect(last, 2);
});
});

test('should run microtasks before deciding no timers exist', () {
FakeAsync().run((async) {
var last = 0;
scheduleMicrotask(() => Timer(const Duration(days: 1), () => last = 1));
expect(async.runNextTimer(), true);
expect(last, 1);
});
});

test('should run microtasks after timer', () {
FakeAsync().run((async) {
var ran = false;
Timer.run(() => scheduleMicrotask(() => ran = true));
expect(async.runNextTimer(), true);
expect(ran, true);
});
});

test('should update elapsed before running timer', () {
FakeAsync().run((async) {
Duration? time;
Timer(const Duration(days: 1), () => time = async.elapsed);
expect(async.runNextTimer(), true);
expect(time, const Duration(days: 1));
expect(async.elapsed, const Duration(days: 1));
});
});

test('should not update elapsed if no timers exist', () {
FakeAsync().run((async) {
async.elapse(const Duration(hours: 1));
expect(async.runNextTimer(), false);
expect(async.elapsed, const Duration(hours: 1));
});
});

test('should apply `until`', () {
FakeAsync().run((async) {
var ran = false;
Timer(const Duration(days: 1), () => ran = true);
expect(async.runNextTimer(until: const Duration(hours: 1)), false);
expect(ran, false);
});
});

test('should not update elapsed if all timers are past `until`', () {
FakeAsync().run((async) {
Timer(const Duration(days: 1), () {});
expect(async.runNextTimer(until: const Duration(hours: 1)), false);
expect(async.elapsed, Duration.zero);
});
});

test('should apply `until` as non-strict bound', () {
FakeAsync().run((async) {
var ran = false;
Timer(const Duration(hours: 1), () => ran = true);
expect(async.runNextTimer(until: const Duration(hours: 1)), true);
expect(ran, true);
});
});

test('should apply `until` relative to start', () {
FakeAsync().run((async) {
var ran = false;
Timer(const Duration(hours: 3), () => ran = true);
async.elapse(const Duration(hours: 1));

expect(async.runNextTimer(until: const Duration(hours: 2)), false);
expect(ran, false);
expect(async.elapsed, const Duration(hours: 1));

expect(async.runNextTimer(until: const Duration(hours: 3)), true);
expect(ran, true);
});
});

test('should have no time bound by default', () {
FakeAsync().run((async) {
var ran = false;
Timer(const Duration(microseconds: 1 << 52), () => ran = true);
expect(async.runNextTimer(), true);
expect(ran, true);
});
});
});

group('stats', () {
test('should report the number of pending microtasks', () {
FakeAsync().run((async) {
Expand Down