Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Feb 1, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28ab856
Concurrent Mode test for uMS render mutation
acdlite Jan 30, 2021
c16fe14
Use double render to detect render phase mutation
acdlite Jan 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 31 additions & 16 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,18 +904,6 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
const getVersion = source._getVersion;
const version = getVersion(source._source);

let mutableSourceSideEffectDetected = false;
if (__DEV__) {
// Detect side effects that update a mutable source during render.
// See https://github.com/facebook/react/issues/19948
if (source._currentlyRenderingFiber !== currentlyRenderingFiber) {
source._currentlyRenderingFiber = currentlyRenderingFiber;
source._initialVersionAsOfFirstRender = version;
} else if (source._initialVersionAsOfFirstRender !== version) {
mutableSourceSideEffectDetected = true;
}
}

// Is it safe for this component to read from this source during the current render?
let isSafeToReadFromSource = false;

Expand Down Expand Up @@ -978,17 +966,44 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
// but there's nothing we can do about that (short of throwing here and refusing to continue the render).
markSourceAsDirty(source);

// Intentioally throw an error to force React to retry synchronously. During
// the synchronous retry, it will block interleaved mutations, so we should
// get a consistent read. Therefore, the following error should never be
// visible to the user.
//
// If it were to become visible to the user, it suggests one of two things:
// a bug in React, or (more likely), a mutation during the render phase that
// caused the second re-render attempt to be different from the first.
//
// We know it's the second case if the logs are currently disabled. So in
// dev, we can present a more accurate error message.
if (__DEV__) {
if (mutableSourceSideEffectDetected) {
// eslint-disable-next-line react-internal/no-production-logging
if (console.log.__reactDisabledLog) {
// If the logs are disabled, this is the dev-only double render. This is
// only reachable if there was a mutation during render. Show a helpful
// error message.
//
// Something interesting to note: because we only double render in
// development, this error will never happen during production. This is
// actually true of all errors that occur during a double render,
// because if the first render had thrown, we would have exited the
// begin phase without double rendering. We should consider suppressing
// any error from a double render (with a warning) to more closely match
// the production behavior.
const componentName = getComponentName(currentlyRenderingFiber.type);
console.warn(
'A mutable source was mutated while the %s component was rendering. This is not supported. ' +
'Move any mutations into event handlers or effects.',
invariant(
false,
'A mutable source was mutated while the %s component was rendering. ' +
'This is not supported. Move any mutations into event handlers ' +
'or effects.',
componentName,
);
}
}

// We expect this error not to be thrown during the synchronous retry,
// because we blocked interleaved mutations.
invariant(
false,
'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.',
Expand Down
47 changes: 31 additions & 16 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,18 +885,6 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
const getVersion = source._getVersion;
const version = getVersion(source._source);

let mutableSourceSideEffectDetected = false;
if (__DEV__) {
// Detect side effects that update a mutable source during render.
// See https://github.com/facebook/react/issues/19948
if (source._currentlyRenderingFiber !== currentlyRenderingFiber) {
source._currentlyRenderingFiber = currentlyRenderingFiber;
source._initialVersionAsOfFirstRender = version;
} else if (source._initialVersionAsOfFirstRender !== version) {
mutableSourceSideEffectDetected = true;
}
}

// Is it safe for this component to read from this source during the current render?
let isSafeToReadFromSource = false;

Expand Down Expand Up @@ -959,17 +947,44 @@ function readFromUnsubcribedMutableSource<Source, Snapshot>(
// but there's nothing we can do about that (short of throwing here and refusing to continue the render).
markSourceAsDirty(source);

// Intentioally throw an error to force React to retry synchronously. During
// the synchronous retry, it will block interleaved mutations, so we should
// get a consistent read. Therefore, the following error should never be
// visible to the user.
//
// If it were to become visible to the user, it suggests one of two things:
// a bug in React, or (more likely), a mutation during the render phase that
// caused the second re-render attempt to be different from the first.
//
// We know it's the second case if the logs are currently disabled. So in
// dev, we can present a more accurate error message.
if (__DEV__) {
if (mutableSourceSideEffectDetected) {
// eslint-disable-next-line react-internal/no-production-logging
if (console.log.__reactDisabledLog) {
// If the logs are disabled, this is the dev-only double render. This is
// only reachable if there was a mutation during render. Show a helpful
// error message.
//
// Something interesting to note: because we only double render in
// development, this error will never happen during production. This is
// actually true of all errors that occur during a double render,
// because if the first render had thrown, we would have exited the
// begin phase without double rendering. We should consider suppressing
// any error from a double render (with a warning) to more closely match
// the production behavior.
const componentName = getComponentName(currentlyRenderingFiber.type);
console.warn(
'A mutable source was mutated while the %s component was rendering. This is not supported. ' +
'Move any mutations into event handlers or effects.',
invariant(
false,
'A mutable source was mutated while the %s component was rendering. ' +
'This is not supported. Move any mutations into event handlers ' +
'or effects.',
componentName,
);
}
}

// We expect this error not to be thrown during the synchronous retry,
// because we blocked interleaved mutations.
invariant(
false,
'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.',
Expand Down
66 changes: 51 additions & 15 deletions packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1724,6 +1724,46 @@ describe('useMutableSource', () => {
describe('side effecte detection', () => {
// @gate experimental
it('should throw if a mutable source is mutated during render', () => {
const source = createSource(0);
const mutableSource = createMutableSource(
source,
param => param.version,
);

let mutatedValueInRender = 1;
function MutateDuringRead() {
const value = useMutableSource(
mutableSource,
defaultGetSnapshot,
defaultSubscribe,
);
Scheduler.unstable_yieldValue('MutateDuringRead:' + value);
// Note that mutating an exeternal value during render is a side effect and is not supported.
source.value = mutatedValueInRender++;
return null;
}

expect(() => {
act(() => {
ReactNoop.render(<MutateDuringRead />);
});
}).toThrow(
'A mutable source was mutated while the MutateDuringRead component ' +
'was rendering. This is not supported. Move any mutations into ' +
'event handlers or effects.',
);

expect(Scheduler).toHaveYielded([
// First attempt
'MutateDuringRead:0',

// Synchronous retry
'MutateDuringRead:1',
]);
});

// @gate experimental
it('should throw if a mutable source is mutated during render (legacy mode)', () => {
const source = createSource('initial');
const mutableSource = createMutableSource(
source,
Expand All @@ -1745,21 +1785,17 @@ describe('useMutableSource', () => {
}

expect(() => {
expect(() => {
act(() => {
ReactNoop.renderLegacySyncRoot(
<React.StrictMode>
<MutateDuringRead />
</React.StrictMode>,
);
});
}).toThrow(
'Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.',
);
}).toWarnDev(
'A mutable source was mutated while the MutateDuringRead component was rendering. This is not supported. ' +
'Move any mutations into event handlers or effects.\n' +
' in MutateDuringRead (at **)',
act(() => {
ReactNoop.renderLegacySyncRoot(
<React.StrictMode>
<MutateDuringRead />
</React.StrictMode>,
);
});
}).toThrow(
'A mutable source was mutated while the MutateDuringRead component ' +
'was rendering. This is not supported. Move any mutations into ' +
'event handlers or effects.',
);

expect(Scheduler).toHaveYielded(['MutateDuringRead:initial']);
Expand Down
4 changes: 2 additions & 2 deletions scripts/error-codes/codes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@
"345": "Root did not complete. This is a bug in React.",
"348": "ensureListeningTo(): received a container that was not an element node. This is likely a bug in React.",
"349": "Expected a work-in-progress root. This is a bug in React. Please file an issue.",
"350": "Cannot read from mutable source during the current render without tearing. This is a bug in React. Please file an issue.",
"350": "Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

"351": "Unsupported server component type: %s",
"352": "React Lazy Components are not yet supported on the server.",
"353": "A server block should never encode any other slots. This is a bug in React.",
Expand Down Expand Up @@ -373,5 +373,5 @@
"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.",
"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.",
"384": "Refreshing the cache is not supported in Server Components.",
"385": "Cannot read from mutable source during the current render without tearing. This may be a bug in React. Please file an issue."
"385": "A mutable source was mutated while the %s component was rendering. This is not supported. Move any mutations into event handlers or effects."
}