Skip to content

Commit

Permalink
Detect loops caused by updates during prerender
Browse files Browse the repository at this point in the history
When Meta tried rolling out enableSiblingPrerendering internally, there
were reports of infinite render loops that we suspect were caused by
updates triggered during the render phase. While we don't have an
exact repro, we know that theoretically this is possible because any
update, included one triggered as a side effect of rendering, will
interrupt an in-progress prerender.

Although we already have warnings and protections against updates that
occur during the render phase, the sibling prerendering experiment
introduces new scenarios that could cause previously working (though
technically incorrect) product code to regress.

The solution in this PR is to maintain a counter of how many times a
prerender is interrupted before it successfully completes. Once the
counter reaches that threshold, we disable the prerendering mechanism,
effectively reverting to the behavior that's in canary today.

The counter is reset the next time the update queue is exhausted,
allowing for subsequent prerenders to work as before.
  • Loading branch information
acdlite committed Nov 7, 2024
1 parent e137890 commit d520fcf
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 2 deletions.
20 changes: 18 additions & 2 deletions packages/react-reconciler/src/ReactFiberLane.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,12 @@ export const HydrationLanes =
SelectiveHydrationLane |
IdleHydrationLane;

// If a prerender is interrupted, either by an update or a resolved promise,
// we increment a counter. Once the counter reaches a limit, we disable
// subsequent prerender attempts. This is to prevent an accidental infinite
// render loop caused by a prerender that spawns render phase updates.
const PRERENDER_INTERRUPT_LIMIT = 5;

// This function is used for the experimental timeline (react-devtools-timeline)
// It should be kept in sync with the Lanes values above.
export function getLabelForLane(lane: Lane): string | void {
Expand Down Expand Up @@ -225,6 +231,10 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
// Early bailout if there's no pending work left.
const pendingLanes = root.pendingLanes;
if (pendingLanes === NoLanes) {
// Once we've exhausted the work queue, we can be certain that we're no
// longer in a render loop caused by prerendering. Setting this back to
// zero allows subsequent updates to prerender its siblings when necessary.
root.prerenderInterruptCounter = 0;
return NoLanes;
}

Expand Down Expand Up @@ -273,7 +283,10 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
} else {
if (enableSiblingPrerendering) {
// Nothing has been pinged. Check for lanes that need to be prewarmed.
if (!rootHasPendingCommit) {
if (
root.prerenderInterruptCounter < PRERENDER_INTERRUPT_LIMIT &&
!rootHasPendingCommit
) {
const lanesToPrewarm = nonIdlePendingLanes & ~warmLanes;
if (lanesToPrewarm !== NoLanes) {
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
Expand All @@ -299,7 +312,10 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
} else {
if (enableSiblingPrerendering) {
// Nothing has been pinged. Check for lanes that need to be prewarmed.
if (!rootHasPendingCommit) {
if (
root.prerenderInterruptCounter < PRERENDER_INTERRUPT_LIMIT &&
!rootHasPendingCommit
) {
const lanesToPrewarm = pendingLanes & ~warmLanes;
if (lanesToPrewarm !== NoLanes) {
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberRoot.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ function FiberRootNode(
this.finishedLanes = NoLanes;
this.errorRecoveryDisabledLanes = NoLanes;
this.shellSuspendCounter = 0;
this.prerenderInterruptCounter = 0;

this.entangledLanes = NoLanes;
this.entanglements = createLaneMap(NoLanes);
Expand Down
10 changes: 10 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,15 @@ export function scheduleUpdateOnFiber(
// Mark that the root has a pending update.
markRootUpdated(root, lane);

if (workInProgressRootIsPrerendering && workInProgressRoot !== null) {
// This update happened while we were in the middle of a prerender.
// Increment a counter. If this happens too often before we finish the
// update, we will disable prerenders until the queue is empty. This is to
// prevent a potential scenario where an update is spawned by a prerender,
// which then interrupts itself, leading to an infinite loop.
workInProgressRoot.prerenderInterruptCounter++;
}

if (
(executionContext & RenderContext) !== NoLanes &&
root === workInProgressRoot
Expand Down Expand Up @@ -2271,6 +2280,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) {
break outer;
}
case SuspendedOnImmediate: {
console.log('! suspend on immediate');

Check failure on line 2283 in packages/react-reconciler/src/ReactFiberWorkLoop.js

View workflow job for this annotation

GitHub Actions / Run eslint

Unexpected use of console
// If this fiber just suspended, it's possible the data is already
// cached. Yield to the main thread to give it a chance to ping. If
// it does, we can retry immediately without unwinding the stack.
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactInternalTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ type BaseFiberRootProperties = {
expiredLanes: Lanes,
errorRecoveryDisabledLanes: Lanes,
shellSuspendCounter: number,
prerenderInterruptCounter: number,

finishedLanes: Lanes,

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ let ReactNoop;
let Scheduler;
let act;
let assertLog;
let assertConsoleErrorDev;
let waitFor;
let waitForPaint;
let waitForAll;
let textCache;
let startTransition;
let useState;
let Suspense;
let Activity;

Expand All @@ -20,9 +22,12 @@ describe('ReactSiblingPrerendering', () => {
Scheduler = require('scheduler');
act = require('internal-test-utils').act;
assertLog = require('internal-test-utils').assertLog;
assertConsoleErrorDev =
require('internal-test-utils').assertConsoleErrorDev;
waitFor = require('internal-test-utils').waitFor;
waitForPaint = require('internal-test-utils').waitForPaint;
waitForAll = require('internal-test-utils').waitForAll;
useState = React.useState;
startTransition = React.startTransition;
Suspense = React.Suspense;
Activity = React.unstable_Activity;
Expand Down Expand Up @@ -479,4 +484,53 @@ describe('ReactSiblingPrerendering', () => {
assertLog([]);
},
);

it('prevents infinite prerender loop caused by render phase update', async () => {
let resolve;
let resolvedValue = null;
const promise = new Promise(r => {
resolve = value => {
resolvedValue = value;
r();
};
});

function Async() {
if (resolvedValue === null) {
throw promise;
}
return resolvedValue;
}

function UnsafeUpdateDuringRender({setState}) {
if (resolvedValue === null) {
setState(n => n + 1);
}
return null;
}

function App() {
const [, setState] = useState(0);
return (
<Suspense fallback="Loading...">
<Async />
<UnsafeUpdateDuringRender setState={setState} />
</Suspense>
);
}

// This just tests that this update eventually terminates without falling
// into an infinte prerender loop, which it used to before we added a
// counter that disables prerendering after a certain number of attempts.
const root = ReactNoop.createRoot();
await act(() => root.render(<App />));
assertConsoleErrorDev([
'Cannot update a component (`App`) while rendering',
]);
assertLog([]);
expect(root).toMatchRenderedOutput('Loading...');

await act(() => resolve('Done'));
expect(root).toMatchRenderedOutput('Done');
});
});

0 comments on commit d520fcf

Please sign in to comment.