From 6718779054c2832f5910b6a1c004d1a482861d05 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 24 Sep 2024 10:03:15 -0700 Subject: [PATCH 1/6] Make prerendering always non-blocking When a synchronous update suspends, and we prerender the siblings, the prerendering should be non-blocking so that we can immediately restart once the data arrives. This happens automatically when there's a Suspense boundary, because we immediately commit the boundary and then proceed to a Retry render, which are always concurrent. When there's not a Suspense boundary, there is no Retry, so we need to take care to switch from the synchronous work loop to the concurrent one, to enable time slicing. --- .../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, 278 insertions(+), 126 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index ee843996bef1c..027099d54707c 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([]); + assertLog(gate('enableSiblingPrerendering') ? ['Suspend! [/path/b]'] : []); await act(async () => { resolvePromise(); diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js index 7d6bfd16e8aa0..b98019046e3c2 100644 --- a/packages/react-reconciler/src/ReactFiberLane.js +++ b/packages/react-reconciler/src/ReactFiberLane.js @@ -765,12 +765,14 @@ export function markRootSuspended( root: FiberRoot, suspendedLanes: Lanes, spawnedLane: Lane, - didSkipSuspendedSiblings: boolean, + didAttemptEntireTree: 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 && !didSkipSuspendedSiblings) { + if (enableSiblingPrerendering && didAttemptEntireTree) { // 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 9f267e8345e39..39f6f466ed983 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -18,6 +18,7 @@ import { disableSchedulerTimeoutInWorkLoop, enableProfilerTimer, enableProfilerNestedUpdatePhase, + enableSiblingPrerendering, } from 'shared/ReactFeatureFlags'; import { NoLane, @@ -29,6 +30,7 @@ import { markStarvedLanesAsExpired, claimNextTransitionLane, getNextLanesToFlushSync, + checkIfRootIsPrerendering, } from './ReactFiberLane'; import { CommitContext, @@ -206,7 +208,10 @@ function flushSyncWorkAcrossRoots_impl( ? workInProgressRootRenderLanes : NoLanes, ); - if (includesSyncLane(nextLanes)) { + if ( + includesSyncLane(nextLanes) && + !checkIfRootIsPrerendering(root, nextLanes) + ) { // This root has pending sync work. Flush it now. didPerformSomeWork = true; performSyncWorkOnRoot(root, nextLanes); @@ -341,7 +346,13 @@ function scheduleTaskForRootDuringMicrotask( } // Schedule a new callback in the host environment. - if (includesSyncLane(nextLanes)) { + 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)) + ) { // Synchronous work is always flushed at the end of the microtask, so we // don't need to schedule an additional task. if (existingCallbackNode !== null) { @@ -375,9 +386,10 @@ 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 1ea05dd010a81..b02d06884f227 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -764,11 +764,12 @@ 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, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); } @@ -831,11 +832,12 @@ 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, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); } } @@ -897,100 +899,120 @@ 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); + (!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)); + let exitStatus = shouldTimeSlice ? renderRootConcurrent(root, lanes) - : renderRootSync(root, lanes); + : renderRootSync(root, lanes, true); - if (exitStatus !== RootInProgress) { + do { let renderWasConcurrent = shouldTimeSlice; - 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( + 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( root, - lanes, - NoLane, - workInProgressRootDidSkipSuspendedSiblings, + lanesThatJustErrored, ); - } 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( + if (errorRetryLanes !== NoLanes) { + lanes = errorRetryLanes; + exitStatus = recoverFromConcurrentError( root, lanesThatJustErrored, + errorRetryLanes, ); - 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. - } + 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); - 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); - } + 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; + } + + // 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); } @@ -1023,7 +1045,7 @@ function recoverFromConcurrentError( rootWorkInProgress.flags |= ForceClientRender; } - const exitStatus = renderRootSync(root, errorRetryLanes); + const exitStatus = renderRootSync(root, errorRetryLanes, false); if (exitStatus !== RootErrored) { // Successfully finished rendering on retry @@ -1107,11 +1129,13 @@ 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, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); return; } @@ -1167,11 +1191,13 @@ function finishConcurrentRender( // Don't bother with a very short suspense time. if (msUntilTimeout > 10) { + const didAttemptEntireTree = + !workInProgressRootDidSkipSuspendedSiblings; markRootSuspended( root, lanes, workInProgressDeferredLane, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); const nextLanes = getNextLanes(root, NoLanes); @@ -1285,7 +1311,8 @@ function commitRootWhenReady( completedRenderEndTime, ), ); - markRootSuspended(root, lanes, spawnedLane, didSkipSuspendedSiblings); + const didAttemptEntireTree = !didSkipSuspendedSiblings; + markRootSuspended(root, lanes, spawnedLane, didAttemptEntireTree); return; } } @@ -1408,7 +1435,7 @@ function markRootSuspended( root: FiberRoot, suspendedLanes: Lanes, spawnedLane: Lane, - didSkipSuspendedSiblings: boolean, + didAttemptEntireTree: 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. @@ -1417,12 +1444,7 @@ function markRootSuspended( suspendedLanes, workInProgressRootInterleavedUpdatedLanes, ); - _markRootSuspended( - root, - suspendedLanes, - spawnedLane, - didSkipSuspendedSiblings, - ); + _markRootSuspended(root, suspendedLanes, spawnedLane, didAttemptEntireTree); } export function flushRoot(root: FiberRoot, lanes: Lanes) { @@ -1964,7 +1986,12 @@ export function renderDidSuspendDelayIfPossible(): void { if ( !workInProgressRootDidSkipSuspendedSiblings && - !includesBlockingLane(workInProgressRootRenderLanes) + // 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) ) { // This render may not have originally been scheduled as a prerender, but // something suspended inside the visible part of the tree, which means we @@ -1990,11 +2017,12 @@ 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, - workInProgressRootDidSkipSuspendedSiblings, + didAttemptEntireTree, ); } } @@ -2024,7 +2052,11 @@ 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) { +function renderRootSync( + root: FiberRoot, + lanes: Lanes, + shouldYieldForPrerendering: boolean, +): RootExitStatus { const prevExecutionContext = executionContext; executionContext |= RenderContext; const prevDispatcher = pushDispatcher(root.containerInfo); @@ -2064,6 +2096,7 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { } let didSuspendInShell = false; + let exitStatus = workInProgressRootExitStatus; outer: do { try { if ( @@ -2085,16 +2118,37 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { // 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(); - workInProgressRootExitStatus = RootDidNotComplete; + exitStatus = RootDidNotComplete; break outer; } case SuspendedOnImmediate: - case SuspendedOnData: { - if (!didSuspendInShell && getSuspenseHandler() === null) { + case SuspendedOnData: + case SuspendedOnDeprecatedThrowPromise: { + if (getSuspenseHandler() === null) { didSuspendInShell = true; } - // Intentional fallthrough + 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; } default: { // Unwind then continue with the normal work loop. @@ -2107,6 +2161,7 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { } } workLoopSync(); + exitStatus = workInProgressRootExitStatus; break; } catch (thrownValue) { handleThrow(root, thrownValue); @@ -2129,14 +2184,6 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { 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(); @@ -2147,14 +2194,21 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { markRenderStopped(); } - // Set this to null to indicate there's no in-progress render. - workInProgressRoot = null; - workInProgressRootRenderLanes = NoLanes; + 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; - // 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 workInProgressRootExitStatus; + return exitStatus; } // The work loop is an extremely hot path. Tell Closure not to inline it. @@ -2200,9 +2254,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { // // If we were previously in prerendering mode, check if we received any new // data during an interleaved event. - if (workInProgressRootIsPrerendering) { - workInProgressRootIsPrerendering = checkIfRootIsPrerendering(root, lanes); - } + workInProgressRootIsPrerendering = checkIfRootIsPrerendering(root, lanes); } if (__DEV__) { @@ -3744,6 +3796,9 @@ 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 fd03ba7310f83..8ca142cb50517 100644 --- a/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js +++ b/packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js @@ -420,6 +420,10 @@ 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); @@ -459,6 +463,10 @@ 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); @@ -533,6 +541,10 @@ 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 37d907b0fdc65..235e8df2201de 100644 --- a/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSiblingPrerendering-test.js @@ -479,4 +479,75 @@ 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'); + }); }); From e0522aa2185fd00d5c9060bed0c4fb138377ee21 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Thu, 7 Nov 2024 17:09:13 -0500 Subject: [PATCH 2/6] Fix for external store infinite loops We are getting infinite sync render loops from useSyncExternalStore render phase updates (Recoil). renderWasConcurrent within the do block made the sync pass pin the exitStatus to RootSuspended. --- packages/react-reconciler/src/ReactFiberWorkLoop.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index b02d06884f227..bd30492fc0d0e 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -913,8 +913,9 @@ export function performWorkOnRoot( ? renderRootConcurrent(root, lanes) : renderRootSync(root, lanes, true); + let renderWasConcurrent = shouldTimeSlice; + do { - let renderWasConcurrent = shouldTimeSlice; if (exitStatus === RootInProgress) { // Render phase is still in progress. if ( From 1d0738eae297d487682a18a32e7630bdb5995008 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Thu, 7 Nov 2024 18:49:10 -0500 Subject: [PATCH 3/6] Add test --- .../__tests__/useSyncExternalStore-test.js | 147 +++++++++++++++++- 1 file changed, 146 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js index a61b5e49bb890..8cd44b282e72a 100644 --- a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js +++ b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js @@ -24,6 +24,10 @@ let startTransition; let waitFor; let waitForAll; let assertLog; +let Suspense; +let useCallback; +let useMemo; +let textCache; // This tests the native useSyncExternalStore implementation, not the shim. // Tests that apply to both the native implementation and the shim should go @@ -45,7 +49,10 @@ describe('useSyncExternalStore', () => { use = React.use; useSyncExternalStore = React.useSyncExternalStore; startTransition = React.startTransition; - + Suspense = React.Suspense; + useCallback = React.useCallback; + useMemo = React.useMemo; + textCache = new Map(); const InternalTestUtils = require('internal-test-utils'); waitFor = InternalTestUtils.waitFor; waitForAll = InternalTestUtils.waitForAll; @@ -54,6 +61,60 @@ describe('useSyncExternalStore', () => { act = require('internal-test-utils').act; }); + function resolveText(text) { + const record = textCache.get(text); + if (record === undefined) { + const newRecord = { + status: 'resolved', + value: text, + }; + textCache.set(text, newRecord); + } else if (record.status === 'pending') { + const thenable = record.value; + record.status = 'resolved'; + record.value = text; + thenable.pings.forEach(t => t()); + } + } + function readText(text) { + const record = textCache.get(text); + if (record !== undefined) { + switch (record.status) { + case 'pending': + throw record.value; + case 'rejected': + throw record.value; + case 'resolved': + return record.value; + } + } else { + const thenable = { + pings: [], + then(resolve) { + if (newRecord.status === 'pending') { + thenable.pings.push(resolve); + } else { + Promise.resolve().then(() => resolve(newRecord.value)); + } + }, + }; + + const newRecord = { + status: 'pending', + value: thenable, + }; + textCache.set(text, newRecord); + + throw thenable; + } + } + + function AsyncText({text}) { + const result = readText(text); + Scheduler.log(text); + return result; + } + function Text({text}) { Scheduler.log(text); return text; @@ -292,4 +353,88 @@ describe('useSyncExternalStore', () => { ); }, ); + + it('regression: doesnt loop for only changing store reference', async () => { + let store = {validationResults: new Map(), version: 0}; + let listeners = []; + + const ExternalStore = { + set(value) { + // Change the store ref, but not the value. + store = {...store}; + emitChange(); + }, + subscribe(listener) { + listeners = [...listeners, listener]; + return () => { + listeners = listeners.filter(l => l !== listener); + }; + }, + getSnapshot() { + return store; + }, + }; + + function emitChange() { + for (const listener of listeners) { + listener(); + } + } + + function useBug(path: string): SRConfigPathState { + const {value} = useSyncExternalStore( + ExternalStore.subscribe, + ExternalStore.getSnapshot, + ); + + const setStoreValue = useCallback(value => { + ExternalStore.set(value); + }, []); + + const update = useCallback( + newValue => { + if (value == null && newValue != null) { + setStoreValue(newValue); + } + }, + [setStoreValue, value], + ); + + useMemo(() => { + update({foo: 'bar'}); + }, []); + + return {}; + } + + function Bug() { + useBug(); + return ; + } + + function App() { + return ( + <> + + + + + + ); + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + assertLog([...(gate('enableSiblingPrerendering') ? ['B'] : [])]); + + expect(root).toMatchRenderedOutput('Loading...'); + + // Resolve the data and finish rendering. + await act(() => resolveText('A')); + assertLog(['A', 'B', 'A', 'B', 'B']); + + expect(root).toMatchRenderedOutput('AB'); + }); }); From ea8050810a4d3ad5e7968b42d71dc1d9cfffbd68 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Fri, 8 Nov 2024 11:48:10 -0500 Subject: [PATCH 4/6] Clean up test --- .../__tests__/useSyncExternalStore-test.js | 57 ++++++++++--------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js index 8cd44b282e72a..4b7afd48022e4 100644 --- a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js +++ b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js @@ -354,14 +354,21 @@ describe('useSyncExternalStore', () => { }, ); - it('regression: doesnt loop for only changing store reference', async () => { - let store = {validationResults: new Map(), version: 0}; + it('regression: does not infinite loop for only changing store reference in render', async () => { + let store = {value: {}}; let listeners = []; const ExternalStore = { set(value) { // Change the store ref, but not the value. + // This will cause a new snapshot to be returned if set is called in render, + // but the value is the same. Stores should not do this, but if they do + // we shouldn't infinitely render. store = {...store}; + setTimeout(() => { + store = {value}; + emitChange(); + }, 100); emitChange(); }, subscribe(listener) { @@ -381,35 +388,22 @@ describe('useSyncExternalStore', () => { } } - function useBug(path: string): SRConfigPathState { + function StoreText() { const {value} = useSyncExternalStore( ExternalStore.subscribe, ExternalStore.getSnapshot, ); - const setStoreValue = useCallback(value => { - ExternalStore.set(value); - }, []); - - const update = useCallback( - newValue => { - if (value == null && newValue != null) { - setStoreValue(newValue); - } - }, - [setStoreValue, value], - ); - useMemo(() => { - update({foo: 'bar'}); + // Set the store value on mount. + // This breaks the rules of React, but should be handled gracefully. + const newValue = {text: 'B'}; + if (value == null || newValue !== value) { + ExternalStore.set(newValue); + } }, []); - return {}; - } - - function Bug() { - useBug(); - return ; + return ; } function App() { @@ -417,23 +411,32 @@ describe('useSyncExternalStore', () => { <> - + ); } const root = ReactNoop.createRoot(); + + // The initial render suspends. await act(async () => { root.render(); }); - assertLog([...(gate('enableSiblingPrerendering') ? ['B'] : [])]); + assertLog([...(gate('enableSiblingPrerendering') ? ['(not set)'] : [])]); expect(root).toMatchRenderedOutput('Loading...'); // Resolve the data and finish rendering. - await act(() => resolveText('A')); - assertLog(['A', 'B', 'A', 'B', 'B']); + // When resolving, the store should not get stuck in an infinite loop. + await act(() => { + resolveText('A'); + }); + assertLog([ + ...(gate('enableSiblingPrerendering') + ? ['A', 'B', 'A', 'B', 'B'] + : ['A', '(not set)', 'A', '(not set)', 'B']), + ]); expect(root).toMatchRenderedOutput('AB'); }); From 6677c6336b1cce358eff8d7649a7b97928848648 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Fri, 8 Nov 2024 11:54:01 -0500 Subject: [PATCH 5/6] lint --- .../src/__tests__/useSyncExternalStore-test.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js index 4b7afd48022e4..24ca84c7d9762 100644 --- a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js +++ b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js @@ -25,7 +25,6 @@ let waitFor; let waitForAll; let assertLog; let Suspense; -let useCallback; let useMemo; let textCache; @@ -50,7 +49,6 @@ describe('useSyncExternalStore', () => { useSyncExternalStore = React.useSyncExternalStore; startTransition = React.startTransition; Suspense = React.Suspense; - useCallback = React.useCallback; useMemo = React.useMemo; textCache = new Map(); const InternalTestUtils = require('internal-test-utils'); @@ -383,9 +381,7 @@ describe('useSyncExternalStore', () => { }; function emitChange() { - for (const listener of listeners) { - listener(); - } + listeners.forEach(l => l()); } function StoreText() { From 3f356bf5d80da9ab3ff642a06e03879f73e359a6 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Fri, 8 Nov 2024 12:06:12 -0500 Subject: [PATCH 6/6] Fix test for xplat, alwaysThrottleRetries --- .../src/__tests__/useSyncExternalStore-test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js index 24ca84c7d9762..a5c6a6995d718 100644 --- a/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js +++ b/packages/react-reconciler/src/__tests__/useSyncExternalStore-test.js @@ -431,7 +431,9 @@ describe('useSyncExternalStore', () => { assertLog([ ...(gate('enableSiblingPrerendering') ? ['A', 'B', 'A', 'B', 'B'] - : ['A', '(not set)', 'A', '(not set)', 'B']), + : gate(flags => flags.alwaysThrottleRetries) + ? ['A', '(not set)', 'A', '(not set)', 'B'] + : ['A', '(not set)', 'A', '(not set)', '(not set)', 'B']), ]); expect(root).toMatchRenderedOutput('AB');