From f00c02d8e591e7f6cec6a27836703fefe6c270cf 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 66b6420710bb8..44b4c239423a1 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.new.js +++ b/packages/react-reconciler/src/ReactFiberThrow.new.js @@ -63,6 +63,7 @@ import { import { renderDidError, renderDidErrorUncaught, + renderDidSuspendDelayIfPossible, queueConcurrentError, onUncaughtError, markLegacyErrorBoundaryAsFailed, @@ -80,6 +81,7 @@ import { includesSomeLane, mergeLanes, pickArbitraryLane, + includesOnlyTransitions, } from './ReactFiberLane.new'; import { getIsHydrating, @@ -167,12 +169,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, @@ -185,34 +182,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 @@ -472,24 +474,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 4b3b70707d0cc..ee39c05100238 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.old.js +++ b/packages/react-reconciler/src/ReactFiberThrow.old.js @@ -63,6 +63,7 @@ import { import { renderDidError, renderDidErrorUncaught, + renderDidSuspendDelayIfPossible, queueConcurrentError, onUncaughtError, markLegacyErrorBoundaryAsFailed, @@ -80,6 +81,7 @@ import { includesSomeLane, mergeLanes, pickArbitraryLane, + includesOnlyTransitions, } from './ReactFiberLane.old'; import { getIsHydrating, @@ -167,12 +169,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, @@ -185,34 +182,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 @@ -472,24 +474,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 bb002b9e71b3a..a43c021d987e5 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 7f161513a4afa..e0cf7cc2f0fcc 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 be3af3df5b7bd..28be39587e8ac 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -251,14 +251,15 @@ const RenderContext = /* */ 0b0010; const CommitContext = /* */ 0b0100; export const RetryAfterError = /* */ 0b1000; -type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5 | 6; -const RootIncomplete = 0; +type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7; +const RootInProgress = 0; const RootErroredInternal = 1; const RootErroredUncaught = 2; const RootErrored = 3; const RootSuspended = 4; const RootSuspendedWithDelay = 5; const RootCompleted = 6; +const RootDidNotComplete = 7; // 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; // An internal error that can't be handled using the normal error handling path. // This happens when there's a bug within React itself. let workInProgressInternalError: mixed = null; @@ -819,7 +820,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { let exitStatus = shouldTimeSlice ? renderRootConcurrent(root, lanes) : renderRootSync(root, lanes); - if (exitStatus !== RootIncomplete) { + if (exitStatus !== RootInProgress) { if (exitStatus === RootErrored || exitStatus === RootErroredUncaught) { // If something threw an error, try rendering one more time. We'll // render synchronously to block concurrent data mutations, and we'll @@ -839,45 +840,58 @@ function performConcurrentWorkOnRoot(root, didTimeout) { throw internalError; } - // 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 || exitStatus === RootErroredUncaught) { - 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 || exitStatus === RootErroredUncaught) { + 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 === RootErroredInternal) { + const internalError = workInProgressInternalError; + prepareFreshStack(root, NoLanes); + markRootSuspended(root, lanes); + ensureRootIsScheduled(root, now()); + throw internalError; } } - if (exitStatus === RootErroredInternal) { - const internalError = workInProgressInternalError; - prepareFreshStack(root, NoLanes); - markRootSuspended(root, lanes); - ensureRootIsScheduled(root, now()); - throw internalError; - } - } - // 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()); @@ -934,7 +948,7 @@ export function queueRecoverableErrors(errors: Array) { function finishConcurrentRender(root, exitStatus, lanes) { switch (exitStatus) { - case RootIncomplete: + case RootInProgress: case RootErroredInternal: { throw new Error('Root did not complete. This is a bug in React.'); } @@ -1176,6 +1190,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); @@ -1371,7 +1389,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRoot = root; workInProgress = createWorkInProgress(root.current, null); workInProgressRootRenderLanes = subtreeRenderLanes = workInProgressRootIncludedLanes = lanes; - workInProgressRootExitStatus = RootIncomplete; + workInProgressRootExitStatus = RootInProgress; workInProgressInternalError = null; workInProgressRootSkippedLanes = NoLanes; workInProgressRootInterleavedUpdatedLanes = NoLanes; @@ -1501,14 +1519,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 || workInProgressRootExitStatus === RootErroredUncaught @@ -1559,7 +1577,7 @@ export function queueConcurrentError(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) { @@ -1709,7 +1727,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { if (enableSchedulingProfiler) { markRenderYielded(); } - return RootIncomplete; + return RootInProgress; } else { // Completed the tree. if (enableSchedulingProfiler) { @@ -1834,6 +1852,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; } } @@ -1850,7 +1873,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 49fad4ea86eb9..93abf330e8883 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -251,14 +251,15 @@ const RenderContext = /* */ 0b0010; const CommitContext = /* */ 0b0100; export const RetryAfterError = /* */ 0b1000; -type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5 | 6; -const RootIncomplete = 0; +type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7; +const RootInProgress = 0; const RootErroredInternal = 1; const RootErroredUncaught = 2; const RootErrored = 3; const RootSuspended = 4; const RootSuspendedWithDelay = 5; const RootCompleted = 6; +const RootDidNotComplete = 7; // 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; // An internal error that can't be handled using the normal error handling path. // This happens when there's a bug within React itself. let workInProgressInternalError: mixed = null; @@ -819,7 +820,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { let exitStatus = shouldTimeSlice ? renderRootConcurrent(root, lanes) : renderRootSync(root, lanes); - if (exitStatus !== RootIncomplete) { + if (exitStatus !== RootInProgress) { if (exitStatus === RootErrored || exitStatus === RootErroredUncaught) { // If something threw an error, try rendering one more time. We'll // render synchronously to block concurrent data mutations, and we'll @@ -839,45 +840,58 @@ function performConcurrentWorkOnRoot(root, didTimeout) { throw internalError; } - // 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 || exitStatus === RootErroredUncaught) { - 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 || exitStatus === RootErroredUncaught) { + 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 === RootErroredInternal) { + const internalError = workInProgressInternalError; + prepareFreshStack(root, NoLanes); + markRootSuspended(root, lanes); + ensureRootIsScheduled(root, now()); + throw internalError; } } - if (exitStatus === RootErroredInternal) { - const internalError = workInProgressInternalError; - prepareFreshStack(root, NoLanes); - markRootSuspended(root, lanes); - ensureRootIsScheduled(root, now()); - throw internalError; - } - } - // 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()); @@ -934,7 +948,7 @@ export function queueRecoverableErrors(errors: Array) { function finishConcurrentRender(root, exitStatus, lanes) { switch (exitStatus) { - case RootIncomplete: + case RootInProgress: case RootErroredInternal: { throw new Error('Root did not complete. This is a bug in React.'); } @@ -1176,6 +1190,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); @@ -1371,7 +1389,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) { workInProgressRoot = root; workInProgress = createWorkInProgress(root.current, null); workInProgressRootRenderLanes = subtreeRenderLanes = workInProgressRootIncludedLanes = lanes; - workInProgressRootExitStatus = RootIncomplete; + workInProgressRootExitStatus = RootInProgress; workInProgressInternalError = null; workInProgressRootSkippedLanes = NoLanes; workInProgressRootInterleavedUpdatedLanes = NoLanes; @@ -1501,14 +1519,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 || workInProgressRootExitStatus === RootErroredUncaught @@ -1559,7 +1577,7 @@ export function queueConcurrentError(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) { @@ -1709,7 +1727,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { if (enableSchedulingProfiler) { markRenderYielded(); } - return RootIncomplete; + return RootInProgress; } else { // Completed the tree. if (enableSchedulingProfiler) { @@ -1834,6 +1852,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; } } @@ -1850,7 +1873,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 2b65c3fcc826b..78e489865a89c 100644 --- a/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js +++ b/packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js @@ -431,4 +431,138 @@ describe('ReactConcurrentErrorRecovery', () => { // onRecoverableError is not called this time expect(Scheduler).toHaveYielded([]); }); + + // @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!'); + }, + ); });