Skip to content

Commit

Permalink
fix(zone.js): fix several behavior diffs when Promise reject unhandled
Browse files Browse the repository at this point in the history 
There are several behavior differences between ZoneAwarePromise and
Native Promise.

Such as.

```
Promise.reject(new Error("test error")).finally(() => console.log("finally"));
```

1. Native Promise, will console log `finally` and throw a Promise
   unhandled error also process exit with code 1.
2. ZoneAwarePromise, will console log `finally` but will not output any
   uncaught error also process exit with code 0.

This PR fix the issue and make Promise rejection behave the same with
the native one.

PR Close angular#47562
  • Loading branch information
@JiaLiPassion
JiaLiPassion committed Dec 14, 2023
1 parent 629343f commit 3035927
Show file tree
Hide file tree
Showing 6 changed files with 123 additions and 58 deletions.
11 changes: 8 additions & 3 deletions packages/zone.js/lib/browser/browser.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,20 +267,25 @@ Zone.__load_patch('geolocation', (global: any) => {
}
});

Zone.__load_patch('PromiseRejectionEvent', (global: any, Zone: ZoneType) => {
Zone.__load_patch('PromiseRejectionEvent', (global: any, Zone: ZoneType, api: _ZonePrivate) => {
// handle unhandled promise rejection
function findPromiseRejectionHandler(evtName: string) {
return function(e: any) {
const eventTasks = findEventTasks(global, evtName);
let found = false;
eventTasks.forEach(eventTask => {
// windows has added unhandledrejection event listener
// trigger the event listener
const PromiseRejectionEvent = global['PromiseRejectionEvent'];
if (PromiseRejectionEvent) {
const evt = new PromiseRejectionEvent(evtName, {promise: e.promise, reason: e.rejection});
eventTask.invoke(evt);
found = true;
const evt = new PromiseRejectionEvent(evtName, e);
eventTask.invoke(eventTask, global, [evt]);
}
});
if (!found && evtName === 'unhandledrejection') {
api.onUnhandledError(e);
}
};
}

Expand Down
29 changes: 21 additions & 8 deletions packages/zone.js/lib/common/promise.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,19 +54,20 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr
throw uncaughtPromiseError;
});
} catch (error) {
handleUnhandledRejection(error);
handleUnhandledRejection({reason: error, promise: uncaughtPromiseError.promise});
}
}
};

const UNHANDLED_PROMISE_REJECTION_HANDLER_SYMBOL = __symbol__('unhandledPromiseRejectionHandler');

function handleUnhandledRejection(this: unknown, e: any) {
api.onUnhandledError(e);
try {
const handler = (Zone as any)[UNHANDLED_PROMISE_REJECTION_HANDLER_SYMBOL];
if (typeof handler === 'function') {
handler.call(this, e);
} else {
api.onUnhandledError(e);
}
} catch (err) {
}
Expand Down Expand Up @@ -188,10 +189,18 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr
}
}

let isAllFinallyPromiseOrEmptyQueue = true;
for (let i = 0; i < queue.length;) {
scheduleResolveOrReject(promise, queue[i++], queue[i++], queue[i++], queue[i++]);
const isFinally =
scheduleResolveOrReject(promise, queue[i++], queue[i++], queue[i++], queue[i++]);
if (isAllFinallyPromiseOrEmptyQueue && !isFinally) {
isAllFinallyPromiseOrEmptyQueue = false;
}
}
if (queue.length == 0 && state == REJECTED) {
// If the queue is empty or all queued promises are generated from finally call
// we need to check the state is REJECTED or not, if it is rejected, we got an
// uncaught promise rejection.
if (isAllFinallyPromiseOrEmptyQueue && state == REJECTED) {
(promise as any)[symbolState] = REJECTED_NO_CATCH;
let uncaughtPromiseError = value;
try {
Expand Down Expand Up @@ -249,8 +258,13 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr
function scheduleResolveOrReject<R, U1, U2>(
promise: ZoneAwarePromise<any>, zone: Zone, chainPromise: ZoneAwarePromise<any>,
onFulfilled?: ((value: R) => U1)|null|undefined,
onRejected?: ((error: any) => U2)|null|undefined): void {
clearRejectedNoCatch(promise);
onRejected?: ((error: any) => U2)|null|undefined): boolean {
const isFinallyPromise = symbolFinally === (chainPromise as any)?.[symbolFinally];
// If the chainPromise is generated from promise.finally() call, we should not
// clear the uncaught promise errors.
// otherwise, the code Promise.reject(new Error("test error")).finally(() =>
// console.log("finally")); will only print finally without throw rejection error.
!isFinallyPromise && clearRejectedNoCatch(promise);
const promiseState = (promise as any)[symbolState];
const delegate = promiseState ?
(typeof onFulfilled === 'function') ? onFulfilled : forwardResolution :
Expand All @@ -259,8 +273,6 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr
zone.scheduleMicroTask(source, () => {
try {
const parentPromiseValue = (promise as any)[symbolValue];
const isFinallyPromise =
!!chainPromise && symbolFinally === (chainPromise as any)[symbolFinally];
if (isFinallyPromise) {
// if the promise is generated from finally call, keep parent promise's state and value
(chainPromise as any)[symbolParentPromiseValue] = parentPromiseValue;
Expand All @@ -278,6 +290,7 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr
resolvePromise(chainPromise, false, error);
}
}, chainPromise as TaskData);
return isFinallyPromise;
}

const ZONE_AWARE_PROMISE_TO_STRING = 'function ZoneAwarePromise() { [native code] }';
Expand Down
13 changes: 12 additions & 1 deletion packages/zone.js/lib/node/node.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,17 +96,28 @@ Zone.__load_patch(
function findProcessPromiseRejectionHandler(evtName: string) {
return function(e: any) {
const eventTasks = findEventTasks(process, evtName);
let found = false;
eventTasks.forEach(eventTask => {
// process has added unhandledrejection event listener
// trigger the event listener
if (evtName === 'unhandledRejection') {
eventTask.invoke(e.rejection, e.promise);
eventTask.invoke(e.reason, e.promise);
found = true;
} else if (evtName === 'rejectionHandled') {
eventTask.invoke(e.promise);
found = true;
}
});
if (!found && evtName === 'unhandledRejection') {
api.onUnhandledError(e);
}
};
}

// api.onUnhandledError = function(error: any) {
// const NativePromise = global[api.symbol('Promise')];
// NativePromise?.reject(error?.rejection || error);
// }
});


Expand Down
121 changes: 78 additions & 43 deletions packages/zone.js/test/common/Promise.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,9 @@ describe(
(new Promise(function(_, rejectFn) {
reject = rejectFn;
}) as any)
.catch((err: any) => {
expect(err).toBe('error');
})
.finally(function() {
expect(arguments.length).toBe(0);
expect(Zone.current).toBe(testZone);
Expand All @@ -296,6 +299,81 @@ describe(
reject!('error');
});

it('should work with .finally with lazy rejected promise without catch', async function() {
await jasmine.spyOnGlobalErrorsAsync(async () => {
let reject: Function|null = null;
let uncaughtError: any = null;
let promise: any = null;

const uncaught = (error: any, p: Promise<any>) => {
uncaughtError = (error?.reason || error).rejection;
promise = error?.promise || p;
if (isNode) {
process?.removeListener('unhandledRejection', uncaught);
} else {
window?.removeEventListener('unhandledrejection', uncaught as any);
}
expect(uncaughtError).toBe('error');
};
if (isNode) {
process?.addListener('unhandledRejection', uncaught);
} else {
window?.addEventListener('unhandledrejection', uncaught as any);
}

testZone.run(function() {
(new Promise(function(_, rejectFn) {
reject = rejectFn;
}) as any)
.finally(function() {
expect(arguments.length).toBe(0);
expect(Zone.current).toBe(testZone);
});
});

reject!('error');
return new Promise(res => setTimeout(() => {
promise?.catch(() => {});
res();
}));
});
});

it('should work with .finally with rejected promise without catch', async function() {
await jasmine.spyOnGlobalErrorsAsync(async () => {
let uncaughtError: any = null;
let promise: any = null;

const uncaught = (error: any, p: any) => {
uncaughtError = (error?.reason || error).rejection;
promise = error?.promise || p;
if (isNode) {
process?.removeListener('unhandledRejection', uncaught);
} else {
window?.removeEventListener('unhandledrejection', uncaught as any);
}
expect(uncaughtError).toBe('error');
};
if (isNode) {
process?.addListener('unhandledRejection', uncaught);
} else {
window?.addEventListener('unhandledrejection', uncaught as any);
}

testZone.run(function() {
Promise.reject('error').finally(function() {
expect(arguments.length).toBe(0);
expect(Zone.current).toBe(testZone);
});
});
return new Promise(res => setTimeout(() => {
promise?.catch(() => {});
res();
}));
});
});


it('should work with Promise.resolve', () => {
queueZone.run(() => {
let value: any = null;
Expand Down Expand Up @@ -357,49 +435,6 @@ describe(
});
});

it('should output error to console if ignoreConsoleErrorUncaughtError is false',
async () => {
await jasmine.spyOnGlobalErrorsAsync(() => {
const originalConsoleError = console.error;
Zone.current.fork({name: 'promise-error'}).run(() => {
(Zone as any)[Zone.__symbol__('ignoreConsoleErrorUncaughtError')] = false;
console.error = jasmine.createSpy('consoleErr');
const p = new Promise((resolve, reject) => {
throw new Error('promise error');
});
});
return new Promise(res => {
setTimeout(() => {
expect(console.error).toHaveBeenCalled();
console.error = originalConsoleError;
res();
});
});
});
});

it('should not output error to console if ignoreConsoleErrorUncaughtError is true',
async () => {
await jasmine.spyOnGlobalErrorsAsync(() => {
const originalConsoleError = console.error;
Zone.current.fork({name: 'promise-error'}).run(() => {
(Zone as any)[Zone.__symbol__('ignoreConsoleErrorUncaughtError')] = true;
console.error = jasmine.createSpy('consoleErr');
const p = new Promise((resolve, reject) => {
throw new Error('promise error');
});
});
return new Promise(res => {
setTimeout(() => {
expect(console.error).not.toHaveBeenCalled();
console.error = originalConsoleError;
(Zone as any)[Zone.__symbol__('ignoreConsoleErrorUncaughtError')] = false;
res();
});
});
});
});

it('should notify Zone.onHandleError if no one catches promise', (done) => {
let promiseError: Error|null = null;
let zone: Zone|null = null;
Expand Down
6 changes: 3 additions & 3 deletions packages/zone.js/test/node/process.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe('process related test', () => {
(Zone as any)[zoneSymbol('ignoreConsoleErrorUncaughtError')] = true;
Zone.current.fork({name: 'promise'}).run(function() {
const listener = function(reason: any, promise: any) {
hookSpy(promise, reason.message);
hookSpy(promise, reason?.rejection?.message);
process.removeListener('unhandledRejection', listener);
};
process.on('unhandledRejection', listener);
Expand Down Expand Up @@ -124,11 +124,11 @@ describe('process related test', () => {
let p: any = null;
Zone.current.fork({name: 'promise'}).run(function() {
const listener1 = function(reason: any, promise: any) {
hookSpy(promise, reason.message);
hookSpy(promise, reason.rejection.message);
process.removeListener('unhandledRejection', listener1);
};
const listener2 = function(reason: any, promise: any) {
hookSpy(promise, reason.message);
hookSpy(promise, reason.rejection.message);
process.removeListener('unhandledRejection', listener2);
};
process.on('unhandledRejection', listener1);
Expand Down
1 change: 1 addition & 0 deletions packages/zone.js/test/zone-spec/long-stack-trace-zone.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ describe(
});
setTimeout(function() {
expectElapsed(log[0].stack!, 5);
promise.catch(() => {});
done();
}, 0);
}, 0);
Expand Down

0 comments on commit 3035927

Please sign in to comment.