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.

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

Fix: Detect infinite update loops caused by render phase updates #26625

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Apr 14, 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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 14, 2023
@react-sizebot
Copy link

react-sizebot commented Apr 14, 2023

Comparing: 80d9a40...ae39371

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.18% 164.02 kB 164.32 kB +0.21% 51.64 kB 51.75 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.17% 171.44 kB 171.73 kB +0.20% 53.86 kB 53.97 kB
facebook-www/ReactDOM-prod.classic.js +0.33% 566.36 kB 568.20 kB +0.28% 99.86 kB 100.14 kB
facebook-www/ReactDOM-prod.modern.js +0.33% 550.16 kB 552.00 kB +0.29% 97.01 kB 97.29 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-dev.js +0.69% 792.53 kB 798.03 kB +0.88% 172.18 kB 173.70 kB
facebook-www/ReactTestRenderer-dev.modern.js +0.69% 798.83 kB 804.33 kB +0.87% 173.03 kB 174.53 kB
facebook-www/ReactTestRenderer-dev.classic.js +0.69% 798.83 kB 804.33 kB +0.87% 173.03 kB 174.53 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.development.js +0.68% 780.59 kB 785.89 kB +0.86% 170.87 kB 172.34 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js +0.68% 780.62 kB 785.91 kB +0.86% 170.90 kB 172.37 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.development.js +0.68% 780.99 kB 786.28 kB +0.86% 170.99 kB 172.46 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.development.js +0.67% 817.65 kB 823.11 kB +0.87% 172.65 kB 174.15 kB
oss-stable/react-test-renderer/umd/react-test-renderer.development.js +0.67% 817.67 kB 823.14 kB +0.87% 172.68 kB 174.18 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.development.js +0.67% 818.06 kB 823.53 kB +0.87% 172.78 kB 174.28 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js +0.66% 296.37 kB 298.34 kB +0.67% 52.68 kB 53.04 kB
oss-stable-semver/react-art/cjs/react-art.development.js +0.66% 799.21 kB 804.51 kB +0.84% 174.57 kB 176.03 kB
oss-stable/react-art/cjs/react-art.development.js +0.66% 799.24 kB 804.54 kB +0.84% 174.59 kB 176.06 kB
oss-experimental/react-art/cjs/react-art.development.js +0.65% 821.22 kB 826.52 kB +0.82% 178.96 kB 180.42 kB
react-native/implementations/ReactFabric-dev.js +0.64% 857.30 kB 862.80 kB +0.83% 186.63 kB 188.18 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js +0.63% 312.22 kB 314.19 kB +0.69% 55.10 kB 55.48 kB
react-native/implementations/ReactNativeRenderer-dev.js +0.63% 872.96 kB 878.46 kB +0.79% 191.01 kB 192.51 kB
react-native/implementations/ReactFabric-dev.fb.js +0.62% 883.88 kB 889.37 kB +0.79% 191.28 kB 192.80 kB
react-native/implementations/ReactFabric-prod.js +0.62% 318.09 kB 320.06 kB +0.59% 56.09 kB 56.42 kB
facebook-www/ReactART-dev.modern.js +0.61% 898.94 kB 904.44 kB +0.77% 191.54 kB 193.01 kB
react-native/implementations/ReactNativeRenderer-dev.fb.js +0.61% 899.54 kB 905.04 kB +0.78% 195.72 kB 197.25 kB
react-native/implementations/ReactFabric-prod.fb.js +0.61% 324.82 kB 326.79 kB +0.56% 57.56 kB 57.88 kB
facebook-www/ReactART-dev.classic.js +0.60% 910.14 kB 915.64 kB +0.78% 193.81 kB 195.32 kB
react-native/implementations/ReactNativeRenderer-prod.js +0.60% 327.04 kB 329.00 kB +0.58% 57.81 kB 58.15 kB
oss-stable-semver/react-art/umd/react-art.development.js +0.60% 913.86 kB 919.33 kB +0.78% 193.60 kB 195.12 kB
oss-stable/react-art/umd/react-art.development.js +0.60% 913.89 kB 919.35 kB +0.78% 193.63 kB 195.15 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.development.js +0.59% 895.76 kB 901.05 kB +0.76% 192.12 kB 193.59 kB
oss-stable/react-reconciler/cjs/react-reconciler.development.js +0.59% 895.78 kB 901.08 kB +0.76% 192.15 kB 193.62 kB
react-native/implementations/ReactNativeRenderer-prod.fb.js +0.59% 333.65 kB 335.62 kB +0.55% 59.26 kB 59.59 kB
oss-experimental/react-art/umd/react-art.development.js +0.58% 936.96 kB 942.42 kB +0.74% 197.98 kB 199.45 kB
react-native/implementations/ReactFabric-profiling.js +0.58% 337.65 kB 339.61 kB +0.59% 59.31 kB 59.66 kB
oss-experimental/react-reconciler/cjs/react-reconciler.development.js +0.58% 919.26 kB 924.56 kB +0.75% 196.87 kB 198.34 kB
react-native/implementations/ReactNativeRenderer-profiling.js +0.57% 346.59 kB 348.56 kB +0.55% 61.07 kB 61.41 kB
react-native/implementations/ReactFabric-profiling.fb.js +0.56% 352.23 kB 354.20 kB +0.64% 61.77 kB 62.17 kB
react-native/implementations/ReactNativeRenderer-profiling.fb.js +0.55% 361.06 kB 363.03 kB +0.61% 63.52 kB 63.91 kB
facebook-www/ReactART-prod.modern.js +0.53% 334.50 kB 336.27 kB +0.52% 56.87 kB 57.17 kB
facebook-www/ReactART-prod.classic.js +0.51% 345.50 kB 347.27 kB +0.50% 58.79 kB 59.08 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.production.min.js +0.48% 102.74 kB 103.24 kB +0.75% 31.53 kB 31.77 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.production.min.js +0.48% 102.79 kB 103.29 kB +0.75% 31.56 kB 31.80 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.min.js +0.48% 102.91 kB 103.41 kB +0.75% 31.60 kB 31.83 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.production.min.js +0.48% 103.13 kB 103.62 kB +0.73% 31.95 kB 32.19 kB
oss-stable/react-test-renderer/umd/react-test-renderer.production.min.js +0.48% 103.18 kB 103.68 kB +0.73% 31.98 kB 32.21 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.production.min.js +0.48% 103.30 kB 103.80 kB +0.63% 32.04 kB 32.24 kB
oss-stable-semver/react-dom/cjs/react-dom.development.js +0.42% 1,259.84 kB 1,265.14 kB +0.52% 279.22 kB 280.68 kB
oss-stable/react-dom/cjs/react-dom.development.js +0.42% 1,259.87 kB 1,265.17 kB +0.52% 279.25 kB 280.71 kB
oss-stable-semver/react-dom/umd/react-dom.development.js +0.41% 1,320.83 kB 1,326.29 kB +0.52% 282.20 kB 283.67 kB
oss-stable/react-dom/umd/react-dom.development.js +0.41% 1,320.85 kB 1,326.32 kB +0.52% 282.24 kB 283.70 kB
oss-experimental/react-dom/cjs/react-dom.development.js +0.41% 1,304.11 kB 1,309.41 kB +0.51% 288.85 kB 290.33 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.development.js +0.40% 1,322.17 kB 1,327.47 kB +0.50% 293.20 kB 294.67 kB
oss-experimental/react-dom/umd/react-dom.development.js +0.40% 1,367.16 kB 1,372.63 kB +0.51% 291.76 kB 293.24 kB
facebook-www/ReactDOM-dev.modern.js +0.39% 1,400.34 kB 1,405.84 kB +0.51% 302.82 kB 304.36 kB
facebook-www/ReactDOMTesting-dev.modern.js +0.39% 1,418.69 kB 1,424.19 kB +0.50% 307.23 kB 308.77 kB
facebook-www/ReactDOM-dev.classic.js +0.39% 1,428.24 kB 1,433.73 kB +0.51% 308.35 kB 309.90 kB
facebook-www/ReactDOMTesting-dev.classic.js +0.38% 1,446.58 kB 1,452.08 kB +0.49% 312.82 kB 314.35 kB
facebook-www/ReactDOM-prod.modern.js +0.33% 550.16 kB 552.00 kB +0.29% 97.01 kB 97.29 kB
facebook-www/ReactDOM-prod.classic.js +0.33% 566.36 kB 568.20 kB +0.28% 99.86 kB 100.14 kB
facebook-www/ReactDOMTesting-prod.modern.js +0.33% 566.70 kB 568.54 kB +0.28% 101.11 kB 101.39 kB
facebook-www/ReactDOM-profiling.modern.js +0.32% 580.64 kB 582.49 kB +0.32% 101.48 kB 101.81 kB
facebook-www/ReactDOMTesting-prod.classic.js +0.32% 581.18 kB 583.02 kB +0.27% 103.56 kB 103.84 kB
oss-stable-semver/react-art/cjs/react-art.production.min.js +0.31% 94.11 kB 94.40 kB +0.43% 28.92 kB 29.05 kB
oss-stable/react-art/cjs/react-art.production.min.js +0.31% 94.16 kB 94.45 kB +0.43% 28.95 kB 29.07 kB
facebook-www/ReactDOM-profiling.classic.js +0.31% 596.92 kB 598.76 kB +0.32% 104.31 kB 104.65 kB
oss-experimental/react-art/cjs/react-art.production.min.js +0.30% 97.99 kB 98.28 kB +0.41% 30.06 kB 30.19 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.min.js +0.27% 110.34 kB 110.63 kB +0.43% 33.68 kB 33.82 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.min.js +0.27% 110.36 kB 110.66 kB +0.44% 33.70 kB 33.85 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js +0.26% 114.56 kB 114.85 kB +0.43% 34.96 kB 35.11 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.profiling.min.js +0.25% 119.35 kB 119.65 kB +0.32% 35.90 kB 36.02 kB
oss-stable/react-reconciler/cjs/react-reconciler.profiling.min.js +0.25% 119.38 kB 119.67 kB +0.32% 35.92 kB 36.04 kB
oss-experimental/react-reconciler/cjs/react-reconciler.profiling.min.js +0.24% 123.58 kB 123.87 kB +0.33% 37.17 kB 37.29 kB
oss-stable-semver/react-art/umd/react-art.production.min.js +0.22% 131.40 kB 131.69 kB +0.37% 41.08 kB 41.24 kB
oss-stable/react-art/umd/react-art.production.min.js +0.22% 131.45 kB 131.74 kB +0.38% 41.11 kB 41.26 kB
oss-experimental/react-art/umd/react-art.production.min.js +0.22% 135.25 kB 135.54 kB +0.30% 42.22 kB 42.35 kB

Generated by 🚫 dangerJS against ae39371

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 acdlite force-pushed the infinite-update-loop-guard branch from f57f15b to ae39371 Compare June 27, 2023 17:19
@acdlite acdlite merged commit 822386f into facebook:main Jun 27, 2023
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
kassens added a commit that referenced this pull request Jun 30, 2023
…tes (#26625)" (#27027)

This reverts commit 822386f.

This broke a number of tests when synced internally. We'll need to
investigate the breakages before relanding this.
github-actions bot pushed a commit that referenced this pull request Jun 30, 2023
…tes (#26625)" (#27027)

This reverts commit 822386f.

This broke a number of tests when synced internally. We'll need to
investigate the breakages before relanding this.

DiffTrain build for [7f362de](7f362de)
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.
github-actions bot pushed 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.

DiffTrain build for [d8c1fa6](d8c1fa6)
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
…tes (#26625)" (#27027)

This reverts commit 822386f.

This broke a number of tests when synced internally. We'll need to
investigate the breakages before relanding this.

DiffTrain build for commit 7f362de.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants