Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

gnprice
Copy link
Contributor

@gnprice gnprice commented Oct 4, 2024

Fixes #88.

Currently when a periodic timer's callback gets invoked, its _nextCall is still the time of the current call, not the next one. If the timer callback itself calls flushTimers or elapse, this causes the same timer to immediately get called again.

Fortunately the fix is easy: update _nextCall just before invoking _callback, instead of just after.

To work through why this is a complete fix (and doesn't leave further bugs of this kind still to be fixed):

After this fix, the call to the timer's callback is a tail call from FakeTimer._fire. Because the call site of FakeTimer._fire is immediately followed by flushMicrotasks(), this means calling other FakeAsync methods from the timer callback is no different from doing so in a subsequent microtask.

Moreover, when running timers from flushTimers, if after the flushMicrotasks call this turns out to be the last timer to run, then flushTimers will return with no further updates to the state. So when the timer callback is invoked (in that case), the whole FakeAsync state must already be in a state that flushTimers would have been happy to leave it in. (And there's no special cleanup that it does only after a non-last timer.)

Similarly, when running timers from elapse (the only other possibility), the only difference from a state that elapse would be happy to leave things in is that _elapsingTo is still set. That field affects only elapse and elapseBlocking; and those are both designed to handle being called from within elapse.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

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

Fixes dart-lang#88.

Currently when a periodic timer's callback gets invoked, its
`_nextCall` is still the time of the current call, not the next one.
If the timer callback itself calls `flushTimers` or `elapse`, this
causes the same timer to immediately get called again.

Fortunately the fix is easy: update `_nextCall` just before invoking
`_callback`, instead of just after.

---

To work through why this is a complete fix (and doesn't leave
further bugs of this kind still to be fixed):

After this fix, the call to the timer's callback is a tail call from
`FakeTimer._fire`.  Because the call site of `FakeTimer._fire` is
immediately followed by `flushMicrotasks()`, this means calling
other `FakeAsync` methods from the timer callback is no different
from doing so in a subsequent microtask.

Moreover, when running timers from `flushTimers`, if after the
`flushMicrotasks` call this turns out to be the last timer to run,
then `flushTimers` will return with no further updates to the state.
So when the timer callback is invoked (in that case), the whole
`FakeAsync` state must already be in a state that `flushTimers`
would have been happy to leave it in.  (And there's no special
cleanup that it does only after a non-last timer.)

Similarly, when running timers from `elapse` (the only other
possibility), the only difference from a state that `elapse` would
be happy to leave things in is that `_elapsingTo` is still set.
That field affects only `elapse` and `elapseBlocking`; and those
are both designed to handle being called from within `elapse`.
@gnprice gnprice mentioned this pull request Oct 4, 2024
1 task
Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

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

@natebosch Any comments?

@natebosch natebosch merged commit 62fa2d9 into dart-lang:master Oct 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

elapse inside periodic timer immediately calls the same timer
3 participants