Skip to content

Commit

Permalink
Don't retry synchronous render that doesn't go through Scheduler
Browse files Browse the repository at this point in the history
  • Loading branch information
eps1lon committed Apr 21, 2024
1 parent a5117da commit cd59515
Show file tree
Hide file tree
Showing 8 changed files with 70 additions and 83 deletions.
Expand Up @@ -752,7 +752,7 @@ describe('ReactInternalTestUtils console assertions', () => {
You must call one of the assertConsoleDev helpers between each act call."
`);

await waitForAll(['A', 'B', 'A', 'B']);
await waitForAll(['A', 'B']);
});

test('should fail if waitForPaint is called before asserting', async () => {
Expand Down Expand Up @@ -1684,7 +1684,7 @@ describe('ReactInternalTestUtils console assertions', () => {
You must call one of the assertConsoleDev helpers between each act call."
`);

await waitForAll(['A', 'B', 'A', 'B']);
await waitForAll(['A', 'B']);
});

test('should fail if waitForPaint is called before asserting', async () => {
Expand Down Expand Up @@ -2634,7 +2634,7 @@ describe('ReactInternalTestUtils console assertions', () => {
You must call one of the assertConsoleDev helpers between each act call."
`);

await waitForAll(['A', 'B', 'A', 'B']);
await waitForAll(['A', 'B']);
});

test('should fail if waitForPaint is called before asserting', async () => {
Expand Down
10 changes: 5 additions & 5 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Expand Up @@ -1364,14 +1364,14 @@ export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null {
}

let exitStatus = renderRootSync(root, lanes);

const wasRootDehydrated = supportsHydration && isRootDehydrated(root);
if (
(disableLegacyMode || root.tag !== LegacyRoot) &&
exitStatus === RootErrored
exitStatus === RootErrored &&
wasRootDehydrated
) {
// If something threw an error, try rendering one more time. We'll render
// synchronously to block concurrent data mutations, and we'll includes
// all pending updates are included. If it still fails after the second
// attempt, we'll give up and commit the resulting tree.
// If something threw an error and we have work to do, try rendering one more time.
const originallyAttemptedLanes = lanes;
const errorRetryLanes = getLanesToRetrySynchronouslyOnError(
root,
Expand Down
Expand Up @@ -410,7 +410,7 @@ describe('ReactIncrementalErrorHandling', () => {
ReactNoop.render(<Parent />, () => Scheduler.log('commit'));
});

assertLog(['Parent', 'BadRender', 'Parent', 'BadRender', 'commit']);
assertLog(['Parent', 'BadRender', 'commit']);
expect(ReactNoop).toMatchRenderedOutput(null);
});

Expand Down Expand Up @@ -666,12 +666,6 @@ describe('ReactIncrementalErrorHandling', () => {
assertLog([
'ErrorBoundary render success',
'BrokenRender',

// React retries one more time
'ErrorBoundary render success',
'BrokenRender',

// Errored again on retry. Now handle it.
'ErrorBoundary componentDidCatch',
'ErrorBoundary render error',
]);
Expand Down Expand Up @@ -716,12 +710,6 @@ describe('ReactIncrementalErrorHandling', () => {
assertLog([
'ErrorBoundary render success',
'BrokenRender',

// React retries one more time
'ErrorBoundary render success',
'BrokenRender',

// Errored again on retry. Now handle it.
'ErrorBoundary componentDidCatch',
'ErrorBoundary render error',
]);
Expand Down Expand Up @@ -840,12 +828,6 @@ describe('ReactIncrementalErrorHandling', () => {
assertLog([
'RethrowErrorBoundary render',
'BrokenRender',

// React retries one more time
'RethrowErrorBoundary render',
'BrokenRender',

// Errored again on retry. Now handle it.
'RethrowErrorBoundary componentDidCatch',
]);
expect(ReactNoop).toMatchRenderedOutput(null);
Expand Down Expand Up @@ -882,12 +864,6 @@ describe('ReactIncrementalErrorHandling', () => {
assertLog([
'RethrowErrorBoundary render',
'BrokenRender',

// React retries one more time
'RethrowErrorBoundary render',
'BrokenRender',

// Errored again on retry. Now handle it.
'RethrowErrorBoundary componentDidCatch',
]);
expect(ReactNoop).toMatchRenderedOutput(null);
Expand Down Expand Up @@ -1892,27 +1868,25 @@ describe('ReactIncrementalErrorHandling', () => {
root.render(<Oops />);
});

await act(async () => {
// Schedule a default pri and a low pri update on the root.
root.render(<Oops />);
React.startTransition(() => {
root.render(<AllGood />);
});
await expect(
act(async () => {
// Schedule a default pri and a low pri update on the root.
root.render(<Oops />);
React.startTransition(() => {
root.render(<AllGood />);
});

// Render through just the default pri update. The low pri update remains on
// the queue.
await waitFor(['Everything is fine.']);
// Render through the default pri and low pri update.
await waitFor(['Everything is fine.']);

// Schedule a discrete update on a child that triggers an error.
// The root should capture this error. But since there's still a pending
// update on the root, the error should be suppressed.
ReactNoop.discreteUpdates(() => {
setShouldThrow(true);
});
});
// Should render the final state without throwing the error.
assertLog(['Everything is fine.']);
expect(root).toMatchRenderedOutput('Everything is fine.');
// Schedule a discrete update on a child that triggers an error.
ReactNoop.discreteUpdates(() => {
setShouldThrow(true);
});
}),
).rejects.toThrow('Oops');
assertLog([]);
expect(root).toMatchRenderedOutput(null);
});

it("does not infinite loop if there's a render phase update in the same render as an error", async () => {
Expand Down
Expand Up @@ -250,16 +250,7 @@ describe('ReactIncrementalErrorLogging', () => {
</ErrorBoundary>,
);
await waitForAll(
[
'render: 0',

'render: 1',

// Retry one more time before handling error
'render: 1',

'componentWillUnmount: 0',
].filter(Boolean),
['render: 0', 'render: 1', 'componentWillUnmount: 0'].filter(Boolean),
);

expect(console.error).toHaveBeenCalledTimes(1);
Expand Down
15 changes: 10 additions & 5 deletions packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee
Expand Up @@ -56,11 +56,16 @@ describe 'ReactCoffeeScriptClass', ->
expect(->
ReactDOM.flushSync ->
root.render React.createElement(Foo)
).toErrorDev([
# A failed component renders twice in DEV in concurrent mode
'No `render` method found on the Foo instance',
'No `render` method found on the Foo instance',
])
).toErrorDev(
if featureFlags.enableUnifiedSyncLane
['No `render` method found on the Foo instance']
else
[
'No `render` method found on the Foo instance',
# Retry in non-blocking updates
'No `render` method found on the Foo instance',
]
)
window.removeEventListener 'error', errorHandler;
expect(caughtErrors).toEqual([
expect.objectContaining(
Expand Down
23 changes: 16 additions & 7 deletions packages/react/src/__tests__/ReactES6Class-test.js
Expand Up @@ -13,6 +13,7 @@ let PropTypes;
let React;
let ReactDOM;
let ReactDOMClient;
let ReactFeatureFlags;

describe('ReactES6Class', () => {
let container;
Expand All @@ -30,6 +31,7 @@ describe('ReactES6Class', () => {
React = require('react');
ReactDOM = require('react-dom');
ReactDOMClient = require('react-dom/client');
ReactFeatureFlags = require('shared/ReactFeatureFlags');
container = document.createElement('div');
root = ReactDOMClient.createRoot(container);
attachedListener = null;
Expand Down Expand Up @@ -69,13 +71,20 @@ describe('ReactES6Class', () => {
try {
expect(() => {
ReactDOM.flushSync(() => root.render(<Foo />));
}).toErrorDev([
// A failed component renders twice in DEV in concurrent mode
'Warning: No `render` method found on the Foo instance: ' +
'you may have forgotten to define `render`.',
'Warning: No `render` method found on the Foo instance: ' +
'you may have forgotten to define `render`.',
]);
}).toErrorDev(
ReactFeatureFlags.enableUnifiedSyncLane
? [
'Warning: No `render` method found on the Foo instance: ' +
'you may have forgotten to define `render`.',
]
: [
'Warning: No `render` method found on the Foo instance: ' +
'you may have forgotten to define `render`.',
// Retry in non-blocking updates
'Warning: No `render` method found on the Foo instance: ' +
'you may have forgotten to define `render`.',
],
);
} finally {
window.removeEventListener('error', errorHandler);
}
Expand Down
21 changes: 14 additions & 7 deletions packages/react/src/__tests__/ReactTypeScriptClass-test.ts
Expand Up @@ -337,13 +337,20 @@ describe('ReactTypeScriptClass', function() {
try {
expect(() => {
ReactDOM.flushSync(() => root.render(React.createElement(Empty)))
}).toErrorDev([
// A failed component renders twice in DEV in concurrent mode
'Warning: No `render` method found on the Empty instance: ' +
'you may have forgotten to define `render`.',
'Warning: No `render` method found on the Empty instance: ' +
'you may have forgotten to define `render`.',
]);
}).toErrorDev(
ReactFeatureFlags.enableUnifiedSyncLane
? [
'Warning: No `render` method found on the Empty instance: ' +
'you may have forgotten to define `render`.'
]
: [
'Warning: No `render` method found on the Empty instance: ' +
'you may have forgotten to define `render`.',
// Retry in non-blocking updates
'Warning: No `render` method found on the Empty instance: ' +
'you may have forgotten to define `render`.',
]
);
} finally {
window.removeEventListener('error', errorHandler);
}
Expand Down
Expand Up @@ -588,11 +588,12 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
}

assertLog(
gate(flags => flags.enableUseSyncExternalStoreShim)
gate(flags => flags.enableUseSyncExternalStoreShim) ||
gate(flags => flags.enableUnifiedSyncLane)
? ['Error in getSnapshot']
: [
'Error in getSnapshot',
// In a concurrent root, React renders a second time to attempt to
// In a non-blocking update, React renders a second time to attempt to
// recover from the error.
'Error in getSnapshot',
],
Expand Down

0 comments on commit cd59515

Please sign in to comment.