-
Notifications
You must be signed in to change notification settings - Fork 27.3k
fix(zone.js): allow draining microtasks in Promise.then (through flag)
#68134
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1131,8 +1131,6 @@ export function initZone(): ZoneType { | |
| 'eventTask': 0, | ||
| }; | ||
|
|
||
| private _parentDelegate: _ZoneDelegate | null; | ||
|
|
||
| private _forkDlgt: _ZoneDelegate | null; | ||
| private _forkZS: ZoneSpec | null; | ||
| private _forkCurrZone: Zone | null; | ||
|
|
@@ -1168,7 +1166,6 @@ export function initZone(): ZoneType { | |
|
|
||
| constructor(zone: Zone, parentDelegate: _ZoneDelegate | null, zoneSpec: ZoneSpec | null) { | ||
| this._zone = zone as ZoneImpl; | ||
| this._parentDelegate = parentDelegate; | ||
|
|
||
| this._forkZS = zoneSpec && (zoneSpec && zoneSpec.onFork ? zoneSpec : parentDelegate!._forkZS); | ||
| this._forkDlgt = zoneSpec && (zoneSpec.onFork ? parentDelegate : parentDelegate!._forkDlgt); | ||
|
|
@@ -1438,8 +1435,8 @@ export function initZone(): ZoneType { | |
| task.runCount++; | ||
| return task.zone.runTask(task, target, args); | ||
| } finally { | ||
| if (_numberOfNestedTaskFrames == 1) { | ||
| drainMicroTaskQueue(); | ||
| if (_numberOfNestedTaskFrames === 1 && !global[enableNativeMicrotaskDraining]) { | ||
| drainMicroTaskQueueSynchronously(); | ||
| } | ||
| _numberOfNestedTaskFrames--; | ||
| } | ||
|
|
@@ -1503,54 +1500,75 @@ export function initZone(): ZoneType { | |
| const symbolSetTimeout = __symbol__('setTimeout'); | ||
| const symbolPromise = __symbol__('Promise'); | ||
| const symbolThen = __symbol__('then'); | ||
| // To prevent any breaking changes resulting from this change, given that | ||
| // it was already causing a significant number of failures in g3, we have hidden | ||
| // that behavior behind a global configuration flag. Consumers can enable this | ||
| // flag explicitly if they want the microtask queue to be drained as defined | ||
| // in the specification. | ||
| const enableNativeMicrotaskDraining = __symbol__('enable_native_microtask_draining'); | ||
| let _microTaskQueue: Task[] = []; | ||
| let _isDrainingMicrotaskQueue: boolean = false; | ||
| let _isDrainingMicrotaskQueue = false; | ||
| let nativeMicroTaskQueuePromise: any; | ||
|
|
||
| function nativeScheduleMicroTask(func: Function) { | ||
| if (!nativeMicroTaskQueuePromise) { | ||
| if (global[symbolPromise]) { | ||
| nativeMicroTaskQueuePromise = global[symbolPromise].resolve(0); | ||
| } | ||
| if (!nativeMicroTaskQueuePromise && global[symbolPromise]) { | ||
| nativeMicroTaskQueuePromise = global[symbolPromise].resolve(0); | ||
| } | ||
|
|
||
| if (nativeMicroTaskQueuePromise) { | ||
| let nativeThen = nativeMicroTaskQueuePromise[symbolThen]; | ||
| if (!nativeThen) { | ||
| // native Promise is not patchable, we need to use `then` directly | ||
| // issue 1078 | ||
| nativeThen = nativeMicroTaskQueuePromise['then']; | ||
| } | ||
| nativeThen.call(nativeMicroTaskQueuePromise, func); | ||
| const thenFn = nativeMicroTaskQueuePromise[symbolThen] ?? nativeMicroTaskQueuePromise['then']; // fallback for non-patchable Promise | ||
| // Use the resolved native promise to schedule the microtask | ||
| thenFn.call(nativeMicroTaskQueuePromise, func); | ||
| } else { | ||
| // Fallback to setTimeout if native promise is unavailable | ||
| global[symbolSetTimeout](func, 0); | ||
| } | ||
| } | ||
|
|
||
| function scheduleMicroTask(task?: MicroTask) { | ||
| // if we are not running in any task, and there has not been anything scheduled | ||
| // we must bootstrap the initial task creation by manually scheduling the drain | ||
| if (_numberOfNestedTaskFrames === 0 && _microTaskQueue.length === 0) { | ||
| // We are not running in Task, so we need to kickstart the microtask queue. | ||
| nativeScheduleMicroTask(drainMicroTaskQueue); | ||
| const isNativeDrainingEnabled = global[enableNativeMicrotaskDraining]; | ||
| const shouldDrainWithNative = | ||
| isNativeDrainingEnabled && _microTaskQueue.length === 0 && !_isDrainingMicrotaskQueue; | ||
| const shouldDrainWithoutNative = | ||
| !isNativeDrainingEnabled && _numberOfNestedTaskFrames === 0 && _microTaskQueue.length === 0; | ||
|
|
||
| if (shouldDrainWithNative || shouldDrainWithoutNative) { | ||
| // Start draining the microtask queue if: | ||
| // - Native draining is enabled and not currently in progress, or | ||
| // - Native draining is disabled, and there are no nested tasks and no queued microtasks. | ||
| nativeScheduleMicroTask(drainMicroTaskQueueSynchronously); | ||
| } | ||
|
|
||
| if (task) { | ||
| _microTaskQueue.push(task); | ||
| } | ||
| task && _microTaskQueue.push(task); | ||
| } | ||
|
|
||
| function drainMicroTaskQueue() { | ||
| if (!_isDrainingMicrotaskQueue) { | ||
| _isDrainingMicrotaskQueue = true; | ||
| while (_microTaskQueue.length) { | ||
| const queue = _microTaskQueue; | ||
| _microTaskQueue = []; | ||
| for (let i = 0; i < queue.length; i++) { | ||
| const task = queue[i]; | ||
| try { | ||
| task.zone.runTask(task, null, null); | ||
| } catch (error) { | ||
| _api.onUnhandledError(error as Error); | ||
| } | ||
| function drainMicroTaskQueueSynchronously() { | ||
| if (_isDrainingMicrotaskQueue) { | ||
| return; | ||
| } | ||
|
|
||
| _isDrainingMicrotaskQueue = true; | ||
|
|
||
| while (_microTaskQueue.length) { | ||
| const queue = _microTaskQueue; | ||
| _microTaskQueue = []; | ||
|
|
||
| for (const task of queue) { | ||
| try { | ||
| task.zone.runTask(task, null, null); | ||
| } catch (error) { | ||
| _api.onUnhandledError(error as Error); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // The order matters! | ||
| if (global[enableNativeMicrotaskDraining]) { | ||
| _isDrainingMicrotaskQueue = false; | ||
| _api.microtaskDrainDone(); | ||
|
Comment on lines
+1567
to
+1570
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't specifically recall why order matters and it does look odd that it's different from the other case. do you remember why and can you add more details?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't recall too, it didn't have any issues tbh. I think it should always be the following: _isDrainingMicrotaskQueue = false;
_api.microtaskDrainDone();Regardless whether |
||
| } else { | ||
| _api.microtaskDrainDone(); | ||
| _isDrainingMicrotaskQueue = false; | ||
| } | ||
|
|
@@ -1579,7 +1597,7 @@ export function initZone(): ZoneType { | |
| currentZoneFrame: () => _currentZoneFrame, | ||
| onUnhandledError: noop, | ||
| microtaskDrainDone: noop, | ||
| scheduleMicroTask: scheduleMicroTask, | ||
| scheduleMicroTask, | ||
| showUncaughtError: () => !(ZoneImpl as any)[__symbol__('ignoreConsoleErrorUncaughtError')], | ||
| patchEventTarget: () => [], | ||
| patchOnProperties: noop, | ||
|
|
@@ -1599,7 +1617,7 @@ export function initZone(): ZoneType { | |
| attachOriginToPatched: () => noop, | ||
| _redefineProperty: () => noop, | ||
| patchCallbacks: () => noop, | ||
| nativeScheduleMicroTask: nativeScheduleMicroTask, | ||
| nativeScheduleMicroTask, | ||
| }; | ||
| let _currentZoneFrame: ZoneFrame = {parent: null, zone: new ZoneImpl(null, null)}; | ||
| let _currentTask: Task | null = null; | ||
|
|
||
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.
nit: it wasn't a significant number (IIRC, like 10) but more that there were a few and it's really hard to tell if something just isn't covered by tests and is going to cause a production outage.