Make realAsyncZone run microtasks and timers in the correct zone.
#162731
Conversation
|
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 in Discord (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 the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
a: tests
| 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.
This could have just done Zone.root.scheduleMicrotask(zone.bindCallbackGuarded(f)), but that's not necessarily the same as what rootDelegate.scheduleMicrotask(zone, f) does.
The bindCallbackGuarded does an extra Zone.registerCalback, and an extra Zone.run on top of what the root zone's scheduleMicrotask would already do.
Using the root zone's delegate ensures that it does exactly the same as if zone 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 intended registerMicrotask and run behaviores.
|
test-exempt: unblocks future roll |
|
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. Click here 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, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
lrhn
force-pushed
the
fix-real-async-zone
branch
from
faa51aa to
baea359
Compare
|
Rebase on current master, hope it fixes golden file tests. |
lrhn
force-pushed
the
fix-real-async-zone
branch
3 times, most recently
from
ec49c95 to
b8d544d
Compare
|
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!) |
lrhn
force-pushed
the
fix-real-async-zone
branch
2 times, most recently
from
3654429 to
e2d301e
Compare
|
Looks like the goldens check is good now? Removing the label. |
lrhn
force-pushed
the
fix-real-async-zone
branch
from
e2d301e to
adb9107
Compare
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
lrhn
force-pushed
the
fix-real-async-zone
branch
from
adb9107 to
33762c7
Compare
Current implementation runs timers and microtask callbacks in the root zone. 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 ensure that 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 would do if its `ZoneDelegate` got called with the intended zone and arguments. That should be consistent with the current behavior, and be compatible with changes to the platform `Zone` behavior.
lrhn
force-pushed
the
fix-real-async-zone
branch
from
33762c7 to
63849a5
Compare
|
PTAL. I believe the goldens mark is a false positive. |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
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.
|
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.) |
…lutter#162731) Current implementation runs timers and microtask callbacks in the root zone. 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 ensure that 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 would do if its `ZoneDelegate` got called with the intended zone and arguments. That should be consistent with the current behavior, and be compatible with incoming bug-fixes to the platform `Zone` behavior. Prepares Flutter for landing https://dart-review.googlesource.com/c/sdk/+/406961 which is currently blocked (so this indirectly fixes dart-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.dart` test which threw the error-after-test in the root zone instead of the test zone. This change fixes that.
…lutter#162731) Current implementation runs timers and microtask callbacks in the root zone. 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 ensure that 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 would do if its `ZoneDelegate` got called with the intended zone and arguments. That should be consistent with the current behavior, and be compatible with incoming bug-fixes to the platform `Zone` behavior. Prepares Flutter for landing https://dart-review.googlesource.com/c/sdk/+/406961 which is currently blocked (so this indirectly fixes dart-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.dart` test which threw the error-after-test in the root zone instead of the test zone. This change fixes that.
4 participants
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 landing https://dart-review.googlesource.com/c/sdk/+/406961
which is currently blocked (so this indirectly fixes dart-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.