Skip to content

Limit the number of nested synchronous updates #10178

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

Merged
acdlite merged 2 commits into from
Jul 13, 2017
Merged

Limit the number of nested synchronous updates #10178

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
d451024
Limit the number of nested synchronous updates
acdlite Jul 13, 2017
cd4dc1e
Move check to findNextUnitOfWork
acdlite Jul 13, 2017
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
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,7 @@ src/renderers/__tests__/ReactUpdates-test.js
* mounts and unmounts are sync even in a batch
* does not re-render if state update is null
* synchronously renders hidden subtrees
* does not fall into an infinite update loop

src/renderers/__tests__/multiple-copies-of-react-test.js
* throws the "Refs must have owner" warning
Expand Down
20 changes: 20 additions & 0 deletions src/renderers/__tests__/ReactUpdates-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1138,4 +1138,24 @@ describe('ReactUpdates', () => {
ReactDOM.render(<Foo />, container);
expect(ops).toEqual(['Foo', 'Bar', 'Baz']);
});

it('does not fall into an infinite update loop', () => {
class NonTerminating extends React.Component {
state = {step: 0};
componentDidMount() {
this.setState({step: 1});
}
componentWillUpdate() {
this.setState({step: 2});
}
render() {
return <div>Hello {this.props.name}{this.state.step}</div>;
}
}

const container = document.createElement('div');
expect(() => {
ReactDOM.render(<NonTerminating />, container);
}).toThrow('Maximum');
});
});
22 changes: 22 additions & 0 deletions src/renderers/shared/fiber/ReactFiberScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
let isCommitting: boolean = false;
let isUnmounting: boolean = false;

// Use these to prevent an infinite loop of nested updates
let didCommit = false;
let nestedSyncUpdates = 0;
Copy link
Copy Markdown
Contributor

@sebmarkbage sebmarkbage Jul 13, 2017

Choose a reason for hiding this comment

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

This is never reset. That makes me suspicious.

Copy link
Copy Markdown
Collaborator Author

@acdlite acdlite Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Oops. Fixed to reset at the end of performWork.

Before fixing, I changed the limit to 3 and ran the tests again, got lots of failures as expected. So I guess in our whole test suite we have fewer than 1000 nested updates.

EDIT: The scheduler state is reset for each individual Jest file, and the test that triggers the infinite loop is at the end of its module. So it wasn't triggering a failure because no Jest file has greater than 1000 nested updates.

let NESTED_SYNC_UPDATE_LIMIT = 1000;

function resetContextStack() {
// Reset the stack
reset();
Expand Down Expand Up @@ -285,6 +290,20 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
nextPriorityLevel = highestPriorityLevel;
priorityContext = nextPriorityLevel;

if (
didCommit &&
(nextPriorityLevel === TaskPriority ||
nextPriorityLevel === SynchronousPriority)
) {
invariant(
nestedSyncUpdates++ <= NESTED_SYNC_UPDATE_LIMIT,
'Maximum update depth exceeded. This can happen when a ' +
'component repeatedly calls setState inside componentWillUpdate or ' +
'componentDidUpdate. React limits the number of nested updates to ' +
'prevent infinite loops.',
);
}

// Before we start any new work, let's make sure that we have a fresh
// stack to work from.
// TODO: This call is buried a bit too deep. It would be nice to have
Expand Down Expand Up @@ -415,6 +434,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// local to this function is because errors that occur during cWU are
// captured elsewhere, to prevent the unmount from being interrupted.
isCommitting = true;
didCommit = true;
if (__DEV__) {
startCommitTimer();
}
Expand Down Expand Up @@ -991,6 +1011,8 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
firstUncaughtError = null;
capturedErrors = null;
failedBoundaries = null;
didCommit = false;
nestedSyncUpdates = 0;
if (__DEV__) {
stopWorkLoopTimer();
}
Expand Down