Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 56 additions & 38 deletions packages/zone.js/lib/zone-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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--;
}
Expand Down Expand Up @@ -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.
Comment on lines +1503 to +1507

Copy link
Copy Markdown
Contributor

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.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 microtaskDrainDone would throw.

} else {
_api.microtaskDrainDone();
_isDrainingMicrotaskQueue = false;
}
Expand Down Expand Up @@ -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,
Expand All @@ -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;
Expand Down
53 changes: 53 additions & 0 deletions packages/zone.js/lib/zone.configurations.api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,59 @@ declare global {
* the user with a string returned from the event handler.
*/
__zone_symbol__enable_beforeunload?: boolean;

/**
* https://github.com/angular/angular/issues/41506
* https://github.com/angular/angular/issues/44446
*
* By default, `zone.js` maintains a microtask queue manually, which means the microtask
* queue is drained whenever `zone.js` decides to do so under certain circumstances.
* Typically, `zone.js` invokes a task (e.g., an event task) and, after invoking the task,
* checks whether the number of nested task frames is equal to 1 before calling the microtask
* queue draining.
* As thus, there are cases when the microtask queue may be drained synchronously after an
* event task is invoked (if it’s the very first task in the call stack).
* Tasks may actually schedule other tasks, thereby incrementing the stack frame.
* In that case, the microtask queue might be drained after the last task is invoked.
*
* Given that code:
* ```js
* Zone.current.fork({name: 'child'}).run(() => {
* const div = document.createElement('div');
* div.style.height = '200px';
* div.style.width = '200px';
* div.style.backgroundColor = 'red';
* document.body.appendChild(div);
*
* function listener() {
* Promise.resolve().then(() => {
* div.style.height = '400px';
* });
* }
*
* div.addEventListener('fakeEvent', listener);
* div.dispatchEvent(new Event('fakeEvent'));
* console.log(div.getBoundingClientRect().height); // 400
* });
* ```
*
* We would assume that "200" would be logged. However, with `zone.js`, "400" will
* be logged first because it drains the microtask queue too early, as the `fakeEvent`
* event task is the very top task on the stack.
*
* https://promisesaplus.com/#the-then-method
* According to the spec: `onFulfilled` or `onRejected` must not be called until the
* execution context stack contains only platform code.
*
* You may consider enabling the flag below. This will ensure that microtask draining
* does not happen synchronously and always occurs within a browser microtask.
*
* This is critically important for our code and other third-party code, which is
* beyond our control, to work properly. If a microtask is scheduled within an event
* listener to be executed "later", it should indeed be executed later and not synchronously,
* as this would break the expected flow of code execution.
*/
__zone_symbol__enable_native_microtask_draining?: boolean;
}

/**
Expand Down
150 changes: 150 additions & 0 deletions packages/zone.js/test/common/zone.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.dev/license
*/

import {isNode} from '../../lib/common/utils';

describe('Zone Common', function () {
it('should have a name', function () {
expect(Zone.current.name).toBeDefined();
Expand Down Expand Up @@ -432,6 +435,153 @@ describe('Zone Common', function () {
log = [];
});

// https://github.com/angular/angular/issues/44446
// https://github.com/angular/angular/issues/55590
// https://github.com/angular/angular/issues/51328
describe('__zone_symbol__enable_native_microtask_draining', () => {
it('should not drain the microtask queue too early without task (if the flag is enabled)', (done) => {
// Regression test for https://github.com/angular/angular/issues/44446.
// Verifies that a microtask scheduled inside an event task is not drained
// synchronously mid-stack when the native draining flag is enabled.
const globalAny = global as any;
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = true;
const zone = Zone.current;
const event = zone.scheduleEventTask(
'test',
() => {
log.push('eventTask');
zone.scheduleMicroTask('test', () => log.push('microTask'));
},
undefined,
noop,
noop,
);
log.push('after schedule eventTask');
expect(log).toEqual(['after schedule eventTask']);
event.invoke();
// At this point, we should not have invoked the microtask.
expect(log).toEqual(['after schedule eventTask', 'eventTask']);
globalAny[Zone.__symbol__('setTimeout')](() => {
expect(log).toEqual(['after schedule eventTask', 'eventTask', 'microTask']);
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = false;
done();
});
});

it('should not drain the microtask queue too early (if the flag is enabled)', (done) => {
// We need `document` in this test.
if (isNode) {
done();
return;
}

// Regression test for https://github.com/angular/angular/issues/44446.
// Verifies that a Promise.then() callback scheduled inside a DOM event listener
// is not drained synchronously before the main stack unwinds.

const globalAny = global as any;
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = true;
const zone = Zone.current;

zone.run(() => {
const listener = () => {
Promise.resolve().then(() => log.push('promise.then'));
};

document.body.addEventListener('click', listener);
document.body.click();
log.push('main stack');

globalAny[Zone.__symbol__('setTimeout')](() => {
document.body.removeEventListener('click', listener);
expect(log).toEqual(['main stack', 'promise.then']);
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = false;
done();
});
});
});

it('should surface unhandled promise rejections via unhandledrejection event (if the flag is enabled)', async () => {
// We need `window` in this test.
if (isNode) {
return;
}

// Regression test for https://github.com/angular/angular/issues/55590.
// Verifies that unhandled promise rejections originating outside zone.js-patched
// code (e.g. a plain <script> tag) still surface via the native unhandledrejection
// event when the native draining flag is enabled.

let rejectionEvent: PromiseRejectionEvent | null = null;

const onError = window.onerror;
window.onerror = () => {};

const globalAny = global as any;
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = true;

await jasmine.spyOnGlobalErrorsAsync(async () => {
const handler = (event: PromiseRejectionEvent) => {
window.removeEventListener('unhandledrejection', handler);
rejectionEvent = event;
};
window.addEventListener('unhandledrejection', handler);

const script = document.createElement('script');
script.innerHTML = "Promise.reject('Error happened :(')";
document.body.append(script);

// Wait until the event task is dispatched.
await new Promise((resolve) => setTimeout(resolve, 10));

window.onerror = onError;
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = false;

expect(rejectionEvent).not.toBeNull();
expect(rejectionEvent!.reason).toBe('Error happened :(');
});
});

it('should not surface a rejection as unhandled when catch is attached (if the flag is enabled)', async () => {
// We need `window` in this test.
if (isNode) {
return;
}

// Regression test for https://github.com/angular/angular/issues/51328.
// Verifies that an internal `await Promise.reject()` inside an async IIFE does not
// fire `unhandledrejection` when a `.catch()` is attached to the outer promise.

let rejectionEvent: PromiseRejectionEvent | null = null;

const onError = window.onerror;
window.onerror = () => {};

const globalAny = global as any;
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = true;

await jasmine.spyOnGlobalErrorsAsync(async () => {
const handler = (event: PromiseRejectionEvent) => {
window.removeEventListener('unhandledrejection', handler);
rejectionEvent = event;
};
window.addEventListener('unhandledrejection', handler);

(async function () {
await Promise.reject(2);
})().catch(() => {});

// Wait until the event task is dispatched.
await new Promise((resolve) => setTimeout(resolve, 10));

window.onerror = onError;
globalAny[Zone.__symbol__('enable_native_microtask_draining')] = false;

expect(rejectionEvent).toBeNull();
});
});
});

it('should not drain the microtask queue too early', () => {
const z = Zone.current;
const event = z.scheduleEventTask('test', () => log.push('eventTask'), undefined, noop, noop);
Expand Down
Loading