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

Jump to bottom

Warn about async infinite useEffect loop #15180

Merged
merged 2 commits into from Mar 22, 2019
Merged

Warn about async infinite useEffect loop #15180

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
a68fbe8
Warn about async infinite useEffect loop
gaearon Mar 21, 2019
9cdb8c4
Make tests sync
gaearon Mar 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
94 changes: 94 additions & 0 deletions packages/react-dom/src/__tests__/ReactUpdates-test.js
View file
Open in desktop
Expand Up @@ -12,13 +12,15 @@
let React;
let ReactDOM;
let ReactTestUtils;
let Scheduler;

describe('ReactUpdates', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactDOM = require('react-dom');
ReactTestUtils = require('react-dom/test-utils');
Scheduler = require('scheduler');
});

it('should batch state when updating state twice', () => {
Expand Down Expand Up @@ -1524,4 +1526,96 @@ describe('ReactUpdates', () => {
});
});
});

if (__DEV__) {
it('warns about a deferred infinite update loop with useEffect', () => {
function NonTerminating() {
const [step, setStep] = React.useState(0);
React.useEffect(() => {
setStep(x => x + 1);
Scheduler.yieldValue(step);
});
return step;
}

function App() {
return <NonTerminating />;
}

let error = null;
let stack = null;
let originalConsoleError = console.error;
console.error = (e, s) => {
error = e;
stack = s;
};
try {
const container = document.createElement('div');
ReactDOM.render(<App />, container);
while (error === null) {
Scheduler.unstable_flushNumberOfYields(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the first yieldValue call happens in a passive effect, this actually will flush the effects synchronously

}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is my shoddy attempt to faithfully emulate deferred useEffect case. (I don't want to flush it all synchronously.)

idontknowwhatimdoing.jpg

Copy link
Collaborator

Choose a reason for hiding this comment

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

Try this

ReactDOM.render(<App />, container, () => Scheduler.yieldValue('Did commit'));
expect(Scheduler).toFlushAndYieldThrough(['Did commit']);
// Synchronous effect has run, but not passive effects.
// Now flush the effects:
Scheduler.flushAll();

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I forgot this is sync mode. Might be easier to use a boolean since the effect runs an arbitrary number of times before erroring.

ReactDOM.render(<App />, container);
expect(container.textContent).toEqual(whatever);
// Synchronous effect has run, but not passive effects.
expect(didFlushPassiveEffect).toBe(false);
// Now flush the effects:
Scheduler.flushAll();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to simulate a real deferred loop between them to make sure the warning still fires. Not sure if it matters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To do that you need an assertion in between the synchronous commit phase and the effect phase. As is, your test would pass even if the passive effects were totally sync.

expect(error).toContain('Warning: Maximum update depth exceeded.');
expect(stack).toContain('in NonTerminating');
} finally {
console.error = originalConsoleError;
}
});

it('can have nested updates if they do not cross the limit', () => {
let _setStep;
const LIMIT = 50;

function Terminating() {
const [step, setStep] = React.useState(0);
_setStep = setStep;
React.useEffect(() => {
if (step < LIMIT) {
setStep(x => x + 1);
Scheduler.yieldValue(step);
}
});
return step;
}

const container = document.createElement('div');
ReactDOM.render(<Terminating />, container);

// Verify we can flush them asynchronously without warning
for (let i = 0; i < LIMIT * 2; i++) {
Scheduler.unstable_flushNumberOfYields(1);
}
expect(container.textContent).toBe('50');

// Verify restarting from 0 doesn't cross the limit
expect(() => {
_setStep(0);
}).toWarnDev(
'An update to Terminating inside a test was not wrapped in act',
);
expect(container.textContent).toBe('0');
for (let i = 0; i < LIMIT * 2; i++) {
Scheduler.unstable_flushNumberOfYields(1);
}
expect(container.textContent).toBe('50');
});

it('can have many updates inside useEffect without triggering a warning', () => {
function Terminating() {
const [step, setStep] = React.useState(0);
React.useEffect(() => {
for (let i = 0; i < 1000; i++) {
setStep(x => x + 1);
}
Scheduler.yieldValue('Done');
}, []);
return step;
}

const container = document.createElement('div');
ReactDOM.render(<Terminating />, container);
expect(Scheduler).toFlushAndYield(['Done']);
expect(container.textContent).toBe('1000');
});
}
});
41 changes: 41 additions & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.old.js
View file
Open in desktop
Expand Up @@ -67,6 +67,7 @@ import {
} from 'shared/ReactFeatureFlags';
import getComponentName from 'shared/getComponentName';
import invariant from 'shared/invariant';
import warning from 'shared/warning';
import warningWithoutStack from 'shared/warningWithoutStack';

import ReactFiberInstrumentation from './ReactFiberInstrumentation';
Expand Down Expand Up @@ -547,7 +548,9 @@ function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {
let didError = false;
let error;
if (__DEV__) {
isInPassiveEffectDEV = true;
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
isInPassiveEffectDEV = false;
if (hasCaughtError()) {
didError = true;
error = clearCaughtError();
Expand Down Expand Up @@ -581,6 +584,14 @@ function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {
if (!isBatchingUpdates && !isRendering) {
performSyncWork();
}

if (__DEV__) {
if (rootWithPendingPassiveEffects === root) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only keep incrementing if flushing root's deferred effects schedules the same root again.

nestedPassiveEffectCountDEV++;
} else {
nestedPassiveEffectCountDEV = 0;
}
}
}

function isAlreadyFailedLegacyErrorBoundary(instance: mixed): boolean {
Expand Down Expand Up @@ -1897,6 +1908,21 @@ function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) {
'the number of nested updates to prevent infinite loops.',
);
}
if (__DEV__) {
if (
isInPassiveEffectDEV &&
nestedPassiveEffectCountDEV > NESTED_PASSIVE_UPDATE_LIMIT
) {
nestedPassiveEffectCountDEV = 0;
warning(
false,
'Maximum update depth exceeded. This can happen when a ' +
'component calls setState inside useEffect, but ' +
"useEffect either doesn't have a dependency array, or " +
'one of the dependencies changes on every render.',
);
}
}
}

function deferredUpdates<A>(fn: () => A): A {
Expand Down Expand Up @@ -1962,6 +1988,15 @@ const NESTED_UPDATE_LIMIT = 50;
let nestedUpdateCount: number = 0;
let lastCommittedRootDuringThisBatch: FiberRoot | null = null;

// Similar, but for useEffect infinite loops. These are DEV-only.
const NESTED_PASSIVE_UPDATE_LIMIT = 50;
let nestedPassiveEffectCountDEV;
let isInPassiveEffectDEV;
if (__DEV__) {
nestedPassiveEffectCountDEV = 0;
isInPassiveEffectDEV = false;
}

function recomputeCurrentRendererTime() {
const currentTimeMs = now() - originalStartTimeMs;
currentRendererTime = msToExpirationTime(currentTimeMs);
Expand Down Expand Up @@ -2326,6 +2361,12 @@ function finishRendering() {
nestedUpdateCount = 0;
lastCommittedRootDuringThisBatch = null;

if (__DEV__) {
if (rootWithPendingPassiveEffects === null) {
nestedPassiveEffectCountDEV = 0;
}
}

if (completedBatches !== null) {
const batches = completedBatches;
completedBatches = null;
Expand Down