Skip to content

Commit

Permalink
Add infinite update loop detection
Browse files Browse the repository at this point in the history
This is a partial redo of #26625. Since that was unlanded due to some detected breakages. This now includes a feature flag to be careful in rolling this out.
  • Loading branch information
kassens committed Feb 9, 2024
1 parent 03d6f7c commit 61ac209
Show file tree
Hide file tree
Showing 10 changed files with 177 additions and 11 deletions.
64 changes: 64 additions & 0 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1709,6 +1709,70 @@ describe('ReactUpdates', () => {
expect(subscribers.length).toBe(limit);
});

it("does not infinite loop if there's a synchronous render phase update on another component", () => {
if (gate(flags => !flags.enableInfiniteRenderLoopDetection)) {
return;
}
let setState;
function App() {
const [, _setState] = React.useState(0);
setState = _setState;
return <Child />;
}

function Child(step) {
// This will cause an infinite update loop, and a warning in dev.
setState(n => n + 1);
return null;
}

const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);

expect(() => {
expect(() => ReactDOM.flushSync(() => root.render(<App />))).toThrow(
'Maximum update depth exceeded',
);
}).toErrorDev(
'Warning: Cannot update a component (`App`) while rendering a different component (`Child`)',
);
});

it("does not infinite loop if there's an async render phase update on another component", async () => {
if (gate(flags => !flags.enableInfiniteRenderLoopDetection)) {
return;
}
let setState;
function App() {
const [, _setState] = React.useState(0);
setState = _setState;
return <Child />;
}

function Child(step) {
// This will cause an infinite update loop, and a warning in dev.
setState(n => n + 1);
return null;
}

const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);

await expect(async () => {
let error;
try {
await act(() => {
React.startTransition(() => root.render(<App />));
});
} catch (e) {
error = e;
}
expect(error.message).toMatch('Maximum update depth exceeded');
}).toErrorDev(
'Warning: Cannot update a component (`App`) while rendering a different component (`Child`)',
);
});

// TODO: Replace this branch with @gate pragmas
if (__DEV__) {
it('warns about a deferred infinite update loop with useEffect', async () => {
Expand Down
110 changes: 99 additions & 11 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
useModernStrictMode,
disableLegacyContext,
alwaysThrottleRetries,
enableInfiniteRenderLoopDetection,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import is from 'shared/objectIs';
Expand Down Expand Up @@ -147,10 +148,10 @@ import {
getNextLanes,
getEntangledLanes,
getLanesToRetrySynchronouslyOnError,
markRootUpdated,
markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
markRootPinged,
upgradePendingLanesToSync,
markRootSuspended as _markRootSuspended,
markRootUpdated as _markRootUpdated,
markRootPinged as _markRootPinged,
markRootFinished,
addFiberToLanesMap,
movePendingFibersToMemoized,
Expand Down Expand Up @@ -381,6 +382,13 @@ let workInProgressRootConcurrentErrors: Array<CapturedValue<mixed>> | null =
let workInProgressRootRecoverableErrors: Array<CapturedValue<mixed>> | null =
null;

// Tracks when an update occurs during the render phase.
let workInProgressRootDidIncludeRecursiveRenderUpdate: boolean = false;
// Thacks when an update occurs during the commit phase. It's a separate
// variable from the one for renders because the commit phase may run
// concurrently to a render phase.
let didIncludeCommitPhaseUpdate: boolean = false;

// The most recent time we either committed a fallback, or when a fallback was
// filled in with the resolved UI. This lets us throttle the appearance of new
// content as it streams in, to minimize jank.
Expand Down Expand Up @@ -1154,6 +1162,7 @@ function finishConcurrentRender(
root,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
workInProgressDeferredLane,
);
} else {
Expand Down Expand Up @@ -1189,6 +1198,7 @@ function finishConcurrentRender(
finishedWork,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
lanes,
workInProgressDeferredLane,
),
Expand All @@ -1202,6 +1212,7 @@ function finishConcurrentRender(
finishedWork,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
lanes,
workInProgressDeferredLane,
);
Expand All @@ -1213,6 +1224,7 @@ function commitRootWhenReady(
finishedWork: Fiber,
recoverableErrors: Array<CapturedValue<mixed>> | null,
transitions: Array<Transition> | null,
didIncludeRenderPhaseUpdate: boolean,
lanes: Lanes,
spawnedLane: Lane,
) {
Expand Down Expand Up @@ -1240,15 +1252,27 @@ function commitRootWhenReady(
// us that it's ready. This will be canceled if we start work on the
// root again.
root.cancelPendingCommit = schedulePendingCommit(
commitRoot.bind(null, root, recoverableErrors, transitions),
commitRoot.bind(
null,
root,
recoverableErrors,
transitions,
didIncludeRenderPhaseUpdate,
),
);
markRootSuspended(root, lanes, spawnedLane);
return;
}
}

// Otherwise, commit immediately.
commitRoot(root, recoverableErrors, transitions, spawnedLane);
commitRoot(
root,
recoverableErrors,
transitions,
didIncludeRenderPhaseUpdate,
spawnedLane,
);
}

function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {
Expand Down Expand Up @@ -1304,21 +1328,59 @@ function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {
return true;
}

// The extra indirections around markRootUpdated and markRootSuspended is
// needed to avoid a circular dependency between this module and
// ReactFiberLane. There's probably a better way to split up these modules and
// avoid this problem. Perhaps all the root-marking functions should move into
// the work loop.

function markRootUpdated(root: FiberRoot, updatedLanes: Lanes) {
_markRootUpdated(root, updatedLanes);

if (enableInfiniteRenderLoopDetection) {
// Check for recursive updates
if (executionContext & RenderContext) {
workInProgressRootDidIncludeRecursiveRenderUpdate = true;
} else if (executionContext & CommitContext) {
didIncludeCommitPhaseUpdate = true;
}

throwIfInfiniteUpdateLoopDetected();
}
}

function markRootPinged(root: FiberRoot, pingedLanes: Lanes) {
_markRootPinged(root, pingedLanes);

if (enableInfiniteRenderLoopDetection) {
// Check for recursive pings. Pings are conceptually different from updates in
// other contexts but we call it an "update" in this context because
// repeatedly pinging a suspended render can cause a recursive render loop.
// The relevant property is that it can result in a new render attempt
// being scheduled.
if (executionContext & RenderContext) {
workInProgressRootDidIncludeRecursiveRenderUpdate = true;
} else if (executionContext & CommitContext) {
didIncludeCommitPhaseUpdate = true;
}

throwIfInfiniteUpdateLoopDetected();
}
}

function markRootSuspended(
root: FiberRoot,
suspendedLanes: Lanes,
spawnedLane: Lane,
) {
// When suspending, we should always exclude lanes that were pinged or (more
// rarely, since we try to avoid it) updated during the render phase.
// TODO: Lol maybe there's a better way to factor this besides this
// obnoxiously named function :)
suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes);
suspendedLanes = removeLanes(
suspendedLanes,
workInProgressRootInterleavedUpdatedLanes,
);
markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes, spawnedLane);
_markRootSuspended(root, suspendedLanes, spawnedLane);
}

// This is the entry point for synchronous tasks that don't go
Expand Down Expand Up @@ -1391,6 +1453,7 @@ export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null {
root,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
workInProgressDeferredLane,
);

Expand Down Expand Up @@ -1607,6 +1670,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {
workInProgressDeferredLane = NoLane;
workInProgressRootConcurrentErrors = null;
workInProgressRootRecoverableErrors = null;
workInProgressRootDidIncludeRecursiveRenderUpdate = false;

// Get the lanes that are entangled with whatever we're about to render. We
// track these separately so we can distinguish the priority of the render
Expand Down Expand Up @@ -2675,6 +2739,7 @@ function commitRoot(
root: FiberRoot,
recoverableErrors: null | Array<CapturedValue<mixed>>,
transitions: Array<Transition> | null,
didIncludeRenderPhaseUpdate: boolean,
spawnedLane: Lane,
) {
// TODO: This no longer makes any sense. We already wrap the mutation and
Expand All @@ -2689,6 +2754,7 @@ function commitRoot(
root,
recoverableErrors,
transitions,
didIncludeRenderPhaseUpdate,
previousUpdateLanePriority,
spawnedLane,
);
Expand All @@ -2704,6 +2770,7 @@ function commitRootImpl(
root: FiberRoot,
recoverableErrors: null | Array<CapturedValue<mixed>>,
transitions: Array<Transition> | null,
didIncludeRenderPhaseUpdate: boolean,
renderPriorityLevel: EventPriority,
spawnedLane: Lane,
) {
Expand Down Expand Up @@ -2784,6 +2851,9 @@ function commitRootImpl(

markRootFinished(root, remainingLanes, spawnedLane);

// Reset this before firing side effects so we can detect recursive updates.
didIncludeCommitPhaseUpdate = false;

if (root === workInProgressRoot) {
// We can reset these now that they are finished.
workInProgressRoot = null;
Expand Down Expand Up @@ -3036,10 +3106,15 @@ function commitRootImpl(
// hydration lanes in this check, because render triggered by selective
// hydration is conceptually not an update.
if (
// Check if there was a recursive update spawned by this render, in either
// the render phase or the commit phase. We track these explicitly because
// we can't infer from the remaining lanes alone.
(enableInfiniteRenderLoopDetection &&
(didIncludeRenderPhaseUpdate || didIncludeCommitPhaseUpdate)) ||
// Was the finished render the result of an update (not hydration)?
includesSomeLane(lanes, UpdateLanes) &&
// Did it schedule a sync update?
includesSomeLane(remainingLanes, SyncUpdateLanes)
(includesSomeLane(lanes, UpdateLanes) &&
// Did it schedule a sync update?
includesSomeLane(remainingLanes, SyncUpdateLanes))
) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
markNestedUpdateScheduled();
Expand Down Expand Up @@ -3582,6 +3657,19 @@ export function throwIfInfiniteUpdateLoopDetected() {
rootWithNestedUpdates = null;
rootWithPassiveNestedUpdates = null;

if (enableInfiniteRenderLoopDetection) {
if (executionContext & RenderContext && workInProgressRoot !== null) {
// We're in the render phase. Disable the concurrent error recovery
// mechanism to ensure that the error we're about to throw gets handled.
// We need it to trigger the nearest error boundary so that the infinite
// update loop is broken.
workInProgressRoot.errorRecoveryDisabledLanes = mergeLanes(
workInProgressRoot.errorRecoveryDisabledLanes,
workInProgressRootRenderLanes,
);
}
}

throw new Error(
'Maximum update depth exceeded. This can happen when a component ' +
'repeatedly calls setState inside componentWillUpdate or ' +
Expand Down
6 changes: 6 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,12 @@ export const disableClientCache = false;
// Changes Server Components Reconciliation when they have keys
export const enableServerComponentKeys = __NEXT_MAJOR__;

/**
* Enables a new error detection for infinite render loops from updates caused
* by setState or similar outside of the component owning the state.
*/
export const enableInfiniteRenderLoopDetection = true;

// -----------------------------------------------------------------------------
// Chopping Block
//
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ export const enableUseDeferredValueInitialArg = true;
export const disableClientCache = true;

export const enableServerComponentKeys = true;
export const enableInfiniteRenderLoopDetection = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export const useModernStrictMode = false;
export const enableDO_NOT_USE_disableStrictPassiveEffect = false;
export const enableFizzExternalRuntime = false;
export const enableDeferRootSchedulingToMicrotask = false;
export const enableInfiniteRenderLoopDetection = false;

export const enableAsyncActions = false;

Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__;
export const disableClientCache = true;

export const enableServerComponentKeys = true;
export const enableInfiniteRenderLoopDetection = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export const enableUseMemoCacheHook = true;
export const enableUseEffectEventHook = false;
export const enableClientRenderFallbackOnTextMismatch = true;
export const enableUseRefAccessWarning = false;
export const enableInfiniteRenderLoopDetection = false;

export const enableRetryLaneExpiration = false;
export const retryLaneExpirationMs = 5000;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export const enableUseDeferredValueInitialArg = true;
export const disableClientCache = true;

export const enableServerComponentKeys = true;
export const enableInfiniteRenderLoopDetection = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ export const enableDebugTracing = __EXPERIMENTAL__;

export const enableSchedulingProfiler = __VARIANT__;

export const enableInfiniteRenderLoopDetection = __VARIANT__;

// These are already tested in both modes using the build type dimension,
// so we don't need to use __VARIANT__ to get extra coverage.
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const {
retryLaneExpirationMs,
syncLaneExpirationMs,
transitionLaneExpirationMs,
enableInfiniteRenderLoopDetection,
} = dynamicFeatureFlags;

// On WWW, __EXPERIMENTAL__ is used for a new modern build.
Expand Down

0 comments on commit 61ac209

Please sign in to comment.