From cae764ce81b1bd6c418e9e23651794b6b09208e8 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Fri, 25 Oct 2024 09:17:07 -0700 Subject: [PATCH] Revert "[Re-land] Make prerendering always non-blocking (#31268)" (#31355) This reverts commit 6c4bbc783286bf6eebd9927cb52e8fec5ad4dd74. It looked like the bug we found on the original land was related to broken product code. But through landing #31268 we found additional bugs internally. Since disabling the feature flag does not fix the bugs, we have to revert again to unblock the sync. We can continue to debug with our internal build. --- .../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'); - }); });