Fix: Detect infinite update loops caused by render phase updates #26625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 80d9a40...ae39371 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show |
rickhanlonii
approved these changes
Apr 14, 2023
acdlite
force-pushed
the
infinite-update-loop-guard
branch
from
def5042
to
f57f15b
Compare
We already have an infinite loop guard that detects unguarded updates, like if you repeatedly call `setState` in an effect — the nth+1 update will trigger an error. However, a limitation is that it depends on the error causing the component to be unmounted. This is usually the case but there are ways to break this expectation with undefined behavior, like if you call `setState` during the render phase on an different component (which is a bad pattern that is always accompanied by a warning). Aside from product bugs, if there were a bug in React, it might also cause something like this. Whether the bug happens because of app code or internal React code, we should do our best to avoid infinite loops because they are so hard to debug; even if it's a fatal bug and the UI can't recover, we at least need to free up the main thread so that it's more likely for the error to be reported. So this adds a second infinite loop guard around the synchronous work loop. This won't cover all possible loops, such as ones triggered during a concurrent render, but it should cover the most egregious ones.
If an infinite update loop is caused by a render phase update, the mechanism we typically use to break the loop doesn't work. Our mechanism assumes that by throwing inside `setState`, the error will caise the component to be unmounted, but that only works if the error happens in an effect or lifecycle method. During the render phase, what happens is that React will try to render the component one more time, synchronously, which we do as a way to recover from concurrent data races. But during this second attempt, the "Maximum update" error won't be thrown, because the counter was already reset. I considered a few different ways to fix this, like waiting to reset the counter until after the error has been surfaced. However, it's not obvious where this should happen. So instead the approach I landed on is to temporarily disable the error recovery mechanism. This is the same trick we use to prevent infinite ping loops when an uncached promise is passed to `use` during a sync render. This category of error is also covered by the more generic loop guard I added in the previous commit, but I also confirmed that this change alone fixes it.
This improves our infinite loop detection by explicitly tracking when an update happens during the render or commit phase. Previously, to detect recursive update, we would check if there was synchronous work remaining after the commit phase — because synchronous work is the highest priority, the only way there could be even more synchronous work is if it was spawned by that render. But this approach won't work to detect async update loops.
acdlite
force-pushed
the
infinite-update-loop-guard
branch
from
f57f15b
to
ae39371
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Jun 27, 2023
) This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React). I confirmed that each of these fixes separately are sufficient to fix the regression test I added. We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind. See commit messages for more details. DiffTrain build for [822386f](822386f)
kassens
added a commit
to kassens/react
that referenced
this pull request
Jun 30, 2023
…tes (facebook#26625)" This reverts commit 822386f.
timneutkens
pushed a commit
to vercel/next.js
that referenced
this pull request
Jul 4, 2023
This PR updates the vendored react dependencies using `pnpm sync-react` ### React upstream changes - facebook/react#27028 - facebook/react#27027 - facebook/react#27019 - facebook/react#26954 - facebook/react#26987 - facebook/react#26985 - facebook/react#26933 - facebook/react#26625 - facebook/react#27011 - facebook/react#27008 - facebook/react#26997 - facebook/react#26989 - facebook/react#26955 - facebook/react#26963 - facebook/react#26983 - facebook/react#26914 - facebook/react#26951 - facebook/react#26977 - facebook/react#26958 - facebook/react#26940 - facebook/react#26939 - facebook/react#26887 - facebook/react#26947 - facebook/react#26945 - facebook/react#26942 - facebook/react#26938 - facebook/react#26844 - facebook/react#25510 - facebook/react#26932 - facebook/react#26896 - facebook/react#26913 - facebook/react#26888 - facebook/react#26827 - facebook/react#26889 - facebook/react#26877 - facebook/react#26873 - facebook/react#26880 - facebook/react#26842 - facebook/react#26858 - facebook/react#26754 - facebook/react#26753 - facebook/react#26881 ### Related Closes #49409 (by facebook/react#26977) fix NEXT-1189 Co-authored-by: Shu Ding <[email protected]>
timneutkens
pushed a commit
to vercel/next.js
that referenced
this pull request
Jul 6, 2023
Fixes #49409 ### React upstream changes - facebook/react#27045 - facebook/react#27051 - facebook/react#27032 - facebook/react#27031 - facebook/react#27029 - facebook/react#27028 - facebook/react#27027 - facebook/react#27019 - facebook/react#26954 - facebook/react#26987 - facebook/react#26985 - facebook/react#26933 - facebook/react#26625 - facebook/react#27011 - facebook/react#27008 - facebook/react#26997 - facebook/react#26989 - facebook/react#26955 - facebook/react#26963 - facebook/react#26983 - facebook/react#26914 - facebook/react#26951 - facebook/react#26977 - facebook/react#26958 - facebook/react#26940 - facebook/react#26939 - facebook/react#26887 - facebook/react#26947 - facebook/react#26945 - facebook/react#26942 - facebook/react#26938 - facebook/react#26844 - facebook/react#25510 - facebook/react#26932 - facebook/react#26896 - facebook/react#26913 - facebook/react#26888 - facebook/react#26827 - facebook/react#26889 - facebook/react#26877 - facebook/react#26873 - facebook/react#26880 - facebook/react#26842 - facebook/react#26858 - facebook/react#26754 - facebook/react#26753 - facebook/react#26881 --------- Co-authored-by: Jiachi Liu <[email protected]>
shuding
added a commit
to vercel/next.js
that referenced
this pull request
Jul 8, 2023
This PR updates the vendored react dependencies using `pnpm sync-react` ### React upstream changes - facebook/react#27028 - facebook/react#27027 - facebook/react#27019 - facebook/react#26954 - facebook/react#26987 - facebook/react#26985 - facebook/react#26933 - facebook/react#26625 - facebook/react#27011 - facebook/react#27008 - facebook/react#26997 - facebook/react#26989 - facebook/react#26955 - facebook/react#26963 - facebook/react#26983 - facebook/react#26914 - facebook/react#26951 - facebook/react#26977 - facebook/react#26958 - facebook/react#26940 - facebook/react#26939 - facebook/react#26887 - facebook/react#26947 - facebook/react#26945 - facebook/react#26942 - facebook/react#26938 - facebook/react#26844 - facebook/react#25510 - facebook/react#26932 - facebook/react#26896 - facebook/react#26913 - facebook/react#26888 - facebook/react#26827 - facebook/react#26889 - facebook/react#26877 - facebook/react#26873 - facebook/react#26880 - facebook/react#26842 - facebook/react#26858 - facebook/react#26754 - facebook/react#26753 - facebook/react#26881 ### Related Closes #49409 (by facebook/react#26977) fix NEXT-1189 Co-authored-by: Shu Ding <[email protected]>
tyao1
pushed a commit
that referenced
this pull request
Oct 10, 2023
) This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React). I confirmed that each of these fixes separately are sufficient to fix the regression test I added. We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind. See commit messages for more details.
tyao1
pushed a commit
that referenced
this pull request
Oct 12, 2023
) This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React). I confirmed that each of these fixes separately are sufficient to fix the regression test I added. We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind. See commit messages for more details.
kassens
pushed a commit
to kassens/react
that referenced
this pull request
Feb 7, 2024
…ebook#26625) This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React). I confirmed that each of these fixes separately are sufficient to fix the regression test I added. We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind. See commit messages for more details.
kassens
pushed a commit
to kassens/react
that referenced
this pull request
Feb 7, 2024
…ebook#26625) This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React). I confirmed that each of these fixes separately are sufficient to fix the regression test I added. We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind. See commit messages for more details.
kassens
pushed a commit
to kassens/react
that referenced
this pull request
Feb 7, 2024
…ebook#26625) This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React). I confirmed that each of these fixes separately are sufficient to fix the regression test I added. We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind. See commit messages for more details.
kassens
added a commit
to kassens/react
that referenced
this pull request
Feb 8, 2024
This is a partial redo of facebook#26625. Since that was unlanded due to some detected breakages. This now includes a feature flag to be careful in rolling this out.
kassens
added a commit
to kassens/react
that referenced
this pull request
Feb 8, 2024
This is a partial redo of facebook#26625. Since that was unlanded due to some detected breakages. This now includes a feature flag to be careful in rolling this out.
kassens
added a commit
to kassens/react
that referenced
this pull request
Feb 8, 2024
This is a partial redo of facebook#26625. Since that was unlanded due to some detected breakages. This now includes a feature flag to be careful in rolling this out.
kassens
added a commit
to kassens/react
that referenced
this pull request
Feb 9, 2024
This is a partial redo of facebook#26625. Since that was unlanded due to some detected breakages. This now includes a feature flag to be careful in rolling this out.
kassens
added a commit
that referenced
this pull request
Feb 9, 2024
This is a partial redo of #26625. Since that was unlanded due to some detected breakages. This now includes a feature flag to be careful in rolling this out.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…ebook#26625) This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React). I confirmed that each of these fixes separately are sufficient to fix the regression test I added. We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind. See commit messages for more details.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…tes (facebook#26625)" (facebook#27027) This reverts commit 822386f. This broke a number of tests when synced internally. We'll need to investigate the breakages before relanding this.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
This is a partial redo of facebook#26625. Since that was unlanded due to some detected breakages. This now includes a feature flag to be careful in rolling this out.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
) This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React). I confirmed that each of these fixes separately are sufficient to fix the regression test I added. We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind. See commit messages for more details. DiffTrain build for commit 822386f.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React).
I confirmed that each of these fixes separately are sufficient to fix the regression test I added.
We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind.
See commit messages for more details.