Add FakeAsync.runNextTimer #85

gnprice · 2024-06-06T03:05:01Z

Fixes #84.

This method is like flushTimers, but runs just one timer and then returns.
That allows the caller to write their own loop similar to flushTimers
but with custom logic of their own.

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

If we reach this line, `_timers.isNotEmpty` must be true. That's because this callback's only call site is the line `if(!predicate(timer)) break;` in `_fireTimersWhile`, and there's a check a few lines above there that would have broken out of the loop if `_timers` were empty.

This version is exactly equivalent via Boolean algebra, and will make for a bit simpler of a diff in the next refactor.

Fixes dart-lang#84.

lib/fake_async.dart

natebosch · 2024-06-28T00:04:11Z

I think this looks like a sensible API. I'll double check this doesn't impact existing internal usage. @lrhn any concerns?

lrhn · 2024-06-28T06:34:11Z

lib/fake_async.dart

-      if (timer._nextCall > absoluteTimeout) {
-        // TODO(nweiz): Make this a [TimeoutException].
-        throw StateError('Exceeded timeout $timeout while flushing timers');
+    for (;;) {


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

lrhn · 2024-06-28T06:51:10Z

lib/fake_async.dart

@@ -138,7 +138,7 @@ class FakeAsync {
    }

    _elapsingTo = _elapsed + duration;
-    _fireTimersWhile((next) => next._nextCall <= _elapsingTo!);
+    while (runNextTimer(timeout: _elapsingTo! - _elapsed)) {}


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

lrhn · 2024-06-28T07:07:03Z

lib/fake_async.dart

+  /// timer runs, [elapsed] is updated to the appropriate value.
+  ///
+  /// The [timeout] controls how much fake time may elapse. If non-null,
+  /// then timers further in the future than the given duration will be ignored.


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?

lrhn · 2024-06-28T09:33:31Z

lib/fake_async.dart

+      // 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


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

lrhn · 2024-06-28T09:41:05Z

lib/fake_async.dart

+  /// The [timeout] controls how much fake time may elapse. If non-null,
+  /// then timers further in the future than the given duration will be ignored.
+  ///
+  /// Returns true if a timer was run, false otherwise.


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.

lrhn · 2024-06-28T09:41:56Z

lib/fake_async.dart

+  ///
+  /// Returns true if a timer was run, false otherwise.
+  bool runNextTimer({Duration? timeout}) {
+    final absoluteTimeout = timeout == null ? null : _elapsed + timeout;


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

lrhn · 2024-06-28T09:42:27Z

lib/fake_async.dart

-    flushMicrotasks();
-    for (;;) {
-      if (_timers.isEmpty) break;
+  /// Microtasks are flushed before and after the timer runs. Before the


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

Sure, done.

lrhn

No problem with the API, but suggestions on structuring the implementation.
One change request: Make a reached timeout actually advance the time to that timeout time.

Consider suggesting use-cases for the method in its doc. Something like:

/// Running only one timer at a time, rather than just advancing time, 
/// can be used if a test wants to simulate other asynchronous non-timer events
/// between timer events, for example simulating UI updates
/// or port communication.

(Or something. That's what I could come up with.)

Maybe also say:

  /// Notice that after a call this method, the time may have been advanced 
  /// to where multiple timers are due. Doing an `elapse(Duration.zero)` afterwards
  /// may trigger more timers.

Before adding this method, the only way to advance time was elapse and flushTimers, which both always runs all due timers before exiting.
We should probably check that there is no code assuming that the timers in _timers are not yet due. (Probably not an issue, the code is pretty basic.)

(Should we have an isTimerDue check?)

natebosch · 2024-06-28T16:13:52Z

No impact to internal usage, so this is safe to land after review.

gnprice

Thanks for the review! Pushed a revised version; PTAL.

After making the internal helper you suggested at #85 (comment) that takes the actual end time rather than a duration relative to now, I decided I liked that better for the public interface too. (In particular that's the interface all the known use cases want — the two internal callers, and my external one.) So I changed to that. That's in its own commit near the end, though, so it's easy to revert if you prefer the other way.

As discussed under #85 (comment) , I like the behavior the timeout parameter had where it doesn't cause time to elapse if no timers run, but I take your point that the name doesn't match. In this revision, where it's in absolute time rather than relative to now, I renamed it to until. Other ideas are within or before (though the latter should make it a < instead of <= condition); or maybe you'll think of a better one.

We should probably check that there is no code assuming that the timers in _timers are not yet due. (Probably not an issue, the code is pretty basic.)

I took a skim through for this and didn't spot anything.

I believe this is also a situation that already gets created any time user code calls Timer.run, though. So that means everything is already required to be OK with it.

gnprice · 2024-07-02T03:27:56Z

lib/fake_async.dart

-      if (timer._nextCall > absoluteTimeout) {
-        // TODO(nweiz): Make this a [TimeoutException].
-        throw StateError('Exceeded timeout $timeout while flushing timers');
+    for (;;) {


Sure, changed.

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

gnprice · 2024-07-02T03:31:18Z

lib/fake_async.dart

+      // 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


gnprice · 2024-07-02T03:32:21Z

lib/fake_async.dart

+  /// The [timeout] controls how much fake time may elapse. If non-null,
+  /// then timers further in the future than the given duration will be ignored.
+  ///
+  /// Returns true if a timer was run, false otherwise.


Makes sense, thanks. Done.

gnprice · 2024-07-02T03:39:13Z

lib/fake_async.dart

-    flushMicrotasks();
-    for (;;) {
-      if (_timers.isEmpty) break;
+  /// Microtasks are flushed before and after the timer runs. Before the


Sure, done.

gnprice · 2024-07-02T03:54:24Z

lib/fake_async.dart

+  /// timer runs, [elapsed] is updated to the appropriate value.
+  ///
+  /// The [timeout] controls how much fake time may elapse. If non-null,
+  /// then timers further in the future than the given duration will be ignored.


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?

gnprice · 2024-07-02T04:15:07Z

lib/fake_async.dart

@@ -138,7 +138,7 @@ class FakeAsync {
    }

    _elapsingTo = _elapsed + duration;
-    _fireTimersWhile((next) => next._nextCall <= _elapsingTo!);
+    while (runNextTimer(timeout: _elapsingTo! - _elapsed)) {}


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

gnprice · 2024-07-02T05:50:32Z

lib/fake_async.dart

+  /// 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.


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

gnprice · 2024-10-02T03:29:55Z

Ping. :-)

I just ran into another debugging situation where I needed this functionality (for the reasons in #84), and installed fake_async from this PR branch in order to get it. It'd be great to get it merged.

lrhn

Initial comment. Have to read this again to figure out how it works.

lrhn · 2024-10-03T14:27:37Z

lib/fake_async.dart

-      if (timer._nextCall > absoluteTimeout) {
-        // TODO(nweiz): Make this a [TimeoutException].
-        throw StateError('Exceeded timeout $timeout while flushing timers');
+    while (true) {


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.

lrhn · 2024-10-03T14:29:36Z

lib/fake_async.dart

+  /// 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.


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

lrhn · 2024-10-03T14:31:03Z

lib/fake_async.dart

+  /// Doing an `elapse(Duration.zero)` afterwards may trigger more timers.
+  ///
+  /// Returns `true` if a timer was run, `false` otherwise.
+  bool runNextTimer({Duration? until}) {


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:

elapse inside periodic timer immediately calls the same timer #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: elapse inside periodic timer immediately calls the same timer #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.

gnprice added 3 commits June 5, 2024 18:26

Rearrange logic around flushPeriodicTimers

fb5f59c

This version is exactly equivalent via Boolean algebra, and will make for a bit simpler of a diff in the next refactor.

Add FakeAsync.runNextTimer

a78ba91

Fixes dart-lang#84.

gnprice mentioned this pull request Jun 6, 2024

Single-step version of flushTimers #84

Open

natebosch self-requested a review June 27, 2024 22:59

natebosch reviewed Jun 27, 2024

View reviewed changes

lib/fake_async.dart Outdated Show resolved Hide resolved

natebosch requested a review from lrhn June 28, 2024 00:03

lrhn reviewed Jun 28, 2024

View reviewed changes

gnprice added 6 commits July 1, 2024 20:58

Apply several small comments

4916d20

Internal helper taking absolute time

ae309c9

Rename from "timeout" to "within"

158b5d8

Add more discussion in docs

02188dc

Take an absolute time bound, rather than relative

4fd4365

Add test cases on not updating elapsed

6227b63

gnprice commented Jul 2, 2024

View reviewed changes

natebosch requested a review from lrhn October 2, 2024 22:40

lrhn reviewed Oct 3, 2024

View reviewed changes

Document possible re-entrancy on runNextTimer

6fb6a8c

gnprice mentioned this pull request Oct 4, 2024

elapse inside periodic timer immediately calls the same timer #88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FakeAsync.runNextTimer #85

Add FakeAsync.runNextTimer #85

gnprice commented Jun 6, 2024

natebosch commented Jun 28, 2024

lrhn Jun 28, 2024

gnprice Jul 2, 2024

lrhn Jun 28, 2024

gnprice Jul 2, 2024

lrhn Jun 28, 2024

gnprice Jul 2, 2024

lrhn Jun 28, 2024

gnprice Jul 2, 2024

lrhn Jun 28, 2024

gnprice Jul 2, 2024

lrhn Jun 28, 2024

lrhn Jun 28, 2024

gnprice Jul 2, 2024

lrhn left a comment

natebosch commented Jun 28, 2024

gnprice left a comment •

edited

Loading

gnprice Jul 2, 2024

gnprice Jul 2, 2024

gnprice Jul 2, 2024

gnprice Jul 2, 2024

gnprice Jul 2, 2024

gnprice Jul 2, 2024

gnprice Jul 2, 2024

lrhn Oct 3, 2024

gnprice commented Oct 2, 2024

lrhn left a comment

lrhn Oct 3, 2024

gnprice Oct 3, 2024

lrhn Oct 4, 2024

lrhn Oct 3, 2024

lrhn Oct 3, 2024

gnprice Oct 4, 2024

Add FakeAsync.runNextTimer #85

Are you sure you want to change the base?

Add FakeAsync.runNextTimer #85

Conversation

gnprice commented Jun 6, 2024

natebosch commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrhn left a comment

Choose a reason for hiding this comment

natebosch commented Jun 28, 2024

gnprice left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Oct 2, 2024

lrhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment •

edited

Loading