Detect non-synchronous recursive update loops

This improves our infinite loop detection by explicitly tracking
when an update happens during the render or commit phase.

Previously, to detect recursive update, we would check if there was
synchronous work remaining after the commit phase — because synchronous
work is the highest priority, the only way there could be even more
synchronous work is if it was spawned by that render. But this approach
won't work to detect async update loops.

Author: acdlite

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

@@ -1646,6 +1646,38 @@ describe('ReactUpdates', () => {
     );
   });
 
+  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 () => {

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -143,9 +143,9 @@ import {
   includesExpiredLane,
   getNextLanes,
   getLanesToRetrySynchronouslyOnError,
-  markRootUpdated,
-  markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
-  markRootPinged,
+  markRootSuspended as _markRootSuspended,
+  markRootUpdated as _markRootUpdated,
+  markRootPinged as _markRootPinged,
   markRootEntangled,
   markRootFinished,
   addFiberToLanesMap,
@@ -372,6 +372,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 committed a fallback. This lets us ensure a train
 // model where we don't commit new loading states in too quick succession.
 let globalMostRecentFallbackTime: number = 0;
@@ -1117,6 +1124,7 @@ function finishConcurrentRender(
       root,
       workInProgressRootRecoverableErrors,
       workInProgressTransitions,
+      workInProgressRootDidIncludeRecursiveRenderUpdate,
     );
   } else {
     if (includesOnlyRetries(lanes)) {
@@ -1148,6 +1156,7 @@ function finishConcurrentRender(
             finishedWork,
             workInProgressRootRecoverableErrors,
             workInProgressTransitions,
+            workInProgressRootDidIncludeRecursiveRenderUpdate,
             lanes,
           ),
           msUntilTimeout,
@@ -1160,6 +1169,7 @@ function finishConcurrentRender(
       finishedWork,
       workInProgressRootRecoverableErrors,
       workInProgressTransitions,
+      workInProgressRootDidIncludeRecursiveRenderUpdate,
       lanes,
     );
   }
@@ -1170,6 +1180,7 @@ 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
@@ -1196,15 +1207,21 @@ 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);
       return;
     }
   }
 
   // Otherwise, commit immediately.
-  commitRoot(root, recoverableErrors, transitions);
+  commitRoot(root, recoverableErrors, transitions, didIncludeRenderPhaseUpdate);
 }
 
 function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {
@@ -1260,17 +1277,51 @@ 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_dontCallThisOneDirectly(root, suspendedLanes);
+  _markRootSuspended(root, suspendedLanes);
 }
 
 // This is the entry point for synchronous tasks that don't go
@@ -1341,6 +1392,7 @@ export function performSyncWorkOnRoot(root: FiberRoot): null {
     root,
     workInProgressRootRecoverableErrors,
     workInProgressTransitions,
+    workInProgressRootDidIncludeRecursiveRenderUpdate,
   );
 
   // Before exiting, make sure there's a callback scheduled for the next
@@ -1555,6 +1607,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {
   workInProgressRootPingedLanes = NoLanes;
   workInProgressRootConcurrentErrors = null;
   workInProgressRootRecoverableErrors = null;
+  workInProgressRootDidIncludeRecursiveRenderUpdate = false;
 
   finishQueueingConcurrentUpdates();
 
@@ -2569,6 +2622,7 @@ 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.
@@ -2582,6 +2636,7 @@ function commitRoot(
       root,
       recoverableErrors,
       transitions,
+      didIncludeRenderPhaseUpdate,
       previousUpdateLanePriority,
     );
   } finally {
@@ -2596,6 +2651,7 @@ function commitRootImpl(
   root: FiberRoot,
   recoverableErrors: null | Array<CapturedValue<mixed>>,
   transitions: Array<Transition> | null,
+  didIncludeRenderPhaseUpdate: boolean,
   renderPriorityLevel: EventPriority,
 ) {
   do {
@@ -2675,6 +2731,9 @@ 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;
@@ -2921,7 +2980,19 @@ function commitRootImpl(
 
   // Read this again, since a passive effect might have updated it
   remainingLanes = root.pendingLanes;
-  if (includesSyncLane(remainingLanes)) {
+  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 (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
       markNestedUpdateScheduled();
     }