Skip to content

Commit

Permalink
Revert "Fix: Detect infinite update loops caused by render phase upda…
Browse files Browse the repository at this point in the history
…tes (facebook#26625)" (facebook#27027)

This reverts commit 822386f.

This broke a number of tests when synced internally. We'll need to
investigate the breakages before relanding this.
kassens authored and AndyPengc12 committed Apr 15, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 32d0c31 commit 40f9658
Showing 4 changed files with 14 additions and 207 deletions.
58 changes: 0 additions & 58 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
Original file line number Diff line number Diff line change
@@ -1620,64 +1620,6 @@ describe('ReactUpdates', () => {
});
});

it("does not infinite loop if there's a synchronous render phase update on another component", () => {
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 () => {
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 () => {
49 changes: 0 additions & 49 deletions packages/react-reconciler/src/ReactFiberRootScheduler.js
Original file line number Diff line number Diff line change
@@ -139,18 +139,6 @@ export function ensureRootIsScheduled(root: FiberRoot): void {
}
}

function unscheduleAllRoots() {
// This is only done in a fatal error situation, as a last resort to prevent
// an infinite render loop.
let root = firstScheduledRoot;
while (root !== null) {
const next = root.next;
root.next = null;
root = next;
}
firstScheduledRoot = lastScheduledRoot = null;
}

export function flushSyncWorkOnAllRoots() {
// This is allowed to be called synchronously, but the caller should check
// the execution context first.
@@ -181,47 +169,10 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) {

// There may or may not be synchronous work scheduled. Let's check.
let didPerformSomeWork;
let nestedUpdatePasses = 0;
let errors: Array<mixed> | null = null;
isFlushingWork = true;
do {
didPerformSomeWork = false;

// This outer loop re-runs if performing sync work on a root spawns
// additional sync work. If it happens too many times, it's very likely
// caused by some sort of infinite update loop. We already have a loop guard
// in place that will trigger an error on the n+1th update, but it's
// possible for that error to get swallowed if the setState is called from
// an unexpected place, like during the render phase. So as an added
// precaution, we also use a guard here.
//
// Ideally, there should be no known way to trigger this synchronous loop.
// It's really just here as a safety net.
//
// This limit is slightly larger than the one that throws inside setState,
// because that one is preferable because it includes a componens stack.
if (++nestedUpdatePasses > 60) {
// This is a fatal error, so we'll unschedule all the roots.
unscheduleAllRoots();
// TODO: Change this error message to something different to distinguish
// it from the one that is thrown from setState. Those are less fatal
// because they usually will result in the bad component being unmounted,
// and an error boundary being triggered, rather than us having to
// forcibly stop the entire scheduler.
const infiniteUpdateError = new Error(
'Maximum update depth exceeded. This can happen when a component ' +
'repeatedly calls setState inside componentWillUpdate or ' +
'componentDidUpdate. React limits the number of nested updates to ' +
'prevent infinite loops.',
);
if (errors === null) {
errors = [infiniteUpdateError];
} else {
errors.push(infiniteUpdateError);
}
break;
}

let root = firstScheduledRoot;
while (root !== null) {
if (onlyLegacy && root.tag !== LegacyRoot) {
100 changes: 9 additions & 91 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
@@ -141,9 +141,9 @@ import {
includesExpiredLane,
getNextLanes,
getLanesToRetrySynchronouslyOnError,
markRootSuspended as _markRootSuspended,
markRootUpdated as _markRootUpdated,
markRootPinged as _markRootPinged,
markRootUpdated,
markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
markRootPinged,
markRootEntangled,
markRootFinished,
addFiberToLanesMap,
@@ -370,13 +370,6 @@ 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.
@@ -1121,7 +1114,6 @@ function finishConcurrentRender(
root,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
);
} else {
if (
@@ -1156,7 +1148,6 @@ function finishConcurrentRender(
finishedWork,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
lanes,
),
msUntilTimeout,
@@ -1169,7 +1160,6 @@ function finishConcurrentRender(
finishedWork,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
lanes,
);
}
@@ -1180,7 +1170,6 @@ function commitRootWhenReady(
finishedWork: Fiber,
recoverableErrors: Array<CapturedValue<mixed>> | null,
transitions: Array<Transition> | null,
didIncludeRenderPhaseUpdate: boolean,
lanes: Lanes,
) {
// TODO: Combine retry throttling with Suspensey commits. Right now they run
@@ -1207,21 +1196,15 @@ 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,
didIncludeRenderPhaseUpdate,
),
commitRoot.bind(null, root, recoverableErrors, transitions),
);
markRootSuspended(root, lanes);
return;
}
}

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

function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {
@@ -1277,51 +1260,17 @@ 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);

// 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);

// 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) {
// 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(root, suspendedLanes);
markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes);
}

// This is the entry point for synchronous tasks that don't go
@@ -1392,7 +1341,6 @@ export function performSyncWorkOnRoot(root: FiberRoot): null {
root,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
);

// Before exiting, make sure there's a callback scheduled for the next
@@ -1607,7 +1555,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {
workInProgressRootPingedLanes = NoLanes;
workInProgressRootConcurrentErrors = null;
workInProgressRootRecoverableErrors = null;
workInProgressRootDidIncludeRecursiveRenderUpdate = false;

finishQueueingConcurrentUpdates();

@@ -2649,7 +2596,6 @@ function commitRoot(
root: FiberRoot,
recoverableErrors: null | Array<CapturedValue<mixed>>,
transitions: Array<Transition> | null,
didIncludeRenderPhaseUpdate: boolean,
) {
// TODO: This no longer makes any sense. We already wrap the mutation and
// layout phases. Should be able to remove.
@@ -2663,7 +2609,6 @@ function commitRoot(
root,
recoverableErrors,
transitions,
didIncludeRenderPhaseUpdate,
previousUpdateLanePriority,
);
} finally {
@@ -2678,7 +2623,6 @@ function commitRootImpl(
root: FiberRoot,
recoverableErrors: null | Array<CapturedValue<mixed>>,
transitions: Array<Transition> | null,
didIncludeRenderPhaseUpdate: boolean,
renderPriorityLevel: EventPriority,
) {
do {
@@ -2758,9 +2702,6 @@ function commitRootImpl(

markRootFinished(root, remainingLanes);

// 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;
@@ -3007,19 +2948,7 @@ function commitRootImpl(

// Read this again, since a passive effect might have updated it
remainingLanes = root.pendingLanes;
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.
didIncludeCommitPhaseUpdate ||
didIncludeRenderPhaseUpdate ||
// As an additional precaution, we also check if there's any remaining sync
// work. Theoretically this should be unreachable but if there's a mistake
// in React it helps to be overly defensive given how hard it is to debug
// those scenarios otherwise. This won't catch recursive async updates,
// though, which is why we check the flags above first.
includesSyncLane(remainingLanes)
) {
if (includesSyncLane(remainingLanes)) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
markNestedUpdateScheduled();
}
@@ -3561,17 +3490,6 @@ export function throwIfInfiniteUpdateLoopDetected() {
rootWithNestedUpdates = null;
rootWithPassiveNestedUpdates = null;

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 ' +
14 changes: 5 additions & 9 deletions scripts/jest/matchers/toWarnDev.js
Original file line number Diff line number Diff line change
@@ -69,16 +69,12 @@ const createMatcherFor = (consoleMethod, matcherName) =>
(message.includes('\n in ') || message.includes('\n at '));

const consoleSpy = (format, ...args) => {
// Ignore uncaught errors reported by jsdom
// and React addendums because they're too noisy.
if (
// Ignore uncaught errors reported by jsdom
// and React addendums because they're too noisy.
(!logAllErrors &&
consoleMethod === 'error' &&
shouldIgnoreConsoleError(format, args)) ||
// Ignore error objects passed to console.error, which we sometimes
// use as a fallback behavior, like when reportError
// isn't available.
typeof format !== 'string'
!logAllErrors &&
consoleMethod === 'error' &&
shouldIgnoreConsoleError(format, args)
) {
return;
}

0 comments on commit 40f9658

Please sign in to comment.