fix(zone.js): always run microtasks in Promise.then to avoid drain microtasks too early.#45273
Closed
JiaLiPassion wants to merge 3 commits into
Closed
fix(zone.js): always run microtasks in Promise.then to avoid drain microtasks too early.#45273JiaLiPassion wants to merge 3 commits into
Promise.then to avoid drain microtasks too early.#45273JiaLiPassion wants to merge 3 commits into
Conversation
Promise.then to avoid drain microtasks too early.
290827e to
065ca21
Compare
065ca21 to
12837d1
Compare
Promise.then to avoid drain microtasks too early.Promise.then to avoid drain microtasks too early.
Promise.then to avoid drain microtasks too early.Promise.then to avoid drain microtasks too early.
288665d to
1217dda
Compare
devversion
approved these changes
Mar 14, 2022
Member
devversion
left a comment
There was a problem hiding this comment.
Seems reasonable to me, but definitely also breaky (in terms of apps relying on this synchronous draining currently). LGTM
Contributor
Contributor
|
Quick update after running a global presubmit: this PR breaks a number of targets (40), which would require additional investigation. Adding the "blocked" label for now. |
iconio
approved these changes
Mar 30, 2022
Merged
4 tasks
…crotasks too early
Always `drainMicrotaskQueue` in `Promise.then`.
Consider the following case.
```
const listener = () => {
Promise.resolve().then(() => console.log(`promise then`));
};
document.body.addEventListener('click', listener);
document.body.click();
console.log('main stack');
```
Without `zone.js` patch, the output should be
```
main stack
promise then
```
since `Promise.resolve().then()` should run as microtask after `main stack` finish.
But with `zone.js` patch, it becomes
```
promise then
main stack
```
The reason is currently `zone.js` maintain a `microtask queue` themselves, and after a `task` is invoked (in this case, the `click` eventTask), it will try to `drainMicrotaskQueue` synchronizely if the `task` is the `top task`.
This PR make sure that the `dranMicroTaskQueue` happen inside a Browser native microTask.
This new behavior is the correct behavior but it is very risky since it changes the microTask consuming timing and may impact a lot of existing apps.
Close angular#44446, angular#41506
In jasmine `onComplete()` callback, we used to schedule a new microTask to ensure there is a top task is running to avoid the early draining microtask behavior, since now zone.js always use a native Promise.then to drain the microtask queue, we don't need to do this any longer.
In the mocha runTest patch, we used to schedule a microtask to avoid drain microtask queue to early since we have a maunal top task, since zone now drain microtask queue in a native microtask Promise.then, we don't need to do this manual schedue any longer.
1217dda to
763d554
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
always
drainMicrotaskQueueinPromise.then.Consider the following case.
Without
zone.jspatch, the output should besince
Promise.resolve().then()should run as microtask aftermain stackfinish.But with
zone.jspatch, it becomesThe reason is currently
zone.jsmaintain amicrotask queuethemselves, and after ataskis invoked (in this case, theclickeventTask), it will try todrainMicrotaskQueuesynchronizely if thetaskis thetop task.This PR make sure that the
dranMicroTaskQueuehappen inside a Browser native microTask.This new behavior is the correct behavior but it is very risky since it changes the microTask consuming timing and may impact a lot of existing apps.
Close #44446, #41506