From 325bf67d5afeef310d567b319cf59eb5e254e193 Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 25 Mar 2023 02:21:59 +0000 Subject: [PATCH] fix(zone.js): fix several behavior diffs when Promise reject unhandled Close #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. --- packages/zone.js/lib/browser/browser.ts | 11 +- packages/zone.js/lib/common/promise.ts | 29 +++-- packages/zone.js/lib/node/node.ts | 13 +- packages/zone.js/test/common/Promise.spec.ts | 121 +++++++++++------- packages/zone.js/test/node/process.spec.ts | 6 +- .../zone-spec/long-stack-trace-zone.spec.ts | 1 + 6 files changed, 123 insertions(+), 58 deletions(-) diff --git a/packages/zone.js/lib/browser/browser.ts b/packages/zone.js/lib/browser/browser.ts index 0309a4c7e7765c..b7fbd58493cd19 100644 --- a/packages/zone.js/lib/browser/browser.ts +++ b/packages/zone.js/lib/browser/browser.ts @@ -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); + } }; } diff --git a/packages/zone.js/lib/common/promise.ts b/packages/zone.js/lib/common/promise.ts index 4930c524149844..e598bd31f1754a 100644 --- a/packages/zone.js/lib/common/promise.ts +++ b/packages/zone.js/lib/common/promise.ts @@ -54,7 +54,7 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr throw uncaughtPromiseError; }); } catch (error) { - handleUnhandledRejection(error); + handleUnhandledRejection({reason: error, promise: uncaughtPromiseError.promise}); } } }; @@ -62,11 +62,12 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr 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) { } @@ -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 { @@ -249,8 +258,13 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr function scheduleResolveOrReject( promise: ZoneAwarePromise, zone: Zone, chainPromise: ZoneAwarePromise, 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 : @@ -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; @@ -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] }'; diff --git a/packages/zone.js/lib/node/node.ts b/packages/zone.js/lib/node/node.ts index 728be926588674..70c45d986ce560 100644 --- a/packages/zone.js/lib/node/node.ts +++ b/packages/zone.js/lib/node/node.ts @@ -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); + } }); diff --git a/packages/zone.js/test/common/Promise.spec.ts b/packages/zone.js/test/common/Promise.spec.ts index 126d8b50524af6..34d2fc6986fd02 100644 --- a/packages/zone.js/test/common/Promise.spec.ts +++ b/packages/zone.js/test/common/Promise.spec.ts @@ -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); @@ -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) => { + 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; @@ -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; diff --git a/packages/zone.js/test/node/process.spec.ts b/packages/zone.js/test/node/process.spec.ts index 4324ca798b3321..020b7f6ea1e56d 100644 --- a/packages/zone.js/test/node/process.spec.ts +++ b/packages/zone.js/test/node/process.spec.ts @@ -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); @@ -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); diff --git a/packages/zone.js/test/zone-spec/long-stack-trace-zone.spec.ts b/packages/zone.js/test/zone-spec/long-stack-trace-zone.spec.ts index 36c5f3d26764ac..94e2884a6fbd72 100644 --- a/packages/zone.js/test/zone-spec/long-stack-trace-zone.spec.ts +++ b/packages/zone.js/test/zone-spec/long-stack-trace-zone.spec.ts @@ -152,6 +152,7 @@ describe( }); setTimeout(function() { expectElapsed(log[0].stack!, 5); + promise.catch(() => {}); done(); }, 0); }, 0);