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.

Sign up for GitHub

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

Jump to bottom

Make zone handlers call zone.run to run callbacks. #2451

Merged
merged 4 commits into from
Jan 27, 2025
Merged

Make zone handlers call zone.run to run callbacks. #2451

merged 4 commits into from
Jan 27, 2025

Conversation

lrhn
Copy link
Member

@lrhn lrhn commented Jan 24, 2025

The platform library Zone implementation will change to not use bindCallbackGuarded in the Timer constructors and top-level scheduleMicrotask function.
Instead it only calls registerCallback, and then the Zone.create{,Periodic}Timer and Zone.scheduleMicrotask are in charge of using Zone.runGuarded on the callback when timer/microtask event happens.

This ensures that a surrounding zone's registerCallback can't make the callback throw in a way that is outside of the runGuarded call.

Also makes periodic timers that are delayed past multiple tick points, update their tick count by more than one instead of running multiple times, like timers should.

Adds testing of using the FakeAsync's Zone functions directly, and embedding it in another zone to check that it does run using Zone.run.
Adds testing of periodic timers incrementing tick by more than one.

@lrhn
Make zone handlers call zone.run to run callbacks.
9fa57d1 
The platform library `Zone` implementation will change to
not use `bindCallbackGuarded` in the `Timer` constructors
and top-level `scheduleMicrotask` function.
Instead it only calls `registerCallback`, and then the
`Zone.create{,Periodic}Timer` and `Zone.scheduleMicrotask`
are in charge of using `Zone.runGuarded` on the callback
when timer/microtask event happens.

This ensures that a surrounding zone's `registerCallback`
can't make the callback throw in a way that is outside
of the `runGuarded` call.
@lrhn lrhn requested a review from a team as a code owner January 24, 2025 13:53
@github-actions GitHub Actions
Copy link

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

lrhn
lrhn commented Jan 24, 2025
View reviewed changes
pkgs/fake_async/CHANGELOG.md
## 1.3.2

* Require Dart 3.3
* Fix bug where a `flushTimers` or `elapse` call from within
the callback of a periodic timer would immediately invoke
the same timer.
* Move to `dart-lang/test` monorepo.
* Require Dart 3.5.
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't actually change pubspec.yaml.
(Only one of the first and last line here is correct and needed.)

lrhn
lrhn commented Jan 24, 2025
View reviewed changes
pkgs/fake_async/lib/fake_async.dart
final absoluteTimeout = _elapsed + timeout;
_fireTimersWhile((timer) {
if (timer._nextCall > absoluteTimeout) {
// TODO(nweiz): Make this a [TimeoutException].
throw StateError('Exceeded timeout $timeout while flushing timers');
}

if (flushPeriodicTimers) return _timers.isNotEmpty;
Copy link
Member Author

Choose a reason for hiding this comment

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

The list is never empty here, it always contains at least timer, which is never removed until it is _fired.

lrhn
lrhn commented Jan 24, 2025
View reviewed changes
pkgs/fake_async/lib/fake_async.dart
// (and advancing time to their next due time)
// if flushing periodic timers,
// or if there is any non-periodic timer left.
return flushPeriodicTimers || _timers.any((timer) => !timer.isPeriodic);
Copy link
Member Author

Choose a reason for hiding this comment

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

This function should just check whether to run the current timer, which it should unless it's not due, we're not flushing periodic timers, and all remaining timers (including this one) are periodic timers.

lrhn
lrhn commented Jan 24, 2025
View reviewed changes
pkgs/fake_async/test/fake_async_test.dart
..run((_) {
// Do not use [neverCalled] from package:test.
// It schedules timers to "pump the event loop",
// which stalls the test if you don't call `elapse` or `flushTimers`.
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried, it did not go well.

lrhn
lrhn commented Jan 24, 2025
View reviewed changes
pkgs/fake_async/test/fake_async_test.dart
@@ -27,9 +27,18 @@ void main() {
});

group('elapseBlocking', () {
test('should elapse time without calling timers', () {
Timer(elapseBy ~/ 2, neverCalled);
Copy link
Member Author

Choose a reason for hiding this comment

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

This timer is created outside of a FakeAsync, to run in 12 hours.
Lucky that the test runner kills the process when all tests are successfully completed..

@lrhn lrhn closed this Jan 24, 2025
@lrhn lrhn reopened this Jan 24, 2025
@lrhn lrhn merged commit 92eed6c into master Jan 27, 2025
126 checks passed
@lrhn lrhn deleted the zoned branch January 27, 2025 13:05
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jan 27, 2025
@devoncarew
[deps] rev core, ecosystem, test, webdev
732fa5e 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

core (https://github.com/dart-lang/core/compare/72a2060..3b35aca):
  3b35aca9  2025-01-24  Devon Carew  update the topics section of several pubspecs (dart-lang/core#854)

ecosystem (https://github.com/dart-lang/ecosystem/compare/682c8ef..c4a163a):
  c4a163a  2025-01-23  Devon Carew  print detailed info about the leaked APIs to stdout in the workflow (dart-lang/ecosystem#339)

test (https://github.com/dart-lang/test/compare/7fc9521..92eed6c):
  92eed6c4  2025-01-27  Lasse R.H. Nielsen  Make zone handlers call zone.run to run callbacks. (dart-lang/test#2451)
  ad04f172  2025-01-24  Devon Carew  add a pull request labeler; add issue tracker links; update codeowners file (dart-lang/test#2452)
  1f292db8  2025-01-24  Lasse R.H. Nielsen  Don't require Dart 3.5.
  a56fa947  2025-01-24  Lasse R.H. Nielsen  Accept extra `run`s in test. Move to Dart 3.5.
  4bd54972  2025-01-24  Lasse R.H. Nielsen  Clean-up. Accidentally merged a removed lint back in.
  9fa57d16  2025-01-24  Lasse R.H. Nielsen  Make zone handlers call zone.run to run callbacks.

webdev (https://github.com/dart-lang/webdev/compare/9430dd7..5dc7950):
  5dc79508  2025-01-24  Jessy Yameogo  Added support for callLibraryMethod with the DDC library bundle format.  (dart-lang/webdev#2577)
  1d0b32c2  2025-01-24  Srujan Gaddam  Reset DWDS to 24.3.4-wip (dart-lang/webdev#2580)
  1990d29a  2025-01-23  Srujan Gaddam  Implement hot restart for DDC library bundle format and publish 24.3.3 (dart-lang/webdev#2576)

Change-Id: Ifa23e74a1399ac7ec70a99750419b915a2efbb19
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/406120
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
Auto-Submit: Devon Carew <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natebosch natebosch natebosch approved these changes

Assignees
No one assigned
Labels
package:fake_async
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lrhn @natebosch @devoncarew