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

Detect crashes caused by Async Client Components #27019

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jun 27, 2023

Suspending with an uncached promise is not yet supported. We only support suspending on promises that are cached between render attempts. (We do plan to partially support this in the future, at least in certain constrained cases, like during a route transition.)

This includes the case where a component returns an uncached promise, which is effectively what happens if a Client Component is authored using async/await syntax.

This is an easy mistake to make in a Server Components app, because async/await is available in Server Components.

In the current behavior, this can sometimes cause the app to crash with an infinite loop, because React will repeatedly keep trying to render the component, which will result in a fresh promise, which will result in a new render attempt, and so on. We have some strategies we can use to prevent this — during a concurrent render, we can suspend the work loop until the promise resolves. If it's not a concurrent render, we can show a Suspense fallback and try again at concurrent priority.

There's one case where neither of these strategies work, though: during a sync render when there's no parent Suspense boundary. (We refer to this as the "shell" of the app because it exists outside of any loading UI.)

Since we don't have any great options for this scenario, we should at least error gracefully instead of crashing the app.

So this commit adds a detection mechanism for render loops caused by async client components. The way it works is, if an app suspends repeatedly in the shell during a synchronous render, without committing anything in between, we will count the number of attempts and eventually trigger an error once the count exceeds a threshold.

In the future, we will consider ways to make this case a warning instead of a hard error.

See #26801 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 Jun 27, 2023
@react-sizebot
Copy link

react-sizebot commented Jun 27, 2023

Comparing: a1c62b8...439ca59

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.12% 164.32 kB 164.51 kB +0.21% 51.75 kB 51.86 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.11% 171.73 kB 171.93 kB +0.17% 53.97 kB 54.06 kB
facebook-www/ReactDOM-prod.classic.js +0.07% 568.20 kB 568.63 kB +0.11% 100.14 kB 100.25 kB
facebook-www/ReactDOM-prod.modern.js +0.08% 552.00 kB 552.43 kB +0.12% 97.29 kB 97.41 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.production.min.js +0.38% 103.24 kB 103.63 kB +0.54% 31.77 kB 31.94 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.production.min.js +0.38% 103.29 kB 103.68 kB +0.55% 31.79 kB 31.97 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.production.min.js +0.38% 103.41 kB 103.80 kB +0.55% 31.83 kB 32.01 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.production.min.js +0.37% 103.80 kB 104.19 kB +0.51% 32.24 kB 32.41 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.production.min.js +0.37% 103.62 kB 104.01 kB +0.49% 32.19 kB 32.34 kB
oss-stable/react-test-renderer/umd/react-test-renderer.production.min.js +0.37% 103.68 kB 104.06 kB +0.50% 32.21 kB 32.37 kB
oss-stable-semver/react-test-renderer/cjs/react-test-renderer.development.js +0.26% 785.89 kB 787.95 kB +0.37% 172.34 kB 172.98 kB
oss-stable/react-test-renderer/cjs/react-test-renderer.development.js +0.26% 785.91 kB 787.98 kB +0.37% 172.37 kB 173.01 kB
oss-experimental/react-test-renderer/cjs/react-test-renderer.development.js +0.26% 786.28 kB 788.34 kB +0.37% 172.46 kB 173.09 kB
oss-stable-semver/react-test-renderer/umd/react-test-renderer.development.js +0.26% 823.11 kB 825.25 kB +0.37% 174.15 kB 174.80 kB
oss-stable/react-test-renderer/umd/react-test-renderer.development.js +0.26% 823.14 kB 825.27 kB +0.38% 174.17 kB 174.83 kB
oss-experimental/react-test-renderer/umd/react-test-renderer.development.js +0.26% 823.53 kB 825.66 kB +0.37% 174.27 kB 174.93 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-dev.js +0.26% 798.03 kB 800.09 kB +0.38% 173.70 kB 174.36 kB
facebook-www/ReactTestRenderer-dev.modern.js +0.26% 804.33 kB 806.40 kB +0.37% 174.53 kB 175.18 kB
facebook-www/ReactTestRenderer-dev.classic.js +0.26% 804.33 kB 806.40 kB +0.37% 174.53 kB 175.18 kB
oss-stable-semver/react-art/cjs/react-art.development.js +0.26% 804.51 kB 806.57 kB +0.36% 176.03 kB 176.66 kB
oss-stable/react-art/cjs/react-art.development.js +0.26% 804.54 kB 806.60 kB +0.36% 176.06 kB 176.69 kB
oss-experimental/react-art/cjs/react-art.development.js +0.25% 826.52 kB 828.58 kB +0.36% 180.42 kB 181.06 kB
react-native/implementations/ReactFabric-dev.js +0.24% 862.80 kB 864.87 kB +0.34% 188.18 kB 188.82 kB
react-native/implementations/ReactNativeRenderer-dev.js +0.24% 878.46 kB 880.52 kB +0.34% 192.51 kB 193.16 kB
react-native/implementations/ReactFabric-dev.fb.js +0.23% 889.37 kB 891.44 kB +0.33% 192.80 kB 193.44 kB
oss-stable-semver/react-art/umd/react-art.development.js +0.23% 919.33 kB 921.46 kB +0.34% 195.12 kB 195.77 kB
oss-stable/react-art/umd/react-art.development.js +0.23% 919.35 kB 921.49 kB +0.34% 195.15 kB 195.80 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.development.js +0.23% 901.05 kB 903.12 kB +0.32% 193.59 kB 194.21 kB
oss-stable/react-reconciler/cjs/react-reconciler.development.js +0.23% 901.08 kB 903.14 kB +0.32% 193.62 kB 194.24 kB
facebook-www/ReactART-dev.modern.js +0.23% 904.44 kB 906.50 kB +0.33% 193.01 kB 193.64 kB
react-native/implementations/ReactNativeRenderer-dev.fb.js +0.23% 905.04 kB 907.10 kB +0.33% 197.25 kB 197.89 kB
oss-experimental/react-art/umd/react-art.development.js +0.23% 942.42 kB 944.56 kB +0.33% 199.45 kB 200.10 kB
facebook-www/ReactART-dev.classic.js +0.23% 915.64 kB 917.71 kB +0.32% 195.32 kB 195.95 kB
oss-experimental/react-reconciler/cjs/react-reconciler.development.js +0.22% 924.56 kB 926.62 kB +0.32% 198.35 kB 198.98 kB
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-prod.js +0.21% 298.34 kB 298.96 kB +0.38% 53.04 kB 53.24 kB
oss-stable-semver/react-art/cjs/react-art.production.min.js +0.20% 94.40 kB 94.60 kB +0.30% 29.05 kB 29.13 kB
oss-stable/react-art/cjs/react-art.production.min.js +0.20% 94.45 kB 94.65 kB +0.31% 29.07 kB 29.16 kB

Generated by 🚫 dangerJS against 439ca59

@acdlite acdlite force-pushed the detect-async-component-crash branch 2 times, most recently from 18fced0 to f40e936 Compare June 27, 2023 22:25
Comment on lines 1995 to 1998
'async/await is not yet supported in Client Components, ' +
'only Server Components. This error is often caused by ' +
"accidentally adding `'use client'` to a module that " +
'was originally written for the server.',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'async/await is not yet supported in Client Components, ' +
'only Server Components. This error is often caused by ' +
"accidentally adding `'use client'` to a module that " +
'was originally written for the server.',
'Detected an infinite suspense loop while, ' +
'rendering the app shell. This error is often caused by ' +
"accidentally adding `'use client'` to a module that " +
'was originally written for the server.',

Copy link
Collaborator Author

@acdlite acdlite Jun 28, 2023

Choose a reason for hiding this comment

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

I wanted the message to focus on the mistake that led to the crash. The fact that falls into a loop is an implementation detail; the reason for the error is we just don't support this feature yet.

Is the reason for the suggestion because technically there are other ways the error might occur besides async/await?

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Ok so this is limited to the weird special case where you suspend in the shell with a sync update - which then causes a suspended sync update which then retries synchronously.

@acdlite acdlite force-pushed the detect-async-component-crash branch 4 times, most recently from 8819a4f to 4848659 Compare June 29, 2023 19:52
Suspending with an uncached promise is not yet supported. We only
support suspending on promises that are cached between render attempts.
(We do plan to partially support this in the future, at least in certain
constrained cases, like during a route transition.)

This includes the case where a component returns an uncached promise,
which is effectively what happens if a Client Component is authored
using async/await syntax.

This is an easy mistake to make in a Server Components app, because
async/await _is_ available in Server Components.

In the current behavior, this can sometimes cause the app to crash with
an infinite loop, because React will repeatedly keep trying to render
the component, which will result in a fresh promise, which will result
in a new render attempt, and so on. We have some strategies we can use
to prevent this — during a concurrent render, we can suspend the work
loop until the promise resolves. If it's not a concurrent render, we can
show a Suspense fallback and try again at concurrent priority.

There's one case where neither of these strategies work, though: during
a sync render when there's no parent Suspense boundary. (We refer to
this as the "shell" of the app because it exists outside of any
loading UI.)

Since we don't have any great options for this scenario, we should at
least error gracefully instead of crashing the app.

So this commit adds a detection mechanism for render loops caused by
async client components. The way it works is, if an app suspends
repeatedly in the shell during a synchronous render, without committing
anything in between, we will count the number of attempts and eventually
trigger an error once the count exceeds a threshold.

In the future, we will consider ways to make this case a warning instead
of a hard error.

See facebook#26801 for more details.
@acdlite acdlite force-pushed the detect-async-component-crash branch from 4848659 to 439ca59 Compare June 29, 2023 19:55
@acdlite acdlite merged commit fc80111 into facebook:main Jun 29, 2023
2 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 29, 2023
Suspending with an uncached promise is not yet supported. We only
support suspending on promises that are cached between render attempts.
(We do plan to partially support this in the future, at least in certain
constrained cases, like during a route transition.)

This includes the case where a component returns an uncached promise,
which is effectively what happens if a Client Component is authored
using async/await syntax.

This is an easy mistake to make in a Server Components app, because
async/await _is_ available in Server Components.

In the current behavior, this can sometimes cause the app to crash with
an infinite loop, because React will repeatedly keep trying to render
the component, which will result in a fresh promise, which will result
in a new render attempt, and so on. We have some strategies we can use
to prevent this — during a concurrent render, we can suspend the work
loop until the promise resolves. If it's not a concurrent render, we can
show a Suspense fallback and try again at concurrent priority.

There's one case where neither of these strategies work, though: during
a sync render when there's no parent Suspense boundary. (We refer to
this as the "shell" of the app because it exists outside of any loading
UI.)

Since we don't have any great options for this scenario, we should at
least error gracefully instead of crashing the app.

So this commit adds a detection mechanism for render loops caused by
async client components. The way it works is, if an app suspends
repeatedly in the shell during a synchronous render, without committing
anything in between, we will count the number of attempts and eventually
trigger an error once the count exceeds a threshold.

In the future, we will consider ways to make this case a warning instead
of a hard error.

See #26801 for more details.

DiffTrain build for [fc80111](fc80111)
acdlite added a commit to acdlite/react that referenced this pull request Jun 30, 2023
Adds a development warning to complement the error introduced by facebook#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in facebook#27019. It does not
supersede the infinite loop guard, though, because that mechanism also
prevents the app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a
lint rule.)
acdlite added a commit to acdlite/react that referenced this pull request Jun 30, 2023
Adds a development warning to complement the error introduced by facebook#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in facebook#27019. It does not
supersede the infinite loop guard, though, because that mechanism also
prevents the app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a
lint rule.)
acdlite added a commit to acdlite/react that referenced this pull request Jun 30, 2023
Adds a development warning to complement the error introduced by facebook#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in facebook#27019. It does not
supersede the infinite loop guard, though, because that mechanism also
prevents the app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a
lint rule.)
acdlite added a commit to acdlite/react that referenced this pull request Jun 30, 2023
Adds a development warning to complement the error introduced by facebook#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in facebook#27019. It does not
supersede the infinite loop guard, though, because that mechanism also
prevents the app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a
lint rule.)
acdlite added a commit to acdlite/react that referenced this pull request Jul 2, 2023
Adds a development warning to complement the error introduced by facebook#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in facebook#27019. It does not
supersede the infinite loop guard, though, because that mechanism also
prevents the app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a
lint rule.)
acdlite added a commit to acdlite/react that referenced this pull request Jul 2, 2023
Adds a development warning to complement the error introduced by facebook#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in facebook#27019. It does not
supersede the infinite loop guard, though, because that mechanism also
prevents the app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a
lint rule.)
acdlite added a commit that referenced this pull request Jul 2, 2023
…#27031)

Adds a development warning to complement the error introduced by
#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in
#27019. It does not supersede the
infinite loop guard, though, because that mechanism also prevents the
app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a lint
rule.)
github-actions bot pushed a commit that referenced this pull request Jul 2, 2023
…#27031)

Adds a development warning to complement the error introduced by
#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in
#27019. It does not supersede the
infinite loop guard, though, because that mechanism also prevents the
app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a lint
rule.)

DiffTrain build for [5c8dabf](5c8dabf)
jerrydev0927 added a commit to jerrydev0927/react that referenced this pull request Jan 5, 2024
… (#27031)

Adds a development warning to complement the error introduced by
facebook/react#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in
facebook/react#27019. It does not supersede the
infinite loop guard, though, because that mechanism also prevents the
app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a lint
rule.)

DiffTrain build for [5c8dabf8864e1d826c831d6096b2dfa66309961a](facebook/react@5c8dabf)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
Suspending with an uncached promise is not yet supported. We only
support suspending on promises that are cached between render attempts.
(We do plan to partially support this in the future, at least in certain
constrained cases, like during a route transition.)

This includes the case where a component returns an uncached promise,
which is effectively what happens if a Client Component is authored
using async/await syntax.

This is an easy mistake to make in a Server Components app, because
async/await _is_ available in Server Components.

In the current behavior, this can sometimes cause the app to crash with
an infinite loop, because React will repeatedly keep trying to render
the component, which will result in a fresh promise, which will result
in a new render attempt, and so on. We have some strategies we can use
to prevent this — during a concurrent render, we can suspend the work
loop until the promise resolves. If it's not a concurrent render, we can
show a Suspense fallback and try again at concurrent priority.

There's one case where neither of these strategies work, though: during
a sync render when there's no parent Suspense boundary. (We refer to
this as the "shell" of the app because it exists outside of any loading
UI.)

Since we don't have any great options for this scenario, we should at
least error gracefully instead of crashing the app.

So this commit adds a detection mechanism for render loops caused by
async client components. The way it works is, if an app suspends
repeatedly in the shell during a synchronous render, without committing
anything in between, we will count the number of attempts and eventually
trigger an error once the count exceeds a threshold.

In the future, we will consider ways to make this case a warning instead
of a hard error.

See facebook#26801 for more details.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…facebook#27031)

Adds a development warning to complement the error introduced by
facebook#27019.

We can detect and warn about async client components by checking the
prototype of the function. This won't work for environments where async
functions are transpiled, but for native async functions, it allows us
to log an earlier warning during development, including in cases that
don't trigger the infinite loop guard added in
facebook#27019. It does not supersede the
infinite loop guard, though, because that mechanism also prevents the
app from crashing.

I also added a warning for calling a hook inside an async function. This
one fires even during a transition. We could add a corresponding warning
to Flight, since hooks are not allowed in async Server Components,
either. (Though in both environments, this is better handled by a lint
rule.)
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
Suspending with an uncached promise is not yet supported. We only
support suspending on promises that are cached between render attempts.
(We do plan to partially support this in the future, at least in certain
constrained cases, like during a route transition.)

This includes the case where a component returns an uncached promise,
which is effectively what happens if a Client Component is authored
using async/await syntax.

This is an easy mistake to make in a Server Components app, because
async/await _is_ available in Server Components.

In the current behavior, this can sometimes cause the app to crash with
an infinite loop, because React will repeatedly keep trying to render
the component, which will result in a fresh promise, which will result
in a new render attempt, and so on. We have some strategies we can use
to prevent this — during a concurrent render, we can suspend the work
loop until the promise resolves. If it's not a concurrent render, we can
show a Suspense fallback and try again at concurrent priority.

There's one case where neither of these strategies work, though: during
a sync render when there's no parent Suspense boundary. (We refer to
this as the "shell" of the app because it exists outside of any loading
UI.)

Since we don't have any great options for this scenario, we should at
least error gracefully instead of crashing the app.

So this commit adds a detection mechanism for render loops caused by
async client components. The way it works is, if an app suspends
repeatedly in the shell during a synchronous render, without committing
anything in between, we will count the number of attempts and eventually
trigger an error once the count exceeds a threshold.

In the future, we will consider ways to make this case a warning instead
of a hard error.

See #26801 for more details.

DiffTrain build for commit fc80111.
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.

5 participants