- Notifications
You must be signed in to change notification settings - Fork30k
MakerealAsyncZone run microtasks and timers in the correct zone.#162731
MakerealAsyncZone run microtasks and timers in the correct zone.#162731auto-submit[bot] merged 2 commits intoflutter:masterfrom
realAsyncZone run microtasks and timers in the correct zone.#162731Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel inDiscord (don't just cc them here, they won't see it!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself,is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read theTree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, soall reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
| specification: ZoneSpecification( | ||
| scheduleMicrotask: (Zone self, ZoneDelegate parent, Zone zone, void Function() f) { | ||
| Zone.root.scheduleMicrotask(f); | ||
| _rootDelegate.scheduleMicrotask(zone,f); |
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.
This could have just doneZone.root.scheduleMicrotask(zone.bindCallbackGuarded(f)), but that's not necessarily the same as whatrootDelegate.scheduleMicrotask(zone, f) does.
ThebindCallbackGuarded does an extraZone.registerCalback, and an extraZone.run on top of what the root zone'sscheduleMicrotask would already do.
Using the root zone's delegate ensures that it does exactly the same as ifzone had been a direct child of the root zone, just bypassing the intermediate zones in order to get "real" async operations.
It's a "more correct" approach to bypassing the surrounding zone's overloads of microtasks and timers, while still using its intendedregisterMicrotask andrun behaviores.
stuartmorgan-g commentedFeb 6, 2025
test-exempt: unblocks future roll |
lrhn commentedFeb 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Come to think of it, I should be able to add a test that distinguishes the new behavior from the old if I call the Done. Test added. |
Golden file changes have been found for this pull request. Clickhere to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visitWriting a golden file test for Reviewers: Read theTree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
lrhn commentedFeb 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Rebase on current master, hope it fixes golden file tests. |
ec49c95 tob8d544dComparelrhn commentedFeb 13, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm not sure this actually affects goldens (and the triage link is dead). What is the next step needed to land this? (This question still applies!) |
3654429 toe2d301eComparejustinmc commentedFeb 18, 2025
Looks like the goldens check is good now? Removing the label. |
Golden file changes are available for triage from new commit, Clickhere to view. For more guidance, visitWriting a golden file test for Reviewers: Read theTree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Current implementation runs timers and microtask callbacks in the root zone.That assumes that the top-level `scheduleMicrotask` or `Timer` constructorshave been used, which have so far wrapped the callback with `runCallbackGuarded`before calling the zone implementation.That means that doing `zone.scheduleMicrotask` directly would not ensurethat the microtask was run in the correct zone. If a `run` handler throws,it wouldn't be caught.This change makes the `realAsyncZone` do whatever the root zone woulddo if its `ZoneDelegate` got called with the intended zone and arguments.That should be consistent with the current behavior, and be compatiblewith changes to the platform `Zone` behavior.
lrhn commentedMar 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
PTAL. I believe the goldens mark is a false positive. |
justinmc left a comment
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.
LGTM from my point of view but I am not familiar with these async zones in the tests. Looks like the goldens check is passing now though.
lrhn commentedMar 19, 2025
Thanks. I'll need someone to land the change too. (If it's someone is familiar with the zones, they can look at it too.) |
jonahwilliams left a comment
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.
LGTM
…lutter#162731)Current implementation runs timers and microtask callbacks in the rootzone. That assumes that the top-level `scheduleMicrotask` or `Timer`constructors have been used, which have so far wrapped the callback with`runCallbackGuarded` before calling the zone implementation.That means that doing `zone.scheduleMicrotask` directly would not ensurethat the microtask was run in the correct zone. If a `run` handlerthrows, it wouldn't be caught.This change makes the `realAsyncZone` do whatever the root zone would doif its `ZoneDelegate` got called with the intended zone and arguments.That should be consistent with the current behavior, and be compatiblewith incoming bug-fixes to the platform `Zone` behavior.Prepares Flutter for landinghttps://dart-review.googlesource.com/c/sdk/+/406961which is currently blocked (so this indirectlyfixesdart-lang/sdk#59913).There are no new tests, the goal is that all existing tests keeprunning, and that they keep doing so when the Dart CL lands. Currentlythat CL only breaks one test, the`dev/automated_tests/test_smoke_test/fail_test_on_exception_after_test.dart`test which threw the error-after-test in the root zone instead of thetest zone. This change fixes that.
…lutter#162731)Current implementation runs timers and microtask callbacks in the rootzone. That assumes that the top-level `scheduleMicrotask` or `Timer`constructors have been used, which have so far wrapped the callback with`runCallbackGuarded` before calling the zone implementation.That means that doing `zone.scheduleMicrotask` directly would not ensurethat the microtask was run in the correct zone. If a `run` handlerthrows, it wouldn't be caught.This change makes the `realAsyncZone` do whatever the root zone would doif its `ZoneDelegate` got called with the intended zone and arguments.That should be consistent with the current behavior, and be compatiblewith incoming bug-fixes to the platform `Zone` behavior.Prepares Flutter for landinghttps://dart-review.googlesource.com/c/sdk/+/406961which is currently blocked (so this indirectlyfixesdart-lang/sdk#59913).There are no new tests, the goal is that all existing tests keeprunning, and that they keep doing so when the Dart CL lands. Currentlythat CL only breaks one test, the`dev/automated_tests/test_smoke_test/fail_test_on_exception_after_test.dart`test which threw the error-after-test in the root zone instead of thetest zone. This change fixes that.
Uh oh!
There was an error while loading.Please reload this page.
Current implementation runs timers and microtask callbacks in the root zone. That assumes that the top-level
scheduleMicrotaskorTimerconstructors have been used, which have so far wrapped the callback withrunCallbackGuardedbefore calling the zone implementation.That means that doing
zone.scheduleMicrotaskdirectly would not ensure that the microtask was run in the correct zone. If arunhandler throws, it wouldn't be caught.This change makes the
realAsyncZonedo whatever the root zone would do if itsZoneDelegategot called with the intended zone and arguments. That should be consistent with the current behavior, and be compatible with incoming bug-fixes to the platformZonebehavior.Prepares Flutter for landinghttps://dart-review.googlesource.com/c/sdk/+/406961
which is currently blocked (so this indirectlyfixesdart-lang/sdk#59913).
There are no new tests, the goal is that all existing tests keep running, and that they keep doing so when the Dart CL lands. Currently that CL only breaks one test, the
dev/automated_tests/test_smoke_test/fail_test_on_exception_after_test.darttest which threw the error-after-test in the root zone instead of the test zone. This change fixes that.