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 (#26625)" (#27027)

This reverts commit 822386f.

This broke a number of tests when synced internally. We'll need to
investigate the breakages before relanding this.
  • Loading branch information
@kassens
kassens authored Jun 30, 2023
1 parent fc80111 commit 7f362de
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 207 deletions.
58 changes: 0 additions & 58 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
49 changes: 0 additions & 49 deletions packages/react-reconciler/src/ReactFiberRootScheduler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down
100 changes: 9 additions & 91 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -1121,7 +1114,6 @@ function finishConcurrentRender(
root,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
);
} else {
if (
Expand Down Expand Up @@ -1156,7 +1148,6 @@ function finishConcurrentRender(
finishedWork,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
lanes,
),
msUntilTimeout,
Expand All @@ -1169,7 +1160,6 @@ function finishConcurrentRender(
finishedWork,
workInProgressRootRecoverableErrors,
workInProgressTransitions,
workInProgressRootDidIncludeRecursiveRenderUpdate,
lanes,
);
}
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1607,7 +1555,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {
workInProgressRootPingedLanes = NoLanes;
workInProgressRootConcurrentErrors = null;
workInProgressRootRecoverableErrors = null;
workInProgressRootDidIncludeRecursiveRenderUpdate = false;

finishQueueingConcurrentUpdates();

Expand Down Expand Up @@ -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.
Expand All @@ -2663,7 +2609,6 @@ function commitRoot(
root,
recoverableErrors,
transitions,
didIncludeRenderPhaseUpdate,
previousUpdateLanePriority,
);
} finally {
Expand All @@ -2678,7 +2623,6 @@ function commitRootImpl(
root: FiberRoot,
recoverableErrors: null | Array<CapturedValue<mixed>>,
transitions: Array<Transition> | null,
didIncludeRenderPhaseUpdate: boolean,
renderPriorityLevel: EventPriority,
) {
do {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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 ' +
Expand Down
14 changes: 5 additions & 9 deletions scripts/jest/matchers/toWarnDev.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit 7f362de

Please sign in to comment.