From c14ca1f2373b4347260c520ffd5d11185820b8c6 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 10 Feb 2022 02:49:47 -0500 Subject: [PATCH] Allow suspending outside a Suspense boundary (If the update is wrapped in startTransition) Currently you're not allowed to suspend outside of a Suspense boundary. We throw an error: > A React component suspended while rendering, but no fallback UI was specified We treat this case like an error because discrete renders are expected to finish synchronously to maintain consistency with external state. However, during a concurrent transition (startTransition), what we can do instead is treat this case like a refresh transition: suspend the commit without showing a fallback. The behavior is roughly as if there were a built-in Suspense boundary at the root of the app with unstable_avoidThisFallback enabled. Conceptually it's very similar because during hydration you're already showing server-rendered UI; there's no need to replace that with a fallback when something suspends. --- .../src/ReactFiberThrow.new.js | 95 ++++++++----- .../src/ReactFiberThrow.old.js | 95 ++++++++----- .../src/ReactFiberUnwindWork.new.js | 19 +-- .../src/ReactFiberUnwindWork.old.js | 19 +-- .../src/ReactFiberWorkLoop.new.js | 117 +++++++++------ .../src/ReactFiberWorkLoop.old.js | 117 +++++++++------ .../ReactConcurrentErrorRecovery-test.js | 134 ++++++++++++++++++ 7 files changed, 414 insertions(+), 182 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberThrow.new.js b/packages/react-reconciler/src/ReactFiberThrow.new.js index 6a0c70f8e6d0..824f89e96770 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -62,6 +62,7 @@ import { } from './ReactFiberSuspenseContext.new'; import { renderDidError, + renderDidSuspendDelayIfPossible, onUncaughtError, markLegacyErrorBoundaryAsFailed, isAlreadyFailedLegacyErrorBoundary, @@ -78,6 +79,7 @@ import { includesSomeLane, mergeLanes, pickArbitraryLane, + includesOnlyTransitions, } from './ReactFiberLane.new'; import { getIsHydrating, @@ -165,12 +167,7 @@ function createClassErrorUpdate( return update; } -function attachWakeableListeners( - suspenseBoundary: Fiber, - root: FiberRoot, - wakeable: Wakeable, - lanes: Lanes, -) { +function attachPingListener(root: FiberRoot, wakeable: Wakeable, lanes: Lanes) { // Attach a ping listener // // The data might resolve before we have a chance to commit the fallback. Or, @@ -183,34 +180,39 @@ function attachWakeableListeners( // // We only need to do this in concurrent mode. Legacy Suspense always // commits fallbacks synchronously, so there are no pings. - if (suspenseBoundary.mode & ConcurrentMode) { - let pingCache = root.pingCache; - let threadIDs; - if (pingCache === null) { - pingCache = root.pingCache = new PossiblyWeakMap(); + let pingCache = root.pingCache; + let threadIDs; + if (pingCache === null) { + pingCache = root.pingCache = new PossiblyWeakMap(); + threadIDs = new Set(); + pingCache.set(wakeable, threadIDs); + } else { + threadIDs = pingCache.get(wakeable); + if (threadIDs === undefined) { threadIDs = new Set(); pingCache.set(wakeable, threadIDs); - } else { - threadIDs = pingCache.get(wakeable); - if (threadIDs === undefined) { - threadIDs = new Set(); - pingCache.set(wakeable, threadIDs); - } } - if (!threadIDs.has(lanes)) { - // Memoize using the thread ID to prevent redundant listeners. - threadIDs.add(lanes); - const ping = pingSuspendedRoot.bind(null, root, wakeable, lanes); - if (enableUpdaterTracking) { - if (isDevToolsPresent) { - // If we have pending work still, restore the original updaters - restorePendingUpdaters(root, lanes); - } + } + if (!threadIDs.has(lanes)) { + // Memoize using the thread ID to prevent redundant listeners. + threadIDs.add(lanes); + const ping = pingSuspendedRoot.bind(null, root, wakeable, lanes); + if (enableUpdaterTracking) { + if (isDevToolsPresent) { + // If we have pending work still, restore the original updaters + restorePendingUpdaters(root, lanes); } - wakeable.then(ping, ping); } + wakeable.then(ping, ping); } +} +function attachRetryListener( + suspenseBoundary: Fiber, + root: FiberRoot, + wakeable: Wakeable, + lanes: Lanes, +) { // Retry listener // // If the fallback does commit, we need to attach a different type of @@ -470,24 +472,47 @@ function throwException( root, rootRenderLanes, ); - attachWakeableListeners( - suspenseBoundary, - root, - wakeable, - rootRenderLanes, - ); + // We only attach ping listeners in concurrent mode. Legacy Suspense always + // commits fallbacks synchronously, so there are no pings. + if (suspenseBoundary.mode & ConcurrentMode) { + attachPingListener(root, wakeable, rootRenderLanes); + } + attachRetryListener(suspenseBoundary, root, wakeable, rootRenderLanes); return; } else { - // No boundary was found. Fallthrough to error mode. + // No boundary was found. If we're inside startTransition, this is OK. + // We can suspend and wait for more data to arrive. + + if (includesOnlyTransitions(rootRenderLanes)) { + // This is a transition. Suspend. Since we're not activating a Suspense + // boundary, this will unwind all the way to the root without performing + // a second pass to render a fallback. (This is arguably how refresh + // transitions should work, too, since we're not going to commit the + // fallbacks anyway.) + attachPingListener(root, wakeable, rootRenderLanes); + renderDidSuspendDelayIfPossible(); + return; + } + + // We're not in a transition. We treat this case like an error because + // discrete renders are expected to finish synchronously to maintain + // consistency with external state. + // TODO: This will error during non-transition concurrent renders, too. + // But maybe it shouldn't? + // TODO: We should never call getComponentNameFromFiber in production. // Log a warning or something to prevent us from accidentally bundling it. - value = new Error( + const uncaughtSuspenseError = new Error( (getComponentNameFromFiber(sourceFiber) || 'A React component') + ' suspended while rendering, but no fallback UI was specified.\n' + '\n' + 'Add a component higher in the tree to ' + 'provide a loading indicator or placeholder to display.', ); + + // If we're outside a transition, fall through to the regular error path. + // The error will be caught by the nearest suspense boundary. + value = uncaughtSuspenseError; } } else { // This is a regular error, not a Suspense wakeable. diff --git a/packages/react-reconciler/src/ReactFiberThrow.old.js b/packages/react-reconciler/src/ReactFiberThrow.old.js index 21ab03f4ac92..86a43a4dcaf5 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -62,6 +62,7 @@ import { } from './ReactFiberSuspenseContext.old'; import { renderDidError, + renderDidSuspendDelayIfPossible, onUncaughtError, markLegacyErrorBoundaryAsFailed, isAlreadyFailedLegacyErrorBoundary, @@ -78,6 +79,7 @@ import { includesSomeLane, mergeLanes, pickArbitraryLane, + includesOnlyTransitions, } from './ReactFiberLane.old'; import { getIsHydrating, @@ -165,12 +167,7 @@ function createClassErrorUpdate( return update; } -function attachWakeableListeners( - suspenseBoundary: Fiber, - root: FiberRoot, - wakeable: Wakeable, - lanes: Lanes, -) { +function attachPingListener(root: FiberRoot, wakeable: Wakeable, lanes: Lanes) { // Attach a ping listener // // The data might resolve before we have a chance to commit the fallback. Or, @@ -183,34 +180,39 @@ function attachWakeableListeners( // // We only need to do this in concurrent mode. Legacy Suspense always // commits fallbacks synchronously, so there are no pings. - if (suspenseBoundary.mode & ConcurrentMode) { - let pingCache = root.pingCache; - let threadIDs; - if (pingCache === null) { - pingCache = root.pingCache = new PossiblyWeakMap(); + let pingCache = root.pingCache; + let threadIDs; + if (pingCache === null) { + pingCache = root.pingCache = new PossiblyWeakMap(); + threadIDs = new Set(); + pingCache.set(wakeable, threadIDs); + } else { + threadIDs = pingCache.get(wakeable); + if (threadIDs === undefined) { threadIDs = new Set(); pingCache.set(wakeable, threadIDs); - } else { - threadIDs = pingCache.get(wakeable); - if (threadIDs === undefined) { - threadIDs = new Set(); - pingCache.set(wakeable, threadIDs); - } } - if (!threadIDs.has(lanes)) { - // Memoize using the thread ID to prevent redundant listeners. - threadIDs.add(lanes); - const ping = pingSuspendedRoot.bind(null, root, wakeable, lanes); - if (enableUpdaterTracking) { - if (isDevToolsPresent) { - // If we have pending work still, restore the original updaters - restorePendingUpdaters(root, lanes); - } + } + if (!threadIDs.has(lanes)) { + // Memoize using the thread ID to prevent redundant listeners. + threadIDs.add(lanes); + const ping = pingSuspendedRoot.bind(null, root, wakeable, lanes); + if (enableUpdaterTracking) { + if (isDevToolsPresent) { + // If we have pending work still, restore the original updaters + restorePendingUpdaters(root, lanes); } - wakeable.then(ping, ping); } + wakeable.then(ping, ping); } +} +function attachRetryListener( + suspenseBoundary: Fiber, + root: FiberRoot, + wakeable: Wakeable, + lanes: Lanes, +) { // Retry listener // // If the fallback does commit, we need to attach a different type of @@ -470,24 +472,47 @@ function throwException( root, rootRenderLanes, ); - attachWakeableListeners( - suspenseBoundary, - root, - wakeable, - rootRenderLanes, - ); + // We only attach ping listeners in concurrent mode. Legacy Suspense always + // commits fallbacks synchronously, so there are no pings. + if (suspenseBoundary.mode & ConcurrentMode) { + attachPingListener(root, wakeable, rootRenderLanes); + } + attachRetryListener(suspenseBoundary, root, wakeable, rootRenderLanes); return; } else { - // No boundary was found. Fallthrough to error mode. + // No boundary was found. If we're inside startTransition, this is OK. + // We can suspend and wait for more data to arrive. + + if (includesOnlyTransitions(rootRenderLanes)) { + // This is a transition. Suspend. Since we're not activating a Suspense + // boundary, this will unwind all the way to the root without performing + // a second pass to render a fallback. (This is arguably how refresh + // transitions should work, too, since we're not going to commit the + // fallbacks anyway.) + attachPingListener(root, wakeable, rootRenderLanes); + renderDidSuspendDelayIfPossible(); + return; + } + + // We're not in a transition. We treat this case like an error because + // discrete renders are expected to finish synchronously to maintain + // consistency with external state. + // TODO: This will error during non-transition concurrent renders, too. + // But maybe it shouldn't? + // TODO: We should never call getComponentNameFromFiber in production. // Log a warning or something to prevent us from accidentally bundling it. - value = new Error( + const uncaughtSuspenseError = new Error( (getComponentNameFromFiber(sourceFiber) || 'A React component') + ' suspended while rendering, but no fallback UI was specified.\n' + '\n' + 'Add a component higher in the tree to ' + 'provide a loading indicator or placeholder to display.', ); + + // If we're outside a transition, fall through to the regular error path. + // The error will be caught by the nearest suspense boundary. + value = uncaughtSuspenseError; } } else { // This is a regular error, not a Suspense wakeable. diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.new.js b/packages/react-reconciler/src/ReactFiberUnwindWork.new.js index bb002b9e71b3..a43c021d987e 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.new.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.new.js @@ -89,16 +89,17 @@ function unwindWork(workInProgress: Fiber, renderLanes: Lanes) { popTopLevelLegacyContextObject(workInProgress); resetMutableSourceWorkInProgressVersions(); const flags = workInProgress.flags; - - if ((flags & DidCapture) !== NoFlags) { - throw new Error( - 'The root failed to unmount after an error. This is likely a bug in ' + - 'React. Please file an issue.', - ); + if ( + (flags & ShouldCapture) !== NoFlags && + (flags & DidCapture) === NoFlags + ) { + // There was an error during render that wasn't captured by a suspense + // boundary. Do a second pass on the root to unmount the children. + workInProgress.flags = (flags & ~ShouldCapture) | DidCapture; + return workInProgress; } - - workInProgress.flags = (flags & ~ShouldCapture) | DidCapture; - return workInProgress; + // We unwound to the root without completing it. Exit. + return null; } case HostComponent: { // TODO: popHydrationState diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.old.js b/packages/react-reconciler/src/ReactFiberUnwindWork.old.js index 7f161513a4af..e0cf7cc2f0fc 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.old.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.old.js @@ -89,16 +89,17 @@ function unwindWork(workInProgress: Fiber, renderLanes: Lanes) { popTopLevelLegacyContextObject(workInProgress); resetMutableSourceWorkInProgressVersions(); const flags = workInProgress.flags; - - if ((flags & DidCapture) !== NoFlags) { - throw new Error( - 'The root failed to unmount after an error. This is likely a bug in ' + - 'React. Please file an issue.', - ); + if ( + (flags & ShouldCapture) !== NoFlags && + (flags & DidCapture) === NoFlags + ) { + // There was an error during render that wasn't captured by a suspense + // boundary. Do a second pass on the root to unmount the children. + workInProgress.flags = (flags & ~ShouldCapture) | DidCapture; + return workInProgress; } - - workInProgress.flags = (flags & ~ShouldCapture) | DidCapture; - return workInProgress; + // We unwound to the root without completing it. Exit. + return null; } case HostComponent: { // TODO: popHydrationState diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index ece137e8477e..1bcda2599896 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -252,13 +252,14 @@ const RenderContext = /* */ 0b0010; const CommitContext = /* */ 0b0100; export const RetryAfterError = /* */ 0b1000; -type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5; -const RootIncomplete = 0; +type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5 | 6; +const RootInProgress = 0; const RootFatalErrored = 1; const RootErrored = 2; const RootSuspended = 3; const RootSuspendedWithDelay = 4; const RootCompleted = 5; +const RootDidNotComplete = 6; // Describes where we are in the React execution stack let executionContext: ExecutionContext = NoContext; @@ -281,7 +282,7 @@ export let subtreeRenderLanes: Lanes = NoLanes; const subtreeRenderLanesCursor: StackCursor = createCursor(NoLanes); // Whether to root completed, errored, suspended, etc. -let workInProgressRootExitStatus: RootExitStatus = RootIncomplete; +let workInProgressRootExitStatus: RootExitStatus = RootInProgress; // A fatal error, if one is thrown let workInProgressRootFatalError: mixed = null; // "Included" lanes refer to lanes that were worked on during this render. It's @@ -818,7 +819,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { let exitStatus = shouldTimeSlice ? renderRootConcurrent(root, lanes) : renderRootSync(root, lanes); - if (exitStatus !== RootIncomplete) { + if (exitStatus !== RootInProgress) { if (exitStatus === RootErrored) { // If something threw an error, try rendering one more time. We'll // render synchronously to block concurrent data mutations, and we'll @@ -838,45 +839,58 @@ function performConcurrentWorkOnRoot(root, didTimeout) { throw fatalError; } - // Check if this render may have yielded to a concurrent event, and if so, - // confirm that any newly rendered stores are consistent. - // TODO: It's possible that even a concurrent render may never have yielded - // to the main thread, if it was fast enough, or if it expired. We could - // skip the consistency check in that case, too. - const renderWasConcurrent = !includesBlockingLane(root, lanes); - const finishedWork: Fiber = (root.current.alternate: any); - if ( - renderWasConcurrent && - !isRenderConsistentWithExternalStores(finishedWork) - ) { - // A store was mutated in an interleaved event. Render again, - // synchronously, to block further mutations. - exitStatus = renderRootSync(root, lanes); - - // We need to check again if something threw - if (exitStatus === RootErrored) { - const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root); - if (errorRetryLanes !== NoLanes) { - lanes = errorRetryLanes; - exitStatus = recoverFromConcurrentError(root, errorRetryLanes); - // We assume the tree is now consistent because we didn't yield to any - // concurrent events. + if (exitStatus === RootDidNotComplete) { + // The render unwound without completing the tree. This happens in special + // cases where need to exit the current render without producing a + // consistent tree or committing. + // + // This should only happen during a concurrent render, not a discrete or + // synchronous update. We should have already checked for this when we + // unwound the stack. + markRootSuspended(root, lanes); + } else { + // The render completed. + + // Check if this render may have yielded to a concurrent event, and if so, + // confirm that any newly rendered stores are consistent. + // TODO: It's possible that even a concurrent render may never have yielded + // to the main thread, if it was fast enough, or if it expired. We could + // skip the consistency check in that case, too. + const renderWasConcurrent = !includesBlockingLane(root, lanes); + const finishedWork: Fiber = (root.current.alternate: any); + if ( + renderWasConcurrent && + !isRenderConsistentWithExternalStores(finishedWork) + ) { + // A store was mutated in an interleaved event. Render again, + // synchronously, to block further mutations. + exitStatus = renderRootSync(root, lanes); + + // We need to check again if something threw + if (exitStatus === RootErrored) { + const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root); + if (errorRetryLanes !== NoLanes) { + lanes = errorRetryLanes; + exitStatus = recoverFromConcurrentError(root, errorRetryLanes); + // We assume the tree is now consistent because we didn't yield to any + // concurrent events. + } + } + if (exitStatus === RootFatalErrored) { + const fatalError = workInProgressRootFatalError; + prepareFreshStack(root, NoLanes); + markRootSuspended(root, lanes); + ensureRootIsScheduled(root, now()); + throw fatalError; } } - if (exitStatus === RootFatalErrored) { - const fatalError = workInProgressRootFatalError; - prepareFreshStack(root, NoLanes); - markRootSuspended(root, lanes); - ensureRootIsScheduled(root, now()); - throw fatalError; - } - } - // We now have a consistent tree. The next step is either to commit it, - // or, if something suspended, wait to commit it after a timeout. - root.finishedWork = finishedWork; - root.finishedLanes = lanes; - finishConcurrentRender(root, exitStatus, lanes); + // We now have a consistent tree. The next step is either to commit it, + // or, if something suspended, wait to commit it after a timeout. + root.finishedWork = finishedWork; + root.finishedLanes = lanes; + finishConcurrentRender(root, exitStatus, lanes); + } } ensureRootIsScheduled(root, now()); @@ -933,7 +947,7 @@ export function queueRecoverableErrors(errors: Array) { function finishConcurrentRender(root, exitStatus, lanes) { switch (exitStatus) { - case RootIncomplete: + case RootInProgress: case RootFatalErrored: { throw new Error('Root did not complete. This is a bug in React.'); } @@ -1149,6 +1163,10 @@ function performSyncWorkOnRoot(root) { throw fatalError; } + if (exitStatus === RootDidNotComplete) { + throw new Error('Root did not complete. This is a bug in React.'); + } + // We now have a consistent tree. Because this is a sync render, we // will commit it even if something suspended. const finishedWork: Fiber = (root.current.alternate: any); @@ -1344,7 +1362,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRoot = root; workInProgress = createWorkInProgress(root.current, null); workInProgressRootRenderLanes = subtreeRenderLanes = workInProgressRootIncludedLanes = lanes; - workInProgressRootExitStatus = RootIncomplete; + workInProgressRootExitStatus = RootInProgress; workInProgressRootFatalError = null; workInProgressRootSkippedLanes = NoLanes; workInProgressRootInterleavedUpdatedLanes = NoLanes; @@ -1474,14 +1492,14 @@ export function markSkippedUpdateLanes(lane: Lane | Lanes): void { } export function renderDidSuspend(): void { - if (workInProgressRootExitStatus === RootIncomplete) { + if (workInProgressRootExitStatus === RootInProgress) { workInProgressRootExitStatus = RootSuspended; } } export function renderDidSuspendDelayIfPossible(): void { if ( - workInProgressRootExitStatus === RootIncomplete || + workInProgressRootExitStatus === RootInProgress || workInProgressRootExitStatus === RootSuspended || workInProgressRootExitStatus === RootErrored ) { @@ -1522,7 +1540,7 @@ export function renderDidError(error: mixed) { export function renderHasNotSuspendedYet(): boolean { // If something errored or completed, we can't really be sure, // so those are false. - return workInProgressRootExitStatus === RootIncomplete; + return workInProgressRootExitStatus === RootInProgress; } function renderRootSync(root: FiberRoot, lanes: Lanes) { @@ -1672,7 +1690,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { if (enableSchedulingProfiler) { markRenderYielded(); } - return RootIncomplete; + return RootInProgress; } else { // Completed the tree. if (enableSchedulingProfiler) { @@ -1797,6 +1815,11 @@ function completeUnitOfWork(unitOfWork: Fiber): void { returnFiber.flags |= Incomplete; returnFiber.subtreeFlags = NoFlags; returnFiber.deletions = null; + } else { + // We've unwound all the way to the root. + workInProgressRootExitStatus = RootDidNotComplete; + workInProgress = null; + return; } } @@ -1813,7 +1836,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void { } while (completedWork !== null); // We've reached the root. - if (workInProgressRootExitStatus === RootIncomplete) { + if (workInProgressRootExitStatus === RootInProgress) { workInProgressRootExitStatus = RootCompleted; } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index b3164d5fe2dc..e9efe46bab13 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -252,13 +252,14 @@ const RenderContext = /* */ 0b0010; const CommitContext = /* */ 0b0100; export const RetryAfterError = /* */ 0b1000; -type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5; -const RootIncomplete = 0; +type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5 | 6; +const RootInProgress = 0; const RootFatalErrored = 1; const RootErrored = 2; const RootSuspended = 3; const RootSuspendedWithDelay = 4; const RootCompleted = 5; +const RootDidNotComplete = 6; // Describes where we are in the React execution stack let executionContext: ExecutionContext = NoContext; @@ -281,7 +282,7 @@ export let subtreeRenderLanes: Lanes = NoLanes; const subtreeRenderLanesCursor: StackCursor = createCursor(NoLanes); // Whether to root completed, errored, suspended, etc. -let workInProgressRootExitStatus: RootExitStatus = RootIncomplete; +let workInProgressRootExitStatus: RootExitStatus = RootInProgress; // A fatal error, if one is thrown let workInProgressRootFatalError: mixed = null; // "Included" lanes refer to lanes that were worked on during this render. It's @@ -818,7 +819,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { let exitStatus = shouldTimeSlice ? renderRootConcurrent(root, lanes) : renderRootSync(root, lanes); - if (exitStatus !== RootIncomplete) { + if (exitStatus !== RootInProgress) { if (exitStatus === RootErrored) { // If something threw an error, try rendering one more time. We'll // render synchronously to block concurrent data mutations, and we'll @@ -838,45 +839,58 @@ function performConcurrentWorkOnRoot(root, didTimeout) { throw fatalError; } - // Check if this render may have yielded to a concurrent event, and if so, - // confirm that any newly rendered stores are consistent. - // TODO: It's possible that even a concurrent render may never have yielded - // to the main thread, if it was fast enough, or if it expired. We could - // skip the consistency check in that case, too. - const renderWasConcurrent = !includesBlockingLane(root, lanes); - const finishedWork: Fiber = (root.current.alternate: any); - if ( - renderWasConcurrent && - !isRenderConsistentWithExternalStores(finishedWork) - ) { - // A store was mutated in an interleaved event. Render again, - // synchronously, to block further mutations. - exitStatus = renderRootSync(root, lanes); - - // We need to check again if something threw - if (exitStatus === RootErrored) { - const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root); - if (errorRetryLanes !== NoLanes) { - lanes = errorRetryLanes; - exitStatus = recoverFromConcurrentError(root, errorRetryLanes); - // We assume the tree is now consistent because we didn't yield to any - // concurrent events. + if (exitStatus === RootDidNotComplete) { + // The render unwound without completing the tree. This happens in special + // cases where need to exit the current render without producing a + // consistent tree or committing. + // + // This should only happen during a concurrent render, not a discrete or + // synchronous update. We should have already checked for this when we + // unwound the stack. + markRootSuspended(root, lanes); + } else { + // The render completed. + + // Check if this render may have yielded to a concurrent event, and if so, + // confirm that any newly rendered stores are consistent. + // TODO: It's possible that even a concurrent render may never have yielded + // to the main thread, if it was fast enough, or if it expired. We could + // skip the consistency check in that case, too. + const renderWasConcurrent = !includesBlockingLane(root, lanes); + const finishedWork: Fiber = (root.current.alternate: any); + if ( + renderWasConcurrent && + !isRenderConsistentWithExternalStores(finishedWork) + ) { + // A store was mutated in an interleaved event. Render again, + // synchronously, to block further mutations. + exitStatus = renderRootSync(root, lanes); + + // We need to check again if something threw + if (exitStatus === RootErrored) { + const errorRetryLanes = getLanesToRetrySynchronouslyOnError(root); + if (errorRetryLanes !== NoLanes) { + lanes = errorRetryLanes; + exitStatus = recoverFromConcurrentError(root, errorRetryLanes); + // We assume the tree is now consistent because we didn't yield to any + // concurrent events. + } + } + if (exitStatus === RootFatalErrored) { + const fatalError = workInProgressRootFatalError; + prepareFreshStack(root, NoLanes); + markRootSuspended(root, lanes); + ensureRootIsScheduled(root, now()); + throw fatalError; } } - if (exitStatus === RootFatalErrored) { - const fatalError = workInProgressRootFatalError; - prepareFreshStack(root, NoLanes); - markRootSuspended(root, lanes); - ensureRootIsScheduled(root, now()); - throw fatalError; - } - } - // We now have a consistent tree. The next step is either to commit it, - // or, if something suspended, wait to commit it after a timeout. - root.finishedWork = finishedWork; - root.finishedLanes = lanes; - finishConcurrentRender(root, exitStatus, lanes); + // We now have a consistent tree. The next step is either to commit it, + // or, if something suspended, wait to commit it after a timeout. + root.finishedWork = finishedWork; + root.finishedLanes = lanes; + finishConcurrentRender(root, exitStatus, lanes); + } } ensureRootIsScheduled(root, now()); @@ -933,7 +947,7 @@ export function queueRecoverableErrors(errors: Array) { function finishConcurrentRender(root, exitStatus, lanes) { switch (exitStatus) { - case RootIncomplete: + case RootInProgress: case RootFatalErrored: { throw new Error('Root did not complete. This is a bug in React.'); } @@ -1149,6 +1163,10 @@ function performSyncWorkOnRoot(root) { throw fatalError; } + if (exitStatus === RootDidNotComplete) { + throw new Error('Root did not complete. This is a bug in React.'); + } + // We now have a consistent tree. Because this is a sync render, we // will commit it even if something suspended. const finishedWork: Fiber = (root.current.alternate: any); @@ -1344,7 +1362,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRoot = root; workInProgress = createWorkInProgress(root.current, null); workInProgressRootRenderLanes = subtreeRenderLanes = workInProgressRootIncludedLanes = lanes; - workInProgressRootExitStatus = RootIncomplete; + workInProgressRootExitStatus = RootInProgress; workInProgressRootFatalError = null; workInProgressRootSkippedLanes = NoLanes; workInProgressRootInterleavedUpdatedLanes = NoLanes; @@ -1474,14 +1492,14 @@ export function markSkippedUpdateLanes(lane: Lane | Lanes): void { } export function renderDidSuspend(): void { - if (workInProgressRootExitStatus === RootIncomplete) { + if (workInProgressRootExitStatus === RootInProgress) { workInProgressRootExitStatus = RootSuspended; } } export function renderDidSuspendDelayIfPossible(): void { if ( - workInProgressRootExitStatus === RootIncomplete || + workInProgressRootExitStatus === RootInProgress || workInProgressRootExitStatus === RootSuspended || workInProgressRootExitStatus === RootErrored ) { @@ -1522,7 +1540,7 @@ export function renderDidError(error: mixed) { export function renderHasNotSuspendedYet(): boolean { // If something errored or completed, we can't really be sure, // so those are false. - return workInProgressRootExitStatus === RootIncomplete; + return workInProgressRootExitStatus === RootInProgress; } function renderRootSync(root: FiberRoot, lanes: Lanes) { @@ -1672,7 +1690,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { if (enableSchedulingProfiler) { markRenderYielded(); } - return RootIncomplete; + return RootInProgress; } else { // Completed the tree. if (enableSchedulingProfiler) { @@ -1797,6 +1815,11 @@ function completeUnitOfWork(unitOfWork: Fiber): void { returnFiber.flags |= Incomplete; returnFiber.subtreeFlags = NoFlags; returnFiber.deletions = null; + } else { + // We've unwound all the way to the root. + workInProgressRootExitStatus = RootDidNotComplete; + workInProgress = null; + return; } } @@ -1813,7 +1836,7 @@ function completeUnitOfWork(unitOfWork: Fiber): void { } while (completedWork !== null); // We've reached the root. - if (workInProgressRootExitStatus === RootIncomplete) { + if (workInProgressRootExitStatus === RootInProgress) { workInProgressRootExitStatus = RootCompleted; } } diff --git a/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js b/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js index b96882684592..4c44a31b2b12 100644 --- a/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js +++ b/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js @@ -398,4 +398,138 @@ describe('ReactConcurrentErrorRecovery', () => { // Now we can show the error boundary that's wrapped around B. expect(root).toMatchRenderedOutput('Oops!B2'); }); + + // @gate enableCache + test('suspending in the shell (outside a Suspense boundary) should not throw, warn, or log during a transition', async () => { + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + return {error}; + } + render() { + if (this.state.error !== null) { + return ; + } + return this.props.children; + } + } + + // The initial render suspends without a Suspense boundary. Since it's + // wrapped in startTransition, it suspends instead of erroring. + const root = ReactNoop.createRoot(); + await act(async () => { + startTransition(() => { + root.render(); + }); + }); + expect(Scheduler).toHaveYielded(['Suspend! [Async]']); + expect(root).toMatchRenderedOutput(null); + + // This also works if the suspended component is wrapped with an error + // boundary. (This is only interesting because when a component suspends + // outside of a transition, we throw an error, which can be captured by + // an error boundary. + await act(async () => { + startTransition(() => { + root.render( + + + , + ); + }); + }); + expect(Scheduler).toHaveYielded(['Suspend! [Async]']); + expect(root).toMatchRenderedOutput(null); + + // Continues rendering once data resolves + await act(async () => { + resolveText('Async'); + }); + expect(Scheduler).toHaveYielded(['Async']); + expect(root).toMatchRenderedOutput('Async'); + }); + + // @gate enableCache + test( + 'errors during a suspended transition at the shell should not force ' + + 'fallbacks to display (error then suspend)', + async () => { + // This is similar to the earlier test for errors that occur during + // a refresh transition. Suspending in the shell is conceptually the same + // as a refresh, but they have slightly different implementation paths. + + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + return {error}; + } + render() { + if (this.state.error !== null) { + return ( + + ); + } + return this.props.children; + } + } + + function Throws() { + throw new Error('Oops!'); + } + + // Suspend and throw in the same transition + const root = ReactNoop.createRoot(); + await act(async () => { + startTransition(() => { + root.render( + + + + , + ); + }); + }); + expect(Scheduler).toHaveYielded([ + 'Suspend! [Async]', + // TODO: Ideally we would skip this second render pass to render the + // error UI, since it's not going to commit anyway. The same goes for + // Suspense fallbacks during a refresh transition. + 'Caught an error: Oops!', + ]); + // The render suspended without committing or surfacing the error. + expect(root).toMatchRenderedOutput(null); + + // Try the reverse order, too: throw then suspend + await act(async () => { + startTransition(() => { + root.render( + + + + , + ); + }); + }); + expect(Scheduler).toHaveYielded([ + 'Suspend! [Async]', + 'Caught an error: Oops!', + ]); + expect(root).toMatchRenderedOutput(null); + + await act(async () => { + await resolveText('Async'); + }); + + expect(Scheduler).toHaveYielded([ + 'Async', + 'Caught an error: Oops!', + + // Try recovering from the error by rendering again synchronously + 'Async', + 'Caught an error: Oops!', + ]); + + expect(root).toMatchRenderedOutput('Caught an error: Oops!'); + }, + ); });