Skip to content

Commit

Permalink
flushSync: Exhaust queue even if something throws (#26366)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored Mar 10, 2023
1 parent a22bd99 commit 93c10df
Show file tree
Hide file tree
Showing 12 changed files with 211 additions and 41 deletions.
74 changes: 52 additions & 22 deletions packages/react-reconciler/src/ReactFiberSyncTaskQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<mixed> | 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;
}
46 changes: 46 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(<Text text="Hi" />);
root2.render(<Text text="Andrew" />);
root3.render(<Text text="!" />);
});
assertLog(['Hi', 'Andrew', '!']);

const aahh = new Error('AAHH!');
const nooo = new Error('Noooooooooo!');

let error;
try {
ReactNoop.flushSync(() => {
root1.render(<Throws error={aahh} />);
root2.render(<Throws error={nooo} />);
root3.render(<Text text="aww" />);
});
} 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);
});
});
Original file line number Diff line number Diff line change
@@ -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(<Text text="Hi" />);
root2.render(<Text text="Andrew" />);
root3.render(<Text text="!" />);
});
assertLog(['Hi', 'Andrew', '!']);

const aahh = new Error('AAHH!');
const nooo = new Error('Noooooooooo!');

let error;
try {
ReactNoop.flushSync(() => {
root1.render(<Throws error={aahh} />);
root2.render(<Throws error={nooo} />);
root3.render(<Text text="aww" />);
});
} 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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down
1 change: 1 addition & 0 deletions scripts/flow/environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ declare var globalThis: Object;

declare var queueMicrotask: (fn: Function) => void;
declare var reportError: (error: mixed) => void;
declare var AggregateError: Class<Error>;

declare module 'create-react-class' {
declare var exports: React$CreateClass;
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.cjs.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ module.exports = {

TaskController: 'readonly',
reportError: 'readonly',
AggregateError: 'readonly',

// Flight
Uint8Array: 'readonly',
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.cjs2015.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ module.exports = {

TaskController: 'readonly',
reportError: 'readonly',
AggregateError: 'readonly',

// Flight
Uint8Array: 'readonly',
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.esm.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ module.exports = {

TaskController: 'readonly',
reportError: 'readonly',
AggregateError: 'readonly',

// Flight
Uint8Array: 'readonly',
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ module.exports = {

TaskController: 'readonly',
reportError: 'readonly',
AggregateError: 'readonly',

// Flight
Uint8Array: 'readonly',
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.rn.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ module.exports = {

TaskController: 'readonly',
reportError: 'readonly',
AggregateError: 'readonly',

// Temp
AsyncLocalStorage: 'readonly',
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.umd.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ module.exports = {

TaskController: 'readonly',
reportError: 'readonly',
AggregateError: 'readonly',

// Flight
Uint8Array: 'readonly',
Expand Down

0 comments on commit 93c10df

Please sign in to comment.