Reconciler: Don't retry synchronous render #28810
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
eps1lon
commented
Apr 10, 2024
| @@ -1066,10 +1066,10 @@ describe(`onRender`, () => { | |||
|
|
|||
| // start time: 5 initially + 14 of work | |||
| // Add an additional 3 (ThrowsError) if we replayed the failed work | |||
| expect(mountCall[4]).toBe(19); | |||
| expect(mountCall[4]).toBe(5); | |||
There was a problem hiding this comment.
TODO: Update comments
|
Comparing: 5b903cd...00d183b Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
eps1lon
force-pushed
the
error-boundary-double-commit-26518
branch
2 times, most recently
from
f5663e5
to
ced79ba
Compare
sebmarkbage
reviewed
Apr 11, 2024
| // Check if something threw | ||
| if (exitStatus === RootErrored) { | ||
| // Check if something threw. | ||
| // We only need to retry during hydration or concurrent render. |
There was a problem hiding this comment.
We shouldn't even do it for those because we should refactor the Root recovery to work the same way as Suspense recovery.
eps1lon
force-pushed
the
error-boundary-double-commit-26518
branch
from
ced79ba
to
cd59515
Compare
eps1lon
commented
Apr 21, 2024
Comment on lines
1808
to
1908
| 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); |
There was a problem hiding this comment.
Needs discussion. Might be fine since we already rendered the low-pri update before we error. So last render wins?
eps1lon
force-pushed
the
error-boundary-double-commit-26518
branch
3 times, most recently
from
b3a570c
to
2736d41
Compare
eps1lon
force-pushed
the
error-boundary-double-commit-26518
branch
from
2736d41
to
00d183b
Compare
eps1lon
commented
Apr 22, 2024
Comment on lines
-279
to
+285
| expect(root).toMatchRenderedOutput('Count 0 (n=1)'); | ||
| expect(Text).toBeCalledTimes(2); | ||
| if (gate(flags => flags.enableUnifiedSyncLane)) { | ||
| expect(root).toMatchRenderedOutput(<h1>Something went wrong.</h1>); | ||
| expect(Text).toBeCalledTimes(1); | ||
| } else { | ||
| expect(root).toMatchRenderedOutput('Count 0 (n=1)'); | ||
| expect(Text).toBeCalledTimes(2); | ||
| } |
There was a problem hiding this comment.
These assertions implicitly tested the error recovery. In concurrent updates we'd retry and this component only throws once:
if (shouldFail) {
shouldFail = false;
throw new Error('failed');
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
In #13041 we started retrying the render synchronously if a render threw during a concurrent render. However, this also applied to synchronous renders which should help very rarely since we don't support mutations during render. The original PR even explicitly said that a synchronous render should not be retried. We also didn't used to retry in legacy roots: https://react-error-recovery-showcase.vercel.app/
A lot of tests asserted on the retry even though their render was sync i.e. not wrapped in
startTransition.I originally caught this while investigating duplicate host instance creation in error boundary fallbacks. But this is expected by proxy of having error recovery for sync renders. Host instances are created during render phase.
How did you test this change?
[ ] Codesandbox in Bug: Elements in ErrorBoundary are created twice, one of them becomes an orphan #26518 no longer callsWe also create host instances twice when recovering from an error during concurrent render. We just never mutate the DOM twice. Creating host instances during render is intentional and documenteddocument.createElementtwice