From bbbc66b33884a39a413e1d605699284f274ff5ff Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 10 Mar 2023 15:28:03 -0500 Subject: [PATCH] flushSync: Exhaust queue even if something throws If something throws as a result of `flushSync`, and there's remaining work left in the queue, React should keep working until all the work is complete. If multiple errors are thrown, React will combine them into an AggregateError object and throw that. In environments where AggregateError is not available, React will rethrow in an async task. (All the evergreen runtimes support AggregateError.) The scenario where this happens is relatively rare, because `flushSync` will only throw if there's no error boundary to capture the error. --- .../src/ReactFiberSyncTaskQueue.js | 74 +++++++++++------ .../src/__tests__/ReactFlushSync-test.js | 46 +++++++++++ .../ReactFlushSyncNoAggregateError-test.js | 79 +++++++++++++++++++ ...tIncrementalErrorHandling-test.internal.js | 25 +++--- .../src/__tests__/ReactTestRenderer-test.js | 21 +++-- scripts/flow/environment.js | 1 + scripts/rollup/validate/eslintrc.cjs.js | 1 + scripts/rollup/validate/eslintrc.cjs2015.js | 1 + scripts/rollup/validate/eslintrc.esm.js | 1 + scripts/rollup/validate/eslintrc.fb.js | 1 + scripts/rollup/validate/eslintrc.rn.js | 1 + scripts/rollup/validate/eslintrc.umd.js | 1 + 12 files changed, 211 insertions(+), 41 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js diff --git a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js b/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js index 273de09874789..0b11be38a1dac 100644 --- a/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js +++ b/packages/react-reconciler/src/ReactFiberSyncTaskQueue.js @@ -52,37 +52,67 @@ export function flushSyncCallbacks(): null { if (!isFlushingSyncQueue && syncQueue !== null) { // Prevent re-entrance. isFlushingSyncQueue = true; - let i = 0; + + // Set the event priority to discrete + // TODO: Is this necessary anymore? The only user code that runs in this + // queue is in the render or commit phases, which already set the + // event priority. Should be able to remove. const previousUpdatePriority = getCurrentUpdatePriority(); - try { - const isSync = true; - const queue = syncQueue; - // TODO: Is this necessary anymore? The only user code that runs in this - // queue is in the render or commit phases. - setCurrentUpdatePriority(DiscreteEventPriority); + setCurrentUpdatePriority(DiscreteEventPriority); + + let errors: Array | null = null; + + const queue = syncQueue; + // $FlowFixMe[incompatible-use] found when upgrading Flow + for (let i = 0; i < queue.length; i++) { // $FlowFixMe[incompatible-use] found when upgrading Flow - for (; i < queue.length; i++) { - // $FlowFixMe[incompatible-use] found when upgrading Flow - let callback: SchedulerCallback = queue[i]; + let callback: SchedulerCallback = queue[i]; + try { do { + const isSync = true; // $FlowFixMe[incompatible-type] we bail out when we get a null callback = callback(isSync); } while (callback !== null); + } catch (error) { + // Collect errors so we can rethrow them at the end + if (errors === null) { + errors = [error]; + } else { + errors.push(error); + } } - syncQueue = null; - includesLegacySyncCallbacks = false; - } catch (error) { - // If something throws, leave the remaining callbacks on the queue. - if (syncQueue !== null) { - syncQueue = syncQueue.slice(i + 1); + } + + syncQueue = null; + includesLegacySyncCallbacks = false; + setCurrentUpdatePriority(previousUpdatePriority); + isFlushingSyncQueue = false; + + if (errors !== null) { + if (errors.length > 1) { + if (typeof AggregateError === 'function') { + // eslint-disable-next-line no-undef + throw new AggregateError(errors); + } else { + for (let i = 1; i < errors.length; i++) { + scheduleCallback( + ImmediatePriority, + throwError.bind(null, errors[i]), + ); + } + const firstError = errors[0]; + throw firstError; + } + } else { + const error = errors[0]; + throw error; } - // Resume flushing in the next tick - scheduleCallback(ImmediatePriority, flushSyncCallbacks); - throw error; - } finally { - setCurrentUpdatePriority(previousUpdatePriority); - isFlushingSyncQueue = false; } } + return null; } + +function throwError(error: mixed) { + throw error; +} diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js index 6ed8d95013947..e2d9ba76660f9 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js @@ -248,4 +248,50 @@ describe('ReactFlushSync', () => { // Now the effects have fired. assertLog(['Effect']); }); + + test('completely exhausts synchronous work queue even if something throws', async () => { + function Throws({error}) { + throw error; + } + + const root1 = ReactNoop.createRoot(); + const root2 = ReactNoop.createRoot(); + const root3 = ReactNoop.createRoot(); + + await act(async () => { + root1.render(); + root2.render(); + root3.render(); + }); + assertLog(['Hi', 'Andrew', '!']); + + const aahh = new Error('AAHH!'); + const nooo = new Error('Noooooooooo!'); + + let error; + try { + ReactNoop.flushSync(() => { + root1.render(); + root2.render(); + root3.render(); + }); + } catch (e) { + error = e; + } + + // The update to root 3 should have finished synchronously, even though the + // earlier updates errored. + assertLog(['aww']); + // Roots 1 and 2 were unmounted. + expect(root1).toMatchRenderedOutput(null); + expect(root2).toMatchRenderedOutput(null); + expect(root3).toMatchRenderedOutput('aww'); + + // Because there were multiple errors, React threw an AggregateError. + // eslint-disable-next-line no-undef + expect(error).toBeInstanceOf(AggregateError); + expect(error.errors.length).toBe(2); + expect(error.errors[0]).toBe(aahh); + expect(error.errors[1]).toBe(nooo); + }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js new file mode 100644 index 0000000000000..6cef94bbd35f5 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactFlushSyncNoAggregateError-test.js @@ -0,0 +1,79 @@ +let React; +let ReactNoop; +let Scheduler; +let act; +let assertLog; +let waitForThrow; + +// TODO: Migrate tests to React DOM instead of React Noop + +describe('ReactFlushSync (AggregateError not available)', () => { + beforeEach(() => { + jest.resetModules(); + + global.AggregateError = undefined; + + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + act = require('internal-test-utils').act; + + const InternalTestUtils = require('internal-test-utils'); + assertLog = InternalTestUtils.assertLog; + waitForThrow = InternalTestUtils.waitForThrow; + }); + + function Text({text}) { + Scheduler.log(text); + return text; + } + + test('completely exhausts synchronous work queue even if something throws', async () => { + function Throws({error}) { + throw error; + } + + const root1 = ReactNoop.createRoot(); + const root2 = ReactNoop.createRoot(); + const root3 = ReactNoop.createRoot(); + + await act(async () => { + root1.render(); + root2.render(); + root3.render(); + }); + assertLog(['Hi', 'Andrew', '!']); + + const aahh = new Error('AAHH!'); + const nooo = new Error('Noooooooooo!'); + + let error; + try { + ReactNoop.flushSync(() => { + root1.render(); + root2.render(); + root3.render(); + }); + } catch (e) { + error = e; + } + + // The update to root 3 should have finished synchronously, even though the + // earlier updates errored. + assertLog(['aww']); + // Roots 1 and 2 were unmounted. + expect(root1).toMatchRenderedOutput(null); + expect(root2).toMatchRenderedOutput(null); + expect(root3).toMatchRenderedOutput('aww'); + + // In modern environments, React would throw an AggregateError. Because + // AggregateError is not available, React throws the first error, then + // throws the remaining errors in separate tasks. + expect(error).toBe(aahh); + // TODO: Currently the remaining error is rethrown in an Immediate Scheduler + // task, but this may change to a timer or microtask in the future. The + // exact mechanism is an implementation detail; they just need to be logged + // in the order the occurred. + await waitForThrow(nooo); + }); +}); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 285274fca0781..e97476b71b580 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -1367,17 +1367,14 @@ describe('ReactIncrementalErrorHandling', () => { ); await waitForAll([]); - expect(() => { - expect(() => { - ReactNoop.flushSync(() => { - inst.setState({fail: true}); - }); - }).toThrow('Hello.'); - - // The unmount is queued in a microtask. In order to capture the error - // that occurs during unmount, we can flush it early with `flushSync`. - ReactNoop.flushSync(); - }).toThrow('One does not simply unmount me.'); + let aggregateError; + try { + ReactNoop.flushSync(() => { + inst.setState({fail: true}); + }); + } catch (e) { + aggregateError = e; + } assertLog([ // Attempt to clean up. @@ -1387,6 +1384,12 @@ describe('ReactIncrementalErrorHandling', () => { 'BrokenRenderAndUnmount componentWillUnmount', ]); expect(ReactNoop).toMatchRenderedOutput(null); + + // React threw both errors as a single AggregateError + const errors = aggregateError.errors; + expect(errors.length).toBe(2); + expect(errors[0].message).toBe('Hello.'); + expect(errors[1].message).toBe('One does not simply unmount me.'); }); it('does not interrupt unmounting if detaching a ref throws', async () => { diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js index 44f58434c2b47..32118f5befede 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js @@ -31,18 +31,23 @@ describe('ReactTestRenderer', () => { it('should warn if used to render a ReactDOM portal', () => { const container = document.createElement('div'); expect(() => { + let error; try { ReactTestRenderer.create(ReactDOM.createPortal('foo', container)); } catch (e) { - // TODO: After the update throws, a subsequent render is scheduled to - // unmount the whole tree. This update also causes an error, and this - // happens in a separate task. Flush this error now and capture it, to - // prevent it from firing asynchronously and causing the Jest test - // to fail. - expect(() => Scheduler.unstable_flushAll()).toThrow( - '.children.indexOf is not a function', - ); + error = e; } + // After the update throws, a subsequent render is scheduled to + // unmount the whole tree. This update also causes an error, so React + // throws an AggregateError. + const errors = error.errors; + expect(errors.length).toBe(2); + expect(errors[0].message.includes('indexOf is not a function')).toBe( + true, + ); + expect(errors[1].message.includes('indexOf is not a function')).toBe( + true, + ); }).toErrorDev('An invalid container has been provided.', { withoutStack: true, }); diff --git a/scripts/flow/environment.js b/scripts/flow/environment.js index 8f5b43fb4532c..18ba25264138e 100644 --- a/scripts/flow/environment.js +++ b/scripts/flow/environment.js @@ -22,6 +22,7 @@ declare var globalThis: Object; declare var queueMicrotask: (fn: Function) => void; declare var reportError: (error: mixed) => void; +declare var AggregateError: Class; declare module 'create-react-class' { declare var exports: React$CreateClass; diff --git a/scripts/rollup/validate/eslintrc.cjs.js b/scripts/rollup/validate/eslintrc.cjs.js index 64d3ddc8aeef7..bf24ac234a39b 100644 --- a/scripts/rollup/validate/eslintrc.cjs.js +++ b/scripts/rollup/validate/eslintrc.cjs.js @@ -33,6 +33,7 @@ module.exports = { TaskController: 'readonly', reportError: 'readonly', + AggregateError: 'readonly', // Flight Uint8Array: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.cjs2015.js b/scripts/rollup/validate/eslintrc.cjs2015.js index f276094c64954..e5b79f84c3e09 100644 --- a/scripts/rollup/validate/eslintrc.cjs2015.js +++ b/scripts/rollup/validate/eslintrc.cjs2015.js @@ -33,6 +33,7 @@ module.exports = { TaskController: 'readonly', reportError: 'readonly', + AggregateError: 'readonly', // Flight Uint8Array: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.esm.js b/scripts/rollup/validate/eslintrc.esm.js index e3ab6ec62fd3d..ef762a404f396 100644 --- a/scripts/rollup/validate/eslintrc.esm.js +++ b/scripts/rollup/validate/eslintrc.esm.js @@ -32,6 +32,7 @@ module.exports = { TaskController: 'readonly', reportError: 'readonly', + AggregateError: 'readonly', // Flight Uint8Array: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.fb.js b/scripts/rollup/validate/eslintrc.fb.js index 7019e22435e12..01e5c5e96e246 100644 --- a/scripts/rollup/validate/eslintrc.fb.js +++ b/scripts/rollup/validate/eslintrc.fb.js @@ -33,6 +33,7 @@ module.exports = { TaskController: 'readonly', reportError: 'readonly', + AggregateError: 'readonly', // Flight Uint8Array: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.rn.js b/scripts/rollup/validate/eslintrc.rn.js index 524e187c9b3cc..023f0d652fd92 100644 --- a/scripts/rollup/validate/eslintrc.rn.js +++ b/scripts/rollup/validate/eslintrc.rn.js @@ -33,6 +33,7 @@ module.exports = { TaskController: 'readonly', reportError: 'readonly', + AggregateError: 'readonly', // Temp AsyncLocalStorage: 'readonly', diff --git a/scripts/rollup/validate/eslintrc.umd.js b/scripts/rollup/validate/eslintrc.umd.js index 09554df63b07b..d2e1bb721815b 100644 --- a/scripts/rollup/validate/eslintrc.umd.js +++ b/scripts/rollup/validate/eslintrc.umd.js @@ -37,6 +37,7 @@ module.exports = { TaskController: 'readonly', reportError: 'readonly', + AggregateError: 'readonly', // Flight Uint8Array: 'readonly',