Skip to content

Commit

Permalink
Make prerendering always non-blocking with fix (#31452)
Browse files Browse the repository at this point in the history
We've previously failed to land this change due to some internal apps
seeing infinite render loops due to external store state updates during
render. It turns out that since the `renderWasConcurrent` var was moved
into the do block, the sync render triggered from the external store
check was stuck with a `RootSuspended` `exitStatus`. So this is not
unique to sibling prerendering but more generally related to how we
handle update to a sync external store during render.

We've tested this build against local repros which now render without
crashes. We will try to add a unit test to cover the scenario as well.

---------

Co-authored-by: Andrew Clark <[email protected]>
Co-authored-by: Rick Hanlon <[email protected]>
  • Loading branch information
3 people authored Nov 8, 2024
1 parent 5c56b87 commit 989af12
Show file tree
Hide file tree
Showing 7 changed files with 428 additions and 129 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
6 changes: 4 additions & 2 deletions packages/react-reconciler/src/ReactFiberLane.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
20 changes: 16 additions & 4 deletions packages/react-reconciler/src/ReactFiberRootScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
disableSchedulerTimeoutInWorkLoop,
enableProfilerTimer,
enableProfilerNestedUpdatePhase,
enableSiblingPrerendering,
} from 'shared/ReactFeatureFlags';
import {
NoLane,
Expand All @@ -29,6 +30,7 @@ import {
markStarvedLanesAsExpired,
claimNextTransitionLane,
getNextLanesToFlushSync,
checkIfRootIsPrerendering,
} from './ReactFiberLane';
import {
CommitContext,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 989af12

Please sign in to comment.