Skip to content

Commit

Permalink
[Bugfix] Prevent infinite update loop caused by a synchronous update …
Browse files Browse the repository at this point in the history
…in a passive effect (#22277)

* Add test that triggers infinite update loop

In 18, passive effects are flushed synchronously if they are the
result of a synchronous update. We have a guard for infinite update
loops that occur in the layout phase, but it doesn't currently work for
synchronous updates from a passive effect.

The reason this probably hasn't come up yet is because synchronous
updates inside the passive effect phase are relatively rare: you either
have to imperatively dispatch a discrete event, like `el.focus`, or you
have to call `ReactDOM.flushSync`, which triggers a warning. (In
general, updates inside a passive effect are not encouraged.)

I discovered this because `useSyncExternalStore` does sometimes
trigger updates inside the passive effect phase.

This commit adds a failing test to prove the issue exists. I will fix
it in the next commit.

* Fix failing test added in previous commit

The way we detect a "nested update" is if there's synchronous work
remaining at the end of the commit phase.

Currently this check happens before we synchronously flush the passive
effects. I moved it to after the effects are fired, so that it detects
whether synchronous work was scheduled in that phase.
  • Loading branch information
acdlite authored Sep 9, 2021
1 parent 24c2e27 commit 8f96c6b
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 34 deletions.
32 changes: 32 additions & 0 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1594,6 +1594,7 @@ describe('ReactUpdates', () => {
});
});

// TODO: Replace this branch with @gate pragmas
if (__DEV__) {
it('warns about a deferred infinite update loop with useEffect', () => {
function NonTerminating() {
Expand Down Expand Up @@ -1684,4 +1685,35 @@ describe('ReactUpdates', () => {
expect(container.textContent).toBe('1000');
});
}

it('prevents infinite update loop triggered by synchronous updates in useEffect', () => {
// Ignore flushSync warning
spyOnDev(console, 'error');

function NonTerminating() {
const [step, setStep] = React.useState(0);
React.useEffect(() => {
// Other examples of synchronous updates in useEffect are imperative
// event dispatches like `el.focus`, or `useSyncExternalStore`, which
// may schedule a synchronous update upon subscribing if it detects
// that the store has been mutated since the initial render.
//
// (Originally I wrote this test using `el.focus` but those errors
// get dispatched in a JSDOM event and I don't know how to "catch" those
// so that they don't fail the test.)
ReactDOM.flushSync(() => {
setStep(step + 1);
});
}, [step]);
return step;
}

const container = document.createElement('div');
const root = ReactDOM.createRoot(container);
expect(() => {
ReactDOM.flushSync(() => {
root.render(<NonTerminating />);
});
}).toThrow('Maximum update depth exceeded');
});
});
45 changes: 28 additions & 17 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -1897,6 +1897,15 @@ function commitRootImpl(root, renderPriorityLevel) {
remainingLanes = root.pendingLanes;

// Check if there's remaining work on this root
// TODO: This is part of the `componentDidCatch` implementation. Its purpose
// is to detect whether something might have called setState inside
// `componentDidCatch`. The mechanism is known to be flawed because `setState`
// inside `componentDidCatch` is itself flawed — that's why we recommend
// `getDerivedStateFromError` instead. However, it could be improved by
// checking if remainingLanes includes Sync work, instead of whether there's
// any work remaining at all (which would also include stuff like Suspense
// retries or transitions). It's been like this for a while, though, so fixing
// it probably isn't that urgent.
if (remainingLanes === NoLanes) {
// If there's no remaining work, we can clear the set of already failed
// error boundaries.
Expand All @@ -1909,23 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) {
}
}

if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
markNestedUpdateScheduled();
}

// Count the number of times the root synchronously re-renders without
// finishing. If there are too many, it indicates an infinite update loop.
if (root === rootWithNestedUpdates) {
nestedUpdateCount++;
} else {
nestedUpdateCount = 0;
rootWithNestedUpdates = root;
}
} else {
nestedUpdateCount = 0;
}

onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel);

if (enableUpdaterTracking) {
Expand Down Expand Up @@ -1964,6 +1956,25 @@ function commitRootImpl(root, renderPriorityLevel) {
flushPassiveEffects();
}

// Read this again, since a passive effect might have updated it
remainingLanes = root.pendingLanes;
if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
markNestedUpdateScheduled();
}

// Count the number of times the root synchronously re-renders without
// finishing. If there are too many, it indicates an infinite update loop.
if (root === rootWithNestedUpdates) {
nestedUpdateCount++;
} else {
nestedUpdateCount = 0;
rootWithNestedUpdates = root;
}
} else {
nestedUpdateCount = 0;
}

// If layout work was scheduled, flush it now.
flushSyncCallbacks();

Expand Down
45 changes: 28 additions & 17 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -1897,6 +1897,15 @@ function commitRootImpl(root, renderPriorityLevel) {
remainingLanes = root.pendingLanes;

// Check if there's remaining work on this root
// TODO: This is part of the `componentDidCatch` implementation. Its purpose
// is to detect whether something might have called setState inside
// `componentDidCatch`. The mechanism is known to be flawed because `setState`
// inside `componentDidCatch` is itself flawed — that's why we recommend
// `getDerivedStateFromError` instead. However, it could be improved by
// checking if remainingLanes includes Sync work, instead of whether there's
// any work remaining at all (which would also include stuff like Suspense
// retries or transitions). It's been like this for a while, though, so fixing
// it probably isn't that urgent.
if (remainingLanes === NoLanes) {
// If there's no remaining work, we can clear the set of already failed
// error boundaries.
Expand All @@ -1909,23 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) {
}
}

if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
markNestedUpdateScheduled();
}

// Count the number of times the root synchronously re-renders without
// finishing. If there are too many, it indicates an infinite update loop.
if (root === rootWithNestedUpdates) {
nestedUpdateCount++;
} else {
nestedUpdateCount = 0;
rootWithNestedUpdates = root;
}
} else {
nestedUpdateCount = 0;
}

onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel);

if (enableUpdaterTracking) {
Expand Down Expand Up @@ -1964,6 +1956,25 @@ function commitRootImpl(root, renderPriorityLevel) {
flushPassiveEffects();
}

// Read this again, since a passive effect might have updated it
remainingLanes = root.pendingLanes;
if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
markNestedUpdateScheduled();
}

// Count the number of times the root synchronously re-renders without
// finishing. If there are too many, it indicates an infinite update loop.
if (root === rootWithNestedUpdates) {
nestedUpdateCount++;
} else {
nestedUpdateCount = 0;
rootWithNestedUpdates = root;
}
} else {
nestedUpdateCount = 0;
}

// If layout work was scheduled, flush it now.
flushSyncCallbacks();

Expand Down

0 comments on commit 8f96c6b

Please sign in to comment.