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 
Close angular#47562

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.
  • Loading branch information
@JiaLiPassion
JiaLiPassion committed Dec 14, 2023
1 parent 629343f commit 325bf67
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 325bf67

Please sign in to comment.