-
Notifications
You must be signed in to change notification settings - Fork 50.9k
[enableInfiniteRenderLoopDetection] Warn about potential infinite loop, instead of interrupting #35999
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.
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
[enableInfiniteRenderLoopDetection] Warn about potential infinite loop, instead of interrupting #35999
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1792,8 +1792,8 @@ describe('ReactUpdates', () => { | |
| expect(subscribers.length).toBe(limit); | ||
| }); | ||
|
|
||
| it("does not infinite loop if there's a synchronous render phase update on another component", async () => { | ||
| if (gate(flags => !flags.enableInfiniteRenderLoopDetection)) { | ||
| it("warns about potential infinite loop if there's a synchronous render phase update on another component", async () => { | ||
| if (!__DEV__ || gate(flags => !flags.enableInfiniteRenderLoopDetection)) { | ||
| return; | ||
| } | ||
| let setState; | ||
|
|
@@ -1809,22 +1809,29 @@ describe('ReactUpdates', () => { | |
| return null; | ||
| } | ||
|
|
||
| const container = document.createElement('div'); | ||
| const root = ReactDOMClient.createRoot(container); | ||
|
|
||
| await expect(async () => { | ||
| await act(() => ReactDOM.flushSync(() => root.render(<App />))); | ||
| }).rejects.toThrow('Maximum update depth exceeded'); | ||
| assertConsoleErrorDev([ | ||
| 'Cannot update a component (`App`) while rendering a different component (`Child`). ' + | ||
| 'To locate the bad setState() call inside `Child`, ' + | ||
| 'follow the stack trace as described in https://react.dev/link/setstate-in-render\n' + | ||
| ' in App (at **)', | ||
| ]); | ||
| const originalConsoleError = console.error; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because I am patching a console not to just intercept the message, but to wait for the specific console.error call, pass the test and avoid being stuck in an infinite loop. I could use |
||
| console.error = e => { | ||
| if ( | ||
| typeof e === 'string' && | ||
| e.startsWith( | ||
| 'Maximum update depth exceeded. This could be an infinite loop.', | ||
| ) | ||
| ) { | ||
| Scheduler.log('stop'); | ||
| } | ||
| }; | ||
| try { | ||
| const container = document.createElement('div'); | ||
| const root = ReactDOMClient.createRoot(container); | ||
| root.render(<App />); | ||
| await waitFor(['stop']); | ||
| } finally { | ||
| console.error = originalConsoleError; | ||
| } | ||
| }); | ||
|
|
||
| it("does not infinite loop if there's an async render phase update on another component", async () => { | ||
| if (gate(flags => !flags.enableInfiniteRenderLoopDetection)) { | ||
| it("warns about potential infinite loop if there's an async render phase update on another component", async () => { | ||
| if (!__DEV__ || gate(flags => !flags.enableInfiniteRenderLoopDetection)) { | ||
| return; | ||
| } | ||
| let setState; | ||
|
|
@@ -1840,21 +1847,25 @@ describe('ReactUpdates', () => { | |
| return null; | ||
| } | ||
|
|
||
| const container = document.createElement('div'); | ||
| const root = ReactDOMClient.createRoot(container); | ||
|
|
||
| await expect(async () => { | ||
| await act(() => { | ||
| React.startTransition(() => root.render(<App />)); | ||
| }); | ||
| }).rejects.toThrow('Maximum update depth exceeded'); | ||
|
|
||
| assertConsoleErrorDev([ | ||
| 'Cannot update a component (`App`) while rendering a different component (`Child`). ' + | ||
| 'To locate the bad setState() call inside `Child`, ' + | ||
| 'follow the stack trace as described in https://react.dev/link/setstate-in-render\n' + | ||
| ' in App (at **)', | ||
| ]); | ||
| const originalConsoleError = console.error; | ||
| console.error = e => { | ||
| if ( | ||
| typeof e === 'string' && | ||
| e.startsWith( | ||
| 'Maximum update depth exceeded. This could be an infinite loop.', | ||
| ) | ||
| ) { | ||
| Scheduler.log('stop'); | ||
| } | ||
| }; | ||
| try { | ||
| const container = document.createElement('div'); | ||
| const root = ReactDOMClient.createRoot(container); | ||
| React.startTransition(() => root.render(<App />)); | ||
| await waitFor(['stop']); | ||
| } finally { | ||
| console.error = originalConsoleError; | ||
| } | ||
| }); | ||
|
|
||
| // TODO: Replace this branch with @gate pragmas | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -751,6 +751,11 @@ let rootWithNestedUpdates: FiberRoot | null = null; | |
| let isFlushingPassiveEffects = false; | ||
| let didScheduleUpdateDuringPassiveEffects = false; | ||
|
|
||
| const NO_NESTED_UPDATE = 0; | ||
| const NESTED_UPDATE_SYNC_LANE = 1; | ||
| const NESTED_UPDATE_PHASE_SPAWN = 2; | ||
| let nestedUpdateKind: 0 | 1 | 2 = NO_NESTED_UPDATE; | ||
|
|
||
| const NESTED_PASSIVE_UPDATE_LIMIT = 50; | ||
| let nestedPassiveUpdateCount: number = 0; | ||
| let rootWithPassiveNestedUpdates: FiberRoot | null = null; | ||
|
|
@@ -4313,15 +4318,30 @@ function flushSpawnedWork(): void { | |
| // hydration lanes in this check, because render triggered by selective | ||
| // hydration is conceptually not an update. | ||
| if ( | ||
| // Was the finished render the result of an update (not hydration)? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did we remove the feature flag gating?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is pre-existing check that was already passing in some scenarios, when the update could be tracked through lanes. Like obvious cases where we schedule the infinite loop inside a single component, causing it to loop on itself. This is outside of the |
||
| includesSomeLane(lanes, UpdateLanes) && | ||
| // Did it schedule a sync update? | ||
| includesSomeLane(remainingLanes, SyncUpdateLanes) | ||
| ) { | ||
| if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { | ||
| markNestedUpdateScheduled(); | ||
| } | ||
|
|
||
| // Count the number of times the root synchronously re-renders without | ||
| // finishing. If there are too many, it indicates an infinite update loop. | ||
| if (root === rootWithNestedUpdates) { | ||
| nestedUpdateCount++; | ||
| } else { | ||
| nestedUpdateCount = 0; | ||
| rootWithNestedUpdates = root; | ||
| } | ||
| nestedUpdateKind = NESTED_UPDATE_SYNC_LANE; | ||
| } else if ( | ||
| // Check if there was a recursive update spawned by this render, in either | ||
| // the render phase or the commit phase. We track these explicitly because | ||
| // we can't infer from the remaining lanes alone. | ||
| (enableInfiniteRenderLoopDetection && | ||
| (didIncludeRenderPhaseUpdate || didIncludeCommitPhaseUpdate)) || | ||
| // Was the finished render the result of an update (not hydration)? | ||
| (includesSomeLane(lanes, UpdateLanes) && | ||
| // Did it schedule a sync update? | ||
| includesSomeLane(remainingLanes, SyncUpdateLanes)) | ||
| enableInfiniteRenderLoopDetection && | ||
| (didIncludeRenderPhaseUpdate || didIncludeCommitPhaseUpdate) | ||
| ) { | ||
| if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { | ||
| markNestedUpdateScheduled(); | ||
|
|
@@ -4335,8 +4355,11 @@ function flushSpawnedWork(): void { | |
| nestedUpdateCount = 0; | ||
| rootWithNestedUpdates = root; | ||
| } | ||
| nestedUpdateKind = NESTED_UPDATE_PHASE_SPAWN; | ||
| } else { | ||
| nestedUpdateCount = 0; | ||
| rootWithNestedUpdates = null; | ||
| nestedUpdateKind = NO_NESTED_UPDATE; | ||
| } | ||
|
|
||
| if (enableProfilerTimer && enableComponentPerformanceTrack) { | ||
|
|
@@ -5152,25 +5175,47 @@ export function throwIfInfiniteUpdateLoopDetected() { | |
| rootWithNestedUpdates = null; | ||
| rootWithPassiveNestedUpdates = null; | ||
|
|
||
| const updateKind = nestedUpdateKind; | ||
| nestedUpdateKind = NO_NESTED_UPDATE; | ||
|
|
||
| if (enableInfiniteRenderLoopDetection) { | ||
| if (executionContext & RenderContext && workInProgressRoot !== null) { | ||
| // We're in the render phase. Disable the concurrent error recovery | ||
| // mechanism to ensure that the error we're about to throw gets handled. | ||
| // We need it to trigger the nearest error boundary so that the infinite | ||
| // update loop is broken. | ||
| workInProgressRoot.errorRecoveryDisabledLanes = mergeLanes( | ||
| workInProgressRoot.errorRecoveryDisabledLanes, | ||
| workInProgressRootRenderLanes, | ||
| ); | ||
| if (updateKind === NESTED_UPDATE_SYNC_LANE) { | ||
| if (executionContext & RenderContext && workInProgressRoot !== null) { | ||
| // This loop was identified only because of the instrumentation gated with enableInfiniteRenderLoopDetection, warn instead of throwing. | ||
| if (__DEV__) { | ||
| console.error( | ||
| 'Maximum update depth exceeded. This could be an infinite loop. This can happen when a component ' + | ||
| 'repeatedly calls setState during render phase or inside useLayoutEffect, ' + | ||
| 'causing infinite render loop. React limits the number of nested updates to ' + | ||
| 'prevent infinite loops.', | ||
| ); | ||
| } | ||
| } else { | ||
| throw new Error( | ||
| '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.', | ||
| ); | ||
| } | ||
| } else if (updateKind === NESTED_UPDATE_PHASE_SPAWN) { | ||
| if (__DEV__) { | ||
| console.error( | ||
| 'Maximum update depth exceeded. This could be an infinite loop. This can happen when a component ' + | ||
| 'repeatedly calls setState during render phase or inside useLayoutEffect, ' + | ||
| 'causing infinite render loop. React limits the number of nested updates to ' + | ||
| 'prevent infinite loops.', | ||
| ); | ||
| } | ||
| } | ||
| } else { | ||
| throw new Error( | ||
| '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.', | ||
| ); | ||
| } | ||
|
|
||
| throw new Error( | ||
| '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.', | ||
| ); | ||
| } | ||
|
|
||
| if (__DEV__) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not
@gate?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
@gate, it also checks that the test fails on assertions when gating conditions are not met, but it will just go infinite loop, and it will considered as failed run, not failed assertion. There is a TODO to convert some other tests in this file:react/packages/react-dom/src/__tests__/ReactUpdates-test.js
Line 1871 in 6e4f4e4
I think there is a way to convert to gate prama these tests, but it would require more attention