-
Notifications
You must be signed in to change notification settings - Fork 16
Add FakeAsync.runNextTimer #85
Changes from 3 commits
b22f6e4
fb5f59c
a78ba91
4916d20
ae309c9
158b5d8
02188dc
4fd4365
6227b63
6fb6a8c
48f268f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,7 +138,7 @@ class FakeAsync { | |
} | ||
|
||
_elapsingTo = _elapsed + duration; | ||
_fireTimersWhile((next) => next._nextCall <= _elapsingTo!); | ||
while (runNextTimer(timeout: _elapsingTo! - _elapsed)) {} | ||
_elapseTo(_elapsingTo!); | ||
_elapsingTo = null; | ||
} | ||
|
@@ -211,39 +211,50 @@ 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'); | ||
for (;;) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, changed. (That's usually what I'd write too; the |
||
// 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(timeout: absoluteTimeout - _elapsed)) { | ||
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. | ||
/// | ||
/// 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) { | ||
flushMicrotasks(); | ||
for (;;) { | ||
if (_timers.isEmpty) break; | ||
/// Microtasks are flushed before and after the timer runs. Before the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mention that the current microtask queue is flushed whether or not there is a timer in range. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, done. |
||
/// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would still advance the time by I can see that If we change the call in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 (The So I'd prefer to find a better name for this parameter, rather than change the behavior to match the |
||
/// | ||
/// Returns true if a timer was run, false otherwise. | ||
natebosch marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd code-quote There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, thanks. Done. |
||
bool runNextTimer({Duration? timeout}) { | ||
final absoluteTimeout = timeout == null ? null : _elapsed + timeout; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is, I'd start the internal |
||
|
||
final timer = minBy(_timers, (FakeTimer timer) => timer._nextCall)!; | ||
if (!predicate(timer)) break; | ||
flushMicrotasks(); | ||
|
||
_elapseTo(timer._nextCall); | ||
timer._fire(); | ||
flushMicrotasks(); | ||
if (_timers.isEmpty) return false; | ||
final timer = minBy(_timers, (FakeTimer timer) => timer._nextCall)!; | ||
if (absoluteTimeout != null && timer._nextCall > absoluteTimeout) { | ||
return false; | ||
} | ||
|
||
_elapseTo(timer._nextCall); | ||
timer._fire(); | ||
flushMicrotasks(); | ||
return true; | ||
} | ||
|
||
/// Creates a new timer controlled by `this` that fires [callback] after | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 sinceinitialTime
).That's the time (
_elapsingTo
) that you already have here, and it's the first thing thatrunNextTimer
computes internally anyway.It avoids repeatedly doing
_elapsingTo - _elapsed
.So:
Then the public function would be:
(I suggest advancing time by at least the [timeout]. If not, it should be renamed to something else, like
before
. Atimeout
only applies if time has actually advanced.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)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
: