@samitouri / QOS-React-2 / commits / 8f96c6b2ac

[Bugfix] Prevent infinite update loop caused by a synchronous update 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.

Andrew Clark committed Sep 9, 2021 at 11:14 UTC 8f96c6b2ac3ff0e38f60c87b1f08deb9993526b5

3 files changed +88 -34

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

+32

@@ -1594,6 +1594,7 @@ describe('ReactUpdates', () => {

1594 });

1595 });

1596

1597 + // TODO: Replace this branch with @gate pragmas

1598 if (__DEV__) {

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

1600 function NonTerminating() {

@@ -1684,4 +1685,35 @@ describe('ReactUpdates', () => {

1685 expect(container.textContent).toBe('1000');

1686 });

1687 }

1688 +

1689 + it('prevents infinite update loop triggered by synchronous updates in useEffect', () => {

1690 + // Ignore flushSync warning

1691 + spyOnDev(console, 'error');

1692 +

1693 + function NonTerminating() {

1694 + const [step, setStep] = React.useState(0);

1695 + React.useEffect(() => {

1696 + // Other examples of synchronous updates in useEffect are imperative

1697 + // event dispatches like `el.focus`, or `useSyncExternalStore`, which

1698 + // may schedule a synchronous update upon subscribing if it detects

1699 + // that the store has been mutated since the initial render.

1700 + //

1701 + // (Originally I wrote this test using `el.focus` but those errors

1702 + // get dispatched in a JSDOM event and I don't know how to "catch" those

1703 + // so that they don't fail the test.)

1704 + ReactDOM.flushSync(() => {

1705 + setStep(step + 1);

1706 + });

1707 + }, [step]);

1708 + return step;

1709 + }

1710 +

1711 + const container = document.createElement('div');

1712 + const root = ReactDOM.createRoot(container);

1713 + expect(() => {

1714 + ReactDOM.flushSync(() => {

1715 + root.render(<NonTerminating />);

1716 + });

1717 + }).toThrow('Maximum update depth exceeded');

1718 + });

1719 });

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

+28 -17

@@ -1897,6 +1897,15 @@ function commitRootImpl(root, renderPriorityLevel) {

1897 remainingLanes = root.pendingLanes;

1898

1899 // Check if there's remaining work on this root

1900 + // TODO: This is part of the `componentDidCatch` implementation. Its purpose

1901 + // is to detect whether something might have called setState inside

1902 + // `componentDidCatch`. The mechanism is known to be flawed because `setState`

1903 + // inside `componentDidCatch` is itself flawed — that's why we recommend

1904 + // `getDerivedStateFromError` instead. However, it could be improved by

1905 + // checking if remainingLanes includes Sync work, instead of whether there's

1906 + // any work remaining at all (which would also include stuff like Suspense

1907 + // retries or transitions). It's been like this for a while, though, so fixing

1908 + // it probably isn't that urgent.

1909 if (remainingLanes === NoLanes) {

1910 // If there's no remaining work, we can clear the set of already failed

1911 // error boundaries.

@@ -1909,23 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) {

1918 }

1919 }

1920

1912 - if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {

1913 - if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {

1914 - markNestedUpdateScheduled();

1915 - }

1916 -

1917 - // Count the number of times the root synchronously re-renders without

1918 - // finishing. If there are too many, it indicates an infinite update loop.

1919 - if (root === rootWithNestedUpdates) {

1920 - nestedUpdateCount++;

1921 - } else {

1922 - nestedUpdateCount = 0;

1923 - rootWithNestedUpdates = root;

1924 - }

1925 - } else {

1926 - nestedUpdateCount = 0;

1927 - }

1928 -

1921 onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel);

1922

1923 if (enableUpdaterTracking) {

@@ -1964,6 +1956,25 @@ function commitRootImpl(root, renderPriorityLevel) {

1956 flushPassiveEffects();

1957 }

1958

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

1960 + remainingLanes = root.pendingLanes;

1961 + if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {

1962 + if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {

1963 + markNestedUpdateScheduled();

1964 + }

1965 +

1966 + // Count the number of times the root synchronously re-renders without

1967 + // finishing. If there are too many, it indicates an infinite update loop.

1968 + if (root === rootWithNestedUpdates) {

1969 + nestedUpdateCount++;

1970 + } else {

1971 + nestedUpdateCount = 0;

1972 + rootWithNestedUpdates = root;

1973 + }

1974 + } else {

1975 + nestedUpdateCount = 0;

1976 + }

1977 +

1978 // If layout work was scheduled, flush it now.

1979 flushSyncCallbacks();

1980

packages/react-reconciler/src/ReactFiberWorkLoop.old.js