@samitouri / QOS-React-2 / commits / 7f362de158

Revert "Fix: Detect infinite update loops caused by render phase updates (#26625)" (#27027)

This reverts commit 822386f252fd1f0e949efa904a1ed790133329f7. This broke a number of tests when synced internally. We'll need to investigate the breakages before relanding this.

Jan Kassens committed Jun 30, 2023 at 12:51 UTC 7f362de1588d98438787d652941533e21f2f332d

4 files changed +14 -207

packages/react-dom/src/__tests__/ReactUpdates-test.js

-58

@@ -1620,64 +1620,6 @@ describe('ReactUpdates', () => {

1620 });

1621 });

1622

1623 - it("does not infinite loop if there's a synchronous render phase update on another component", () => {

1624 - let setState;

1625 - function App() {

1626 - const [, _setState] = React.useState(0);

1627 - setState = _setState;

1628 - return <Child />;

1629 - }

1630 -

1631 - function Child(step) {

1632 - // This will cause an infinite update loop, and a warning in dev.

1633 - setState(n => n + 1);

1634 - return null;

1635 - }

1636 -

1637 - const container = document.createElement('div');

1638 - const root = ReactDOMClient.createRoot(container);

1639 -

1640 - expect(() => {

1641 - expect(() => ReactDOM.flushSync(() => root.render(<App />))).toThrow(

1642 - 'Maximum update depth exceeded',

1643 - );

1644 - }).toErrorDev(

1645 - 'Warning: Cannot update a component (`App`) while rendering a different component (`Child`)',

1646 - );

1647 - });

1648 -

1649 - it("does not infinite loop if there's an async render phase update on another component", async () => {

1650 - let setState;

1651 - function App() {

1652 - const [, _setState] = React.useState(0);

1653 - setState = _setState;

1654 - return <Child />;

1655 - }

1656 -

1657 - function Child(step) {

1658 - // This will cause an infinite update loop, and a warning in dev.

1659 - setState(n => n + 1);

1660 - return null;

1661 - }

1662 -

1663 - const container = document.createElement('div');

1664 - const root = ReactDOMClient.createRoot(container);

1665 -

1666 - await expect(async () => {

1667 - let error;

1668 - try {

1669 - await act(() => {

1670 - React.startTransition(() => root.render(<App />));

1671 - });

1672 - } catch (e) {

1673 - error = e;

1674 - }

1675 - expect(error.message).toMatch('Maximum update depth exceeded');

1676 - }).toErrorDev(

1677 - 'Warning: Cannot update a component (`App`) while rendering a different component (`Child`)',

1678 - );

1679 - });

1680 -

1623 // TODO: Replace this branch with @gate pragmas

1624 if (__DEV__) {

1625 it('warns about a deferred infinite update loop with useEffect', async () => {

packages/react-reconciler/src/ReactFiberRootScheduler.js

-49

@@ -139,18 +139,6 @@ export function ensureRootIsScheduled(root: FiberRoot): void {

139 }

140 }

141

142 -function unscheduleAllRoots() {

143 - // This is only done in a fatal error situation, as a last resort to prevent

144 - // an infinite render loop.

145 - let root = firstScheduledRoot;

146 - while (root !== null) {

147 - const next = root.next;

148 - root.next = null;

149 - root = next;

150 - }

151 - firstScheduledRoot = lastScheduledRoot = null;

152 -}

153 -

142 export function flushSyncWorkOnAllRoots() {

143 // This is allowed to be called synchronously, but the caller should check

144 // the execution context first.

@@ -181,47 +169,10 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) {

169

170 // There may or may not be synchronous work scheduled. Let's check.

171 let didPerformSomeWork;

184 - let nestedUpdatePasses = 0;

172 let errors: Array<mixed> | null = null;

173 isFlushingWork = true;

174 do {

175 didPerformSomeWork = false;

189 -

190 - // This outer loop re-runs if performing sync work on a root spawns

191 - // additional sync work. If it happens too many times, it's very likely

192 - // caused by some sort of infinite update loop. We already have a loop guard

193 - // in place that will trigger an error on the n+1th update, but it's

194 - // possible for that error to get swallowed if the setState is called from

195 - // an unexpected place, like during the render phase. So as an added

196 - // precaution, we also use a guard here.

197 - //

198 - // Ideally, there should be no known way to trigger this synchronous loop.

199 - // It's really just here as a safety net.

200 - //

201 - // This limit is slightly larger than the one that throws inside setState,

202 - // because that one is preferable because it includes a componens stack.

203 - if (++nestedUpdatePasses > 60) {

204 - // This is a fatal error, so we'll unschedule all the roots.

205 - unscheduleAllRoots();

206 - // TODO: Change this error message to something different to distinguish

207 - // it from the one that is thrown from setState. Those are less fatal

208 - // because they usually will result in the bad component being unmounted,

209 - // and an error boundary being triggered, rather than us having to

210 - // forcibly stop the entire scheduler.

211 - const infiniteUpdateError = new Error(

212 - 'Maximum update depth exceeded. This can happen when a component ' +

213 - 'repeatedly calls setState inside componentWillUpdate or ' +

214 - 'componentDidUpdate. React limits the number of nested updates to ' +

215 - 'prevent infinite loops.',

216 - );

217 - if (errors === null) {

218 - errors = [infiniteUpdateError];

219 - } else {

220 - errors.push(infiniteUpdateError);

221 - }

222 - break;

223 - }

224 -

176 let root = firstScheduledRoot;

177 while (root !== null) {

178 if (onlyLegacy && root.tag !== LegacyRoot) {

packages/react-reconciler/src/ReactFiberWorkLoop.js

+9 -91

@@ -141,9 +141,9 @@ import {

141 includesExpiredLane,

142 getNextLanes,

143 getLanesToRetrySynchronouslyOnError,

144 - markRootSuspended as _markRootSuspended,

145 - markRootUpdated as _markRootUpdated,

146 - markRootPinged as _markRootPinged,

144 + markRootUpdated,

145 + markRootSuspended as markRootSuspended_dontCallThisOneDirectly,

146 + markRootPinged,

147 markRootEntangled,

148 markRootFinished,

149 addFiberToLanesMap,

@@ -370,13 +370,6 @@ let workInProgressRootConcurrentErrors: Array<CapturedValue<mixed>> | null =

370 let workInProgressRootRecoverableErrors: Array<CapturedValue<mixed>> | null =

371 null;

372

373 -// Tracks when an update occurs during the render phase.

374 -let workInProgressRootDidIncludeRecursiveRenderUpdate: boolean = false;

375 -// Thacks when an update occurs during the commit phase. It's a separate

376 -// variable from the one for renders because the commit phase may run

377 -// concurrently to a render phase.

378 -let didIncludeCommitPhaseUpdate: boolean = false;

379 -

373 // The most recent time we either committed a fallback, or when a fallback was

374 // filled in with the resolved UI. This lets us throttle the appearance of new

375 // content as it streams in, to minimize jank.

@@ -1121,7 +1114,6 @@ function finishConcurrentRender(

1114 root,

1115 workInProgressRootRecoverableErrors,

1116 workInProgressTransitions,

1124 - workInProgressRootDidIncludeRecursiveRenderUpdate,

1117 );

1118 } else {

1119 if (

@@ -1156,7 +1148,6 @@ function finishConcurrentRender(

1148 finishedWork,

1149 workInProgressRootRecoverableErrors,

1150 workInProgressTransitions,

1159 - workInProgressRootDidIncludeRecursiveRenderUpdate,

1151 lanes,

1152 ),

1153 msUntilTimeout,

@@ -1169,7 +1160,6 @@ function finishConcurrentRender(

1160 finishedWork,

1161 workInProgressRootRecoverableErrors,

1162 workInProgressTransitions,

1172 - workInProgressRootDidIncludeRecursiveRenderUpdate,

1163 lanes,

1164 );

1165 }

@@ -1180,7 +1170,6 @@ function commitRootWhenReady(

1170 finishedWork: Fiber,

1171 recoverableErrors: Array<CapturedValue<mixed>> | null,

1172 transitions: Array<Transition> | null,

1183 - didIncludeRenderPhaseUpdate: boolean,

1173 lanes: Lanes,

1174 ) {

1175 // TODO: Combine retry throttling with Suspensey commits. Right now they run

@@ -1207,13 +1196,7 @@ function commitRootWhenReady(

1196 // us that it's ready. This will be canceled if we start work on the

1197 // root again.

1198 root.cancelPendingCommit = schedulePendingCommit(

1210 - commitRoot.bind(

1211 - null,

1212 - root,

1213 - recoverableErrors,

1214 - transitions,

1215 - didIncludeRenderPhaseUpdate,

1216 - ),

1199 + commitRoot.bind(null, root, recoverableErrors, transitions),

1200 );

1201 markRootSuspended(root, lanes);

1202 return;

@@ -1221,7 +1204,7 @@ function commitRootWhenReady(

1204 }

1205

1206 // Otherwise, commit immediately.

1224 - commitRoot(root, recoverableErrors, transitions, didIncludeRenderPhaseUpdate);

1207 + commitRoot(root, recoverableErrors, transitions);

1208 }

1209

1210 function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {

@@ -1277,51 +1260,17 @@ function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {

1260 return true;

1261 }

1262

1280 -// The extra indirections around markRootUpdated and markRootSuspended is

1281 -// needed to avoid a circular dependency between this module and

1282 -// ReactFiberLane. There's probably a better way to split up these modules and

1283 -// avoid this problem. Perhaps all the root-marking functions should move into

1284 -// the work loop.

1285 -

1286 -function markRootUpdated(root: FiberRoot, updatedLanes: Lanes) {

1287 - _markRootUpdated(root, updatedLanes);

1288 -

1289 - // Check for recursive updates

1290 - if (executionContext & RenderContext) {

1291 - workInProgressRootDidIncludeRecursiveRenderUpdate = true;

1292 - } else if (executionContext & CommitContext) {

1293 - didIncludeCommitPhaseUpdate = true;

1294 - }

1295 -

1296 - throwIfInfiniteUpdateLoopDetected();

1297 -}

1298 -

1299 -function markRootPinged(root: FiberRoot, pingedLanes: Lanes) {

1300 - _markRootPinged(root, pingedLanes);

1301 -

1302 - // Check for recursive pings. Pings are conceptually different from updates in

1303 - // other contexts but we call it an "update" in this context because

1304 - // repeatedly pinging a suspended render can cause a recursive render loop.

1305 - // The relevant property is that it can result in a new render attempt

1306 - // being scheduled.

1307 - if (executionContext & RenderContext) {

1308 - workInProgressRootDidIncludeRecursiveRenderUpdate = true;

1309 - } else if (executionContext & CommitContext) {

1310 - didIncludeCommitPhaseUpdate = true;

1311 - }

1312 -

1313 - throwIfInfiniteUpdateLoopDetected();

1314 -}

1315 -

1263 function markRootSuspended(root: FiberRoot, suspendedLanes: Lanes) {

1264 // When suspending, we should always exclude lanes that were pinged or (more

1265 // rarely, since we try to avoid it) updated during the render phase.

1266 + // TODO: Lol maybe there's a better way to factor this besides this

1267 + // obnoxiously named function :)

1268 suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes);

1269 suspendedLanes = removeLanes(

1270 suspendedLanes,

1271 workInProgressRootInterleavedUpdatedLanes,

1272 );

1324 - _markRootSuspended(root, suspendedLanes);

1273 + markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes);

1274 }

1275

1276 // This is the entry point for synchronous tasks that don't go

@@ -1392,7 +1341,6 @@ export function performSyncWorkOnRoot(root: FiberRoot): null {

1341 root,

1342 workInProgressRootRecoverableErrors,

1343 workInProgressTransitions,

1395 - workInProgressRootDidIncludeRecursiveRenderUpdate,

1344 );

1345

1346 // Before exiting, make sure there's a callback scheduled for the next

@@ -1607,7 +1555,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {

1555 workInProgressRootPingedLanes = NoLanes;

1556 workInProgressRootConcurrentErrors = null;

1557 workInProgressRootRecoverableErrors = null;

1610 - workInProgressRootDidIncludeRecursiveRenderUpdate = false;

1558

1559 finishQueueingConcurrentUpdates();

1560

@@ -2649,7 +2596,6 @@ function commitRoot(

2596 root: FiberRoot,

2597 recoverableErrors: null | Array<CapturedValue<mixed>>,

2598 transitions: Array<Transition> | null,

2652 - didIncludeRenderPhaseUpdate: boolean,

2599 ) {

2600 // TODO: This no longer makes any sense. We already wrap the mutation and

2601 // layout phases. Should be able to remove.

@@ -2663,7 +2609,6 @@ function commitRoot(

2609 root,

2610 recoverableErrors,

2611 transitions,

2666 - didIncludeRenderPhaseUpdate,

2612 previousUpdateLanePriority,

2613 );

2614 } finally {

@@ -2678,7 +2623,6 @@ function commitRootImpl(

2623 root: FiberRoot,

2624 recoverableErrors: null | Array<CapturedValue<mixed>>,

2625 transitions: Array<Transition> | null,

2681 - didIncludeRenderPhaseUpdate: boolean,

2626 renderPriorityLevel: EventPriority,

2627 ) {

2628 do {

@@ -2758,9 +2702,6 @@ function commitRootImpl(

2702

2703 markRootFinished(root, remainingLanes);

2704

2761 - // Reset this before firing side effects so we can detect recursive updates.

2762 - didIncludeCommitPhaseUpdate = false;

2763 -

2705 if (root === workInProgressRoot) {

2706 // We can reset these now that they are finished.

2707 workInProgressRoot = null;

@@ -3007,19 +2948,7 @@ function commitRootImpl(

2948

2949 // Read this again, since a passive effect might have updated it

2950 remainingLanes = root.pendingLanes;

3010 - if (

3011 - // Check if there was a recursive update spawned by this render, in either

3012 - // the render phase or the commit phase. We track these explicitly because

3013 - // we can't infer from the remaining lanes alone.

3014 - didIncludeCommitPhaseUpdate ||

3015 - didIncludeRenderPhaseUpdate ||

3016 - // As an additional precaution, we also check if there's any remaining sync

3017 - // work. Theoretically this should be unreachable but if there's a mistake

3018 - // in React it helps to be overly defensive given how hard it is to debug

3019 - // those scenarios otherwise. This won't catch recursive async updates,

3020 - // though, which is why we check the flags above first.

3021 - includesSyncLane(remainingLanes)

3022 - ) {

2951 + if (includesSyncLane(remainingLanes)) {

2952 if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {

2953 markNestedUpdateScheduled();

2954 }

@@ -3561,17 +3490,6 @@ export function throwIfInfiniteUpdateLoopDetected() {

3490 rootWithNestedUpdates = null;

3491 rootWithPassiveNestedUpdates = null;

3492

3564 - if (executionContext & RenderContext && workInProgressRoot !== null) {

3565 - // We're in the render phase. Disable the concurrent error recovery

3566 - // mechanism to ensure that the error we're about to throw gets handled.

3567 - // We need it to trigger the nearest error boundary so that the infinite

3568 - // update loop is broken.

3569 - workInProgressRoot.errorRecoveryDisabledLanes = mergeLanes(

3570 - workInProgressRoot.errorRecoveryDisabledLanes,

3571 - workInProgressRootRenderLanes,

3572 - );

3573 - }

3574 -

3493 throw new Error(

3494 'Maximum update depth exceeded. This can happen when a component ' +

3495 'repeatedly calls setState inside componentWillUpdate or ' +

scripts/jest/matchers/toWarnDev.js

+5 -9

@@ -69,16 +69,12 @@ const createMatcherFor = (consoleMethod, matcherName) =>

69 (message.includes('\n in ') || message.includes('\n at '));

71 const consoleSpy = (format, ...args) => {

72 + // Ignore uncaught errors reported by jsdom

73 + // and React addendums because they're too noisy.

74 if (

73 - // Ignore uncaught errors reported by jsdom

74 - // and React addendums because they're too noisy.

75 - (!logAllErrors &&

76 - consoleMethod === 'error' &&

77 - shouldIgnoreConsoleError(format, args)) ||

78 - // Ignore error objects passed to console.error, which we sometimes

79 - // use as a fallback behavior, like when reportError

80 - // isn't available.

81 - typeof format !== 'string'

75 + !logAllErrors &&

76 + consoleMethod === 'error' &&

77 + shouldIgnoreConsoleError(format, args)

78 ) {

79 return;

80 }