@samitouri / QOS-React-2 / commits / dc27b5aaae

useMutableSource: Use StrictMode double render to detect render phase mutation (#20698)

* Concurrent Mode test for uMS render mutation Same test as the one added in #20665, but for Concurrent Mode. * Use double render to detect render phase mutation PR #20665 added a mechanism to detect when a `useMutableSource` source is mutated during the render phase. It relies on the fact that we double invoke components that error during development using `invokeGuardedCallback`. If the version in the double render doesn't match the first, that indicates there must have been a mutation during render. At first I thought it worked by detecting inside the *other* double render, the one we do for Strict Mode. It turns out that while it does warn then, the warning is suppressed, because we suppress all console methods that occur during the Strict Mode double render. So it's really the `invokeGuardedCallback` one that makes it work. Anyway, let's set that aside that issue for a second. I realized during review that errors that occur during the Strict Mode double render reveal a useful property: A pure component will never throw during the double render, because if it were pure, it would have also thrown during the first render... in which case it wouldn't have double rendered! This is true of all such errors, not just the one thrown by `useMutableSource`. Given this, we can simplify the `useMutableSource` mutation detection mechanism. Instead of tracking and comparing the source's version, we can instead check if we're inside a double render when the error is thrown. To get around the console suppression issue, I changed the warning to an error. It errors regardless, in both dev and prod, so it doesn't have semantic implications. However, because of the paradox I described above, we arguably _shouldn't_ throw an error in development, since we know that error won't happen in production, because prod doesn't double render. (It's still a tearing bug, but that doesn't mean the component will actually throw.) I considered that, but that requires a larger conversation about how to handle errors that we know are only possible in development. I think we should probably be suppressing *all* errors (with a warning) that occur during a double render.

Andrew Clark committed Feb 1, 2021 at 14:11 UTC dc27b5aaae5e1a028dc77412d5396466aaa87268
4 files changed +115 -49
packages/react-reconciler/src/ReactFiberHooks.new.js
+31 -16
@@ -904,18 +904,6 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
904 const getVersion = source._getVersion;
905 const version = getVersion(source._source);
906
907 - let mutableSourceSideEffectDetected = false;
908 - if (__DEV__) {
909 - // Detect side effects that update a mutable source during render.
910 - // See https://github.com/facebook/react/issues/19948
911 - if (source._currentlyRenderingFiber !== currentlyRenderingFiber) {
912 - source._currentlyRenderingFiber = currentlyRenderingFiber;
913 - source._initialVersionAsOfFirstRender = version;
914 - } else if (source._initialVersionAsOfFirstRender !== version) {
915 - mutableSourceSideEffectDetected = true;
916 - }
917 - }
918 -
907 // Is it safe for this component to read from this source during the current render?
908 let isSafeToReadFromSource = false;
909
@@ -978,17 +966,44 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
966 // but there's nothing we can do about that (short of throwing here and refusing to continue the render).
967 markSourceAsDirty(source);
968
969 + // Intentioally throw an error to force React to retry synchronously. During
970 + // the synchronous retry, it will block interleaved mutations, so we should
971 + // get a consistent read. Therefore, the following error should never be
972 + // visible to the user.
973 + //
974 + // If it were to become visible to the user, it suggests one of two things:
975 + // a bug in React, or (more likely), a mutation during the render phase that
976 + // caused the second re-render attempt to be different from the first.
977 + //
978 + // We know it's the second case if the logs are currently disabled. So in
979 + // dev, we can present a more accurate error message.
980 if (__DEV__) {
982 - if (mutableSourceSideEffectDetected) {
981 + // eslint-disable-next-line react-internal/no-production-logging
982 + if (console.log.__reactDisabledLog) {
983 + // If the logs are disabled, this is the dev-only double render. This is
984 + // only reachable if there was a mutation during render. Show a helpful
985 + // error message.
986 + //
987 + // Something interesting to note: because we only double render in
988 + // development, this error will never happen during production. This is
989 + // actually true of all errors that occur during a double render,
990 + // because if the first render had thrown, we would have exited the
991 + // begin phase without double rendering. We should consider suppressing
992 + // any error from a double render (with a warning) to more closely match
993 + // the production behavior.
994 const componentName = getComponentName(currentlyRenderingFiber.type);
984 - console.warn(
985 - 'A mutable source was mutated while the %s component was rendering. This is not supported. ' +
986 - 'Move any mutations into event handlers or effects.',
995 + invariant(
996 + false,
997 + 'A mutable source was mutated while the %s component was rendering. ' +
998 + 'This is not supported. Move any mutations into event handlers ' +
999 + 'or effects.',
1000 componentName,
1001 );
1002 }
1003 }
1004
1005 + // We expect this error not to be thrown during the synchronous retry,
1006 + // because we blocked interleaved mutations.
1007 invariant(
1008 false,
1009 'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.',
packages/react-reconciler/src/ReactFiberHooks.old.js
+31 -16
@@ -885,18 +885,6 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
885 const getVersion = source._getVersion;
886 const version = getVersion(source._source);
887
888 - let mutableSourceSideEffectDetected = false;
889 - if (__DEV__) {
890 - // Detect side effects that update a mutable source during render.
891 - // See https://github.com/facebook/react/issues/19948
892 - if (source._currentlyRenderingFiber !== currentlyRenderingFiber) {
893 - source._currentlyRenderingFiber = currentlyRenderingFiber;
894 - source._initialVersionAsOfFirstRender = version;
895 - } else if (source._initialVersionAsOfFirstRender !== version) {
896 - mutableSourceSideEffectDetected = true;
897 - }
898 - }
899 -
888 // Is it safe for this component to read from this source during the current render?
889 let isSafeToReadFromSource = false;
890
@@ -959,17 +947,44 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
947 // but there's nothing we can do about that (short of throwing here and refusing to continue the render).
948 markSourceAsDirty(source);
949
950 + // Intentioally throw an error to force React to retry synchronously. During
951 + // the synchronous retry, it will block interleaved mutations, so we should
952 + // get a consistent read. Therefore, the following error should never be
953 + // visible to the user.
954 + //
955 + // If it were to become visible to the user, it suggests one of two things:
956 + // a bug in React, or (more likely), a mutation during the render phase that
957 + // caused the second re-render attempt to be different from the first.
958 + //
959 + // We know it's the second case if the logs are currently disabled. So in
960 + // dev, we can present a more accurate error message.
961 if (__DEV__) {
963 - if (mutableSourceSideEffectDetected) {
962 + // eslint-disable-next-line react-internal/no-production-logging
963 + if (console.log.__reactDisabledLog) {
964 + // If the logs are disabled, this is the dev-only double render. This is
965 + // only reachable if there was a mutation during render. Show a helpful
966 + // error message.
967 + //
968 + // Something interesting to note: because we only double render in
969 + // development, this error will never happen during production. This is
970 + // actually true of all errors that occur during a double render,
971 + // because if the first render had thrown, we would have exited the
972 + // begin phase without double rendering. We should consider suppressing
973 + // any error from a double render (with a warning) to more closely match
974 + // the production behavior.
975 const componentName = getComponentName(currentlyRenderingFiber.type);
965 - console.warn(
966 - 'A mutable source was mutated while the %s component was rendering. This is not supported. ' +
967 - 'Move any mutations into event handlers or effects.',
976 + invariant(
977 + false,
978 + 'A mutable source was mutated while the %s component was rendering. ' +
979 + 'This is not supported. Move any mutations into event handlers ' +
980 + 'or effects.',
981 componentName,
982 );
983 }
984 }
985
986 + // We expect this error not to be thrown during the synchronous retry,
987 + // because we blocked interleaved mutations.
988 invariant(
989 false,
990 'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.',
packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
+51 -15
@@ -1724,6 +1724,46 @@ describe('useMutableSource', () => {
1724 describe('side effecte detection', () => {
1725 // @gate experimental
1726 it('should throw if a mutable source is mutated during render', () => {
1727 + const source = createSource(0);
1728 + const mutableSource = createMutableSource(
1729 + source,
1730 + param => param.version,
1731 + );
1732 +
1733 + let mutatedValueInRender = 1;
1734 + function MutateDuringRead() {
1735 + const value = useMutableSource(
1736 + mutableSource,
1737 + defaultGetSnapshot,
1738 + defaultSubscribe,
1739 + );
1740 + Scheduler.unstable_yieldValue('MutateDuringRead:' + value);
1741 + // Note that mutating an exeternal value during render is a side effect and is not supported.
1742 + source.value = mutatedValueInRender++;
1743 + return null;
1744 + }
1745 +
1746 + expect(() => {
1747 + act(() => {
1748 + ReactNoop.render(<MutateDuringRead />);
1749 + });
1750 + }).toThrow(
1751 + 'A mutable source was mutated while the MutateDuringRead component ' +
1752 + 'was rendering. This is not supported. Move any mutations into ' +
1753 + 'event handlers or effects.',
1754 + );
1755 +
1756 + expect(Scheduler).toHaveYielded([
1757 + // First attempt
1758 + 'MutateDuringRead:0',
1759 +
1760 + // Synchronous retry
1761 + 'MutateDuringRead:1',
1762 + ]);
1763 + });
1764 +
1765 + // @gate experimental
1766 + it('should throw if a mutable source is mutated during render (legacy mode)', () => {
1767 const source = createSource('initial');
1768 const mutableSource = createMutableSource(
1769 source,
@@ -1745,21 +1785,17 @@ describe('useMutableSource', () => {
1785 }
1786
1787 expect(() => {
1748 - expect(() => {
1749 - act(() => {
1750 - ReactNoop.renderLegacySyncRoot(
1751 - <React.StrictMode>
1752 - <MutateDuringRead />
1753 - </React.StrictMode>,
1754 - );
1755 - });
1756 - }).toThrow(
1757 - 'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.',
1758 - );
1759 - }).toWarnDev(
1760 - 'A mutable source was mutated while the MutateDuringRead component was rendering. This is not supported. ' +
1761 - 'Move any mutations into event handlers or effects.\n' +
1762 - ' in MutateDuringRead (at **)',
1788 + act(() => {
1789 + ReactNoop.renderLegacySyncRoot(
1790 + <React.StrictMode>
1791 + <MutateDuringRead />
1792 + </React.StrictMode>,
1793 + );
1794 + });
1795 + }).toThrow(
1796 + 'A mutable source was mutated while the MutateDuringRead component ' +
1797 + 'was rendering. This is not supported. Move any mutations into ' +
1798 + 'event handlers or effects.',
1799 );
1800
1801 expect(Scheduler).toHaveYielded(['MutateDuringRead:initial']);
scripts/error-codes/codes.json
+2 -2
@@ -339,7 +339,7 @@
339 "345": "Root did not complete. This is a bug in React.",
340 "348": "ensureListeningTo(): received a container that was not an element node. This is likely a bug in React.",
341 "349": "Expected a work-in-progress root. This is a bug in React. Please file an issue.",
342 - "350": "Cannot read from mutable source during the current render without tearing. This is a bug in React. Please file an issue.",
342 + "350": "Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.",
343 "351": "Unsupported server component type: %s",
344 "352": "React Lazy Components are not yet supported on the server.",
345 "353": "A server block should never encode any other slots. This is a bug in React.",
@@ -373,5 +373,5 @@
373 "382": "This query has received more parameters than the last time the same query was used. Always pass the exact number of parameters that the query needs.",
374 "383": "This query has received fewer parameters than the last time the same query was used. Always pass the exact number of parameters that the query needs.",
375 "384": "Refreshing the cache is not supported in Server Components.",
376 - "385": "Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue."
376 + "385": "A mutable source was mutated while the %s component was rendering. This is not supported. Move any mutations into event handlers or effects."
377 }