From b001ae323c0079b9af3b1b44df1ffab20bf07d56 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Fri, 25 Oct 2024 09:00:36 -0700 Subject: [PATCH] Revert "[Re-land] Make prerendering always non-blocking (#31268)" This reverts commit 6c4bbc783286bf6eebd9927cb52e8fec5ad4dd74. --- .../src/__tests__/ReactDOMFiberAsync-test.js | 2 +- .../react-reconciler/src/ReactFiberLane.js | 6 +- .../src/ReactFiberRootScheduler.js | 20 +- .../src/ReactFiberWorkLoop.js | 293 +++++++----------- .../src/__tests__/ReactDeferredValue-test.js | 12 - .../ReactSiblingPrerendering-test.js | 71 ----- 6 files changed, 126 insertions(+), 278 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index 027099d54707c..ee843996bef1c 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js @@ -744,7 +744,7 @@ describe('ReactDOMFiberAsync', () => { // Because it suspended, it remains on the current path expect(div.textContent).toBe('/path/a'); }); - assertLog(gate('enableSiblingPrerendering') ? ['Suspend! [/path/b]'] : []); + assertLog([]); await act(async () => { resolvePromise(); diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js index b98019046e3c2..7d6bfd16e8aa0 100644 --- a/packages/react-reconciler/src/ReactFiberLane.js +++ b/packages/react-reconciler/src/ReactFiberLane.js @@ -765,14 +765,12 @@ export function markRootSuspended( root: FiberRoot, suspendedLanes: Lanes, spawnedLane: Lane, - didAttemptEntireTree: boolean, + didSkipSuspendedSiblings: boolean, ) { - // TODO: Split this into separate functions for marking the root at the end of - // a render attempt versus suspending while the root is still in progress. root.suspendedLanes |= suspendedLanes; root.pingedLanes &= ~suspendedLanes; - if (enableSiblingPrerendering && didAttemptEntireTree) { + if (enableSiblingPrerendering && !didSkipSuspendedSiblings) { // Mark these lanes as warm so we know there's nothing else to work on. root.warmLanes |= suspendedLanes; } else { diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index 39f6f466ed983..9f267e8345e39 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -18,7 +18,6 @@ import { disableSchedulerTimeoutInWorkLoop, enableProfilerTimer, enableProfilerNestedUpdatePhase, - enableSiblingPrerendering, } from 'shared/ReactFeatureFlags'; import { NoLane, @@ -30,7 +29,6 @@ import { markStarvedLanesAsExpired, claimNextTransitionLane, getNextLanesToFlushSync, - checkIfRootIsPrerendering, } from './ReactFiberLane'; import { CommitContext, @@ -208,10 +206,7 @@ function flushSyncWorkAcrossRoots_impl( ? workInProgressRootRenderLanes : NoLanes, ); - if ( - includesSyncLane(nextLanes) && - !checkIfRootIsPrerendering(root, nextLanes) - ) { + if (includesSyncLane(nextLanes)) { // This root has pending sync work. Flush it now. didPerformSomeWork = true; performSyncWorkOnRoot(root, nextLanes); @@ -346,13 +341,7 @@ function scheduleTaskForRootDuringMicrotask( } // Schedule a new callback in the host environment. - if ( - includesSyncLane(nextLanes) && - // If we're prerendering, then we should use the concurrent work loop - // even if the lanes are synchronous, so that prerendering never blocks - // the main thread. - !(enableSiblingPrerendering && checkIfRootIsPrerendering(root, nextLanes)) - ) { + if (includesSyncLane(nextLanes)) { // Synchronous work is always flushed at the end of the microtask, so we // don't need to schedule an additional task. if (existingCallbackNode !== null) { @@ -386,10 +375,9 @@ function scheduleTaskForRootDuringMicrotask( let schedulerPriorityLevel; switch (lanesToEventPriority(nextLanes)) { - // Scheduler does have an "ImmediatePriority", but now that we use - // microtasks for sync work we no longer use that. Any sync work that - // reaches this path is meant to be time sliced. case DiscreteEventPriority: + schedulerPriorityLevel = ImmediateSchedulerPriority; + break; case ContinuousEventPriority: schedulerPriorityLevel = UserBlockingSchedulerPriority; break; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 204776075d49f..aa5884444063d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -765,12 +765,11 @@ export function scheduleUpdateOnFiber( // The incoming update might unblock the current render. Interrupt the // current attempt and restart from the top. prepareFreshStack(root, NoLanes); - const didAttemptEntireTree = false; markRootSuspended( root, workInProgressRootRenderLanes, workInProgressDeferredLane, - didAttemptEntireTree, + workInProgressRootDidSkipSuspendedSiblings, ); } @@ -833,12 +832,11 @@ export function scheduleUpdateOnFiber( // effect of interrupting the current render and switching to the update. // TODO: Make sure this doesn't override pings that happen while we've // already started rendering. - const didAttemptEntireTree = false; markRootSuspended( root, workInProgressRootRenderLanes, workInProgressDeferredLane, - didAttemptEntireTree, + workInProgressRootDidSkipSuspendedSiblings, ); } } @@ -900,120 +898,100 @@ export function performWorkOnRoot( // for too long ("expired" work, to prevent starvation), or we're in // sync-updates-by-default mode. const shouldTimeSlice = - (!forceSync && - !includesBlockingLane(lanes) && - !includesExpiredLane(root, lanes)) || - // If we're prerendering, then we should use the concurrent work loop - // even if the lanes are synchronous, so that prerendering never blocks - // the main thread. - // TODO: We should consider doing this whenever a sync lane is suspended, - // even for regular pings. - (enableSiblingPrerendering && checkIfRootIsPrerendering(root, lanes)); - + !forceSync && + !includesBlockingLane(lanes) && + !includesExpiredLane(root, lanes); let exitStatus = shouldTimeSlice ? renderRootConcurrent(root, lanes) - : renderRootSync(root, lanes, true); + : renderRootSync(root, lanes); - do { + if (exitStatus !== RootInProgress) { let renderWasConcurrent = shouldTimeSlice; - if (exitStatus === RootInProgress) { - // Render phase is still in progress. - if ( - enableSiblingPrerendering && - workInProgressRootIsPrerendering && - !shouldTimeSlice - ) { - // We're in prerendering mode, but time slicing is not enabled. This - // happens when something suspends during a synchronous update. Exit the - // the work loop. When we resume, we'll use the concurrent work loop so - // that prerendering is non-blocking. - // - // Mark the root as suspended. Usually we do this at the end of the - // render phase, but we do it here so that we resume in - // prerendering mode. - // TODO: Consider always calling markRootSuspended immediately. - // Needs to be *after* we attach a ping listener, though. - const didAttemptEntireTree = false; - markRootSuspended(root, lanes, NoLane, didAttemptEntireTree); - } - break; - } else 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. - const didAttemptEntireTree = !workInProgressRootDidSkipSuspendedSiblings; - markRootSuspended(root, lanes, NoLane, didAttemptEntireTree); - } 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 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, false); - // We assume the tree is now consistent because we didn't yield to any - // concurrent events. - renderWasConcurrent = false; - // Need to check the exit status again. - continue; - } - - // Check if something threw - if ( - (disableLegacyMode || root.tag !== LegacyRoot) && - exitStatus === RootErrored - ) { - const lanesThatJustErrored = lanes; - const errorRetryLanes = getLanesToRetrySynchronouslyOnError( + do { + 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. + markRootSuspended( root, - lanesThatJustErrored, + lanes, + NoLane, + workInProgressRootDidSkipSuspendedSiblings, ); - if (errorRetryLanes !== NoLanes) { - lanes = errorRetryLanes; - exitStatus = recoverFromConcurrentError( + } 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 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 assume the tree is now consistent because we didn't yield to any + // concurrent events. + renderWasConcurrent = false; + // Need to check the exit status again. + continue; + } + + // Check if something threw + if ( + (disableLegacyMode || root.tag !== LegacyRoot) && + exitStatus === RootErrored + ) { + const lanesThatJustErrored = lanes; + const errorRetryLanes = getLanesToRetrySynchronouslyOnError( root, lanesThatJustErrored, - errorRetryLanes, ); - renderWasConcurrent = false; - // Need to check the exit status again. - if (exitStatus !== RootErrored) { - // The root did not error this time. Restart the exit algorithm - // from the beginning. - // TODO: Refactor the exit algorithm to be less confusing. Maybe - // more branches + recursion instead of a loop. I think the only - // thing that causes it to be a loop is the RootDidNotComplete - // check. If that's true, then we don't need a loop/recursion - // at all. - continue; - } else { - // The root errored yet again. Proceed to commit the tree. + if (errorRetryLanes !== NoLanes) { + lanes = errorRetryLanes; + exitStatus = recoverFromConcurrentError( + root, + lanesThatJustErrored, + errorRetryLanes, + ); + renderWasConcurrent = false; + // Need to check the exit status again. + if (exitStatus !== RootErrored) { + // The root did not error this time. Restart the exit algorithm + // from the beginning. + // TODO: Refactor the exit algorithm to be less confusing. Maybe + // more branches + recursion instead of a loop. I think the only + // thing that causes it to be a loop is the RootDidNotComplete + // check. If that's true, then we don't need a loop/recursion + // at all. + continue; + } else { + // The root errored yet again. Proceed to commit the tree. + } } } - } - if (exitStatus === RootFatalErrored) { - prepareFreshStack(root, NoLanes); - // Since this is a fatal error, we're going to pretend we attempted - // the entire tree, to avoid scheduling a prerender. - const didAttemptEntireTree = true; - markRootSuspended(root, lanes, NoLane, didAttemptEntireTree); - break; - } + if (exitStatus === RootFatalErrored) { + prepareFreshStack(root, NoLanes); + markRootSuspended( + root, + lanes, + NoLane, + workInProgressRootDidSkipSuspendedSiblings, + ); + break; + } - // 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. - finishConcurrentRender(root, exitStatus, finishedWork, lanes); - } - break; - } while (true); + // 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. + finishConcurrentRender(root, exitStatus, finishedWork, lanes); + } + break; + } while (true); + } ensureRootIsScheduled(root); } @@ -1046,7 +1024,7 @@ function recoverFromConcurrentError( rootWorkInProgress.flags |= ForceClientRender; } - const exitStatus = renderRootSync(root, errorRetryLanes, false); + const exitStatus = renderRootSync(root, errorRetryLanes); if (exitStatus !== RootErrored) { // Successfully finished rendering on retry @@ -1130,13 +1108,11 @@ function finishConcurrentRender( // This is a transition, so we should exit without committing a // placeholder and without scheduling a timeout. Delay indefinitely // until we receive more data. - const didAttemptEntireTree = - !workInProgressRootDidSkipSuspendedSiblings; markRootSuspended( root, lanes, workInProgressDeferredLane, - didAttemptEntireTree, + workInProgressRootDidSkipSuspendedSiblings, ); return; } @@ -1192,13 +1168,11 @@ function finishConcurrentRender( // Don't bother with a very short suspense time. if (msUntilTimeout > 10) { - const didAttemptEntireTree = - !workInProgressRootDidSkipSuspendedSiblings; markRootSuspended( root, lanes, workInProgressDeferredLane, - didAttemptEntireTree, + workInProgressRootDidSkipSuspendedSiblings, ); const nextLanes = getNextLanes(root, NoLanes); @@ -1312,8 +1286,7 @@ function commitRootWhenReady( completedRenderEndTime, ), ); - const didAttemptEntireTree = !didSkipSuspendedSiblings; - markRootSuspended(root, lanes, spawnedLane, didAttemptEntireTree); + markRootSuspended(root, lanes, spawnedLane, didSkipSuspendedSiblings); return; } } @@ -1436,7 +1409,7 @@ function markRootSuspended( root: FiberRoot, suspendedLanes: Lanes, spawnedLane: Lane, - didAttemptEntireTree: boolean, + didSkipSuspendedSiblings: boolean, ) { // When suspending, we should always exclude lanes that were pinged or (more // rarely, since we try to avoid it) updated during the render phase. @@ -1445,7 +1418,12 @@ function markRootSuspended( suspendedLanes, workInProgressRootInterleavedUpdatedLanes, ); - _markRootSuspended(root, suspendedLanes, spawnedLane, didAttemptEntireTree); + _markRootSuspended( + root, + suspendedLanes, + spawnedLane, + didSkipSuspendedSiblings, + ); } export function flushRoot(root: FiberRoot, lanes: Lanes) { @@ -1987,12 +1965,7 @@ export function renderDidSuspendDelayIfPossible(): void { if ( !workInProgressRootDidSkipSuspendedSiblings && - // Check if the root will be blocked from committing. - // TODO: Consider aligning this better with the rest of the logic. Maybe - // we should only set the exit status to RootSuspendedWithDelay if this - // condition is true? And remove the equivalent checks elsewhere. - (includesOnlyTransitions(workInProgressRootRenderLanes) || - getSuspenseHandler() === null) + !includesBlockingLane(workInProgressRootRenderLanes) ) { // This render may not have originally been scheduled as a prerender, but // something suspended inside the visible part of the tree, which means we @@ -2018,12 +1991,11 @@ export function renderDidSuspendDelayIfPossible(): void { // pinged or updated while we were rendering. // TODO: Consider unwinding immediately, using the // SuspendedOnHydration mechanism. - const didAttemptEntireTree = false; markRootSuspended( workInProgressRoot, workInProgressRootRenderLanes, workInProgressDeferredLane, - didAttemptEntireTree, + workInProgressRootDidSkipSuspendedSiblings, ); } } @@ -2053,11 +2025,7 @@ export function renderHasNotSuspendedYet(): boolean { // TODO: Over time, this function and renderRootConcurrent have become more // and more similar. Not sure it makes sense to maintain forked paths. Consider // unifying them again. -function renderRootSync( - root: FiberRoot, - lanes: Lanes, - shouldYieldForPrerendering: boolean, -): RootExitStatus { +function renderRootSync(root: FiberRoot, lanes: Lanes) { const prevExecutionContext = executionContext; executionContext |= RenderContext; const prevDispatcher = pushDispatcher(root.containerInfo); @@ -2097,7 +2065,6 @@ function renderRootSync( } let didSuspendInShell = false; - let exitStatus = workInProgressRootExitStatus; outer: do { try { if ( @@ -2119,37 +2086,16 @@ function renderRootSync( // Selective hydration. An update flowed into a dehydrated tree. // Interrupt the current render so the work loop can switch to the // hydration lane. - // TODO: I think we might not need to reset the stack here; we can - // just yield and reset the stack when we re-enter the work loop, - // like normal. resetWorkInProgressStack(); - exitStatus = RootDidNotComplete; + workInProgressRootExitStatus = RootDidNotComplete; break outer; } case SuspendedOnImmediate: - case SuspendedOnData: - case SuspendedOnDeprecatedThrowPromise: { - if (getSuspenseHandler() === null) { + case SuspendedOnData: { + if (!didSuspendInShell && getSuspenseHandler() === null) { didSuspendInShell = true; } - const reason = workInProgressSuspendedReason; - workInProgressSuspendedReason = NotSuspended; - workInProgressThrownValue = null; - throwAndUnwindWorkLoop(root, unitOfWork, thrownValue, reason); - if ( - enableSiblingPrerendering && - shouldYieldForPrerendering && - workInProgressRootIsPrerendering - ) { - // We've switched into prerendering mode. This implies that we - // suspended outside of a Suspense boundary, which means this - // render will be blocked from committing. Yield to the main - // thread so we can switch to prerendering using the concurrent - // work loop. - exitStatus = RootInProgress; - break outer; - } - break; + // Intentional fallthrough } default: { // Unwind then continue with the normal work loop. @@ -2162,7 +2108,6 @@ function renderRootSync( } } workLoopSync(); - exitStatus = workInProgressRootExitStatus; break; } catch (thrownValue) { handleThrow(root, thrownValue); @@ -2185,6 +2130,14 @@ function renderRootSync( popDispatcher(prevDispatcher); popAsyncDispatcher(prevAsyncDispatcher); + if (workInProgress !== null) { + // This is a sync render, so we should have finished the whole tree. + throw new Error( + 'Cannot commit an incomplete root. This error is likely caused by a ' + + 'bug in React. Please file an issue.', + ); + } + if (__DEV__) { if (enableDebugTracing) { logRenderStopped(); @@ -2195,21 +2148,14 @@ function renderRootSync( markRenderStopped(); } - if (workInProgress !== null) { - // Did not complete the tree. This can happen if something suspended in - // the shell. - } else { - // Normal case. We completed the whole tree. - - // Set this to null to indicate there's no in-progress render. - workInProgressRoot = null; - workInProgressRootRenderLanes = NoLanes; + // Set this to null to indicate there's no in-progress render. + workInProgressRoot = null; + workInProgressRootRenderLanes = NoLanes; - // It's safe to process the queue now that the render phase is complete. - finishQueueingConcurrentUpdates(); - } + // It's safe to process the queue now that the render phase is complete. + finishQueueingConcurrentUpdates(); - return exitStatus; + return workInProgressRootExitStatus; } // The work loop is an extremely hot path. Tell Closure not to inline it. @@ -2255,7 +2201,9 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { // // If we were previously in prerendering mode, check if we received any new // data during an interleaved event. - workInProgressRootIsPrerendering = checkIfRootIsPrerendering(root, lanes); + if (workInProgressRootIsPrerendering) { + workInProgressRootIsPrerendering = checkIfRootIsPrerendering(root, lanes); + } } if (__DEV__) { @@ -3805,9 +3753,6 @@ function pingSuspendedRoot( // the logic of whether or not a root suspends once it completes. // TODO: If we're rendering sync either due to Sync, Batched or expired, // we should probably never restart. - // TODO: Attach different listeners depending on whether the listener was - // attached during prerendering. Prerender pings should not interrupt - // normal renders. // If we're suspended with delay, or if it's a retry, we'll always suspend // so we can always restart. diff --git a/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js b/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js index 8ca142cb50517..fd03ba7310f83 100644 --- a/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js +++ b/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js @@ -420,10 +420,6 @@ describe('ReactDeferredValue', () => { // The initial value suspended, so we attempt the final value, which // also suspends. 'Suspend! [Final]', - - ...(gate('enableSiblingPrerendering') - ? ['Suspend! [Loading...]', 'Suspend! [Final]'] - : []), ]); expect(root).toMatchRenderedOutput(null); @@ -463,10 +459,6 @@ describe('ReactDeferredValue', () => { // The initial value suspended, so we attempt the final value, which // also suspends. 'Suspend! [Final]', - - ...(gate('enableSiblingPrerendering') - ? ['Suspend! [Loading...]', 'Suspend! [Final]'] - : []), ]); expect(root).toMatchRenderedOutput(null); @@ -541,10 +533,6 @@ describe('ReactDeferredValue', () => { // The initial value suspended, so we attempt the final value, which // also suspends. 'Suspend! [Final]', - - ...(gate('enableSiblingPrerendering') - ? ['Suspend! [Loading...]', 'Suspend! [Final]'] - : []), ]); expect(root).toMatchRenderedOutput(null); diff --git a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js index 235e8df2201de..37d907b0fdc65 100644 --- a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js @@ -479,75 +479,4 @@ describe('ReactSiblingPrerendering', () => { assertLog([]); }, ); - - it( - 'when a synchronous update suspends outside a boundary, the resulting' + - 'prerender is concurrent', - async () => { - function App() { - return ( - <> - - - - - - - ); - } - - const root = ReactNoop.createRoot(); - // Mount the root synchronously - ReactNoop.flushSync(() => root.render()); - - // Synchronously render everything until we suspend in the shell - assertLog(['A', 'B', 'Suspend! [Async]']); - - if (gate('enableSiblingPrerendering')) { - // The rest of the siblings begin to prerender concurrently. Notice - // that we don't unwind here; we pick up where we left off above. - await waitFor(['C']); - await waitFor(['D']); - } - - assertLog([]); - expect(root).toMatchRenderedOutput(null); - - await resolveText('Async'); - assertLog(['A', 'B', 'Async', 'C', 'D']); - expect(root).toMatchRenderedOutput('ABAsyncCD'); - }, - ); - - it('restart a suspended sync render if something suspends while prerendering the siblings', async () => { - function App() { - return ( - <> - - - - - - - ); - } - - const root = ReactNoop.createRoot(); - // Mount the root synchronously - ReactNoop.flushSync(() => root.render()); - - // Synchronously render everything until we suspend in the shell - assertLog(['A', 'B', 'Suspend! [Async]']); - - if (gate('enableSiblingPrerendering')) { - // The rest of the siblings begin to prerender concurrently - await waitFor(['C']); - } - - // While we're prerendering, Async resolves. We should unwind and - // start over, rather than continue prerendering D. - await resolveText('Async'); - assertLog(['A', 'B', 'Async', 'C', 'D']); - expect(root).toMatchRenderedOutput('ABAsyncCD'); - }); });