From 439ca59441ced8751368ccc84f46d0df5b12cc34 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 27 Jun 2023 17:53:34 -0400 Subject: [PATCH] Detect crashes caused by Async Client Components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 https://github.com/facebook/react/issues/26801 for more details. --- .../react-reconciler/src/ReactFiberLane.js | 1 + .../react-reconciler/src/ReactFiberRoot.js | 1 + .../src/ReactFiberThenable.js | 28 ++++++ .../src/ReactFiberWorkLoop.js | 19 ++++ .../src/ReactInternalTypes.js | 1 + .../src/__tests__/ReactUse-test.js | 93 +++++++++++++++++++ scripts/error-codes/codes.json | 3 +- 7 files changed, 145 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js index e90b02e441958..51126c6cdbcc4 100644 --- a/packages/react-reconciler/src/ReactFiberLane.js +++ b/packages/react-reconciler/src/ReactFiberLane.js @@ -636,6 +636,7 @@ export function markRootFinished(root: FiberRoot, remainingLanes: Lanes) { root.entangledLanes &= remainingLanes; root.errorRecoveryDisabledLanes &= remainingLanes; + root.shellSuspendCounter = 0; const entanglements = root.entanglements; const expirationTimes = root.expirationTimes; diff --git a/packages/react-reconciler/src/ReactFiberRoot.js b/packages/react-reconciler/src/ReactFiberRoot.js index 7b295bdbe80b8..e65e25b97df6b 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.js +++ b/packages/react-reconciler/src/ReactFiberRoot.js @@ -74,6 +74,7 @@ function FiberRootNode( this.expiredLanes = NoLanes; this.finishedLanes = NoLanes; this.errorRecoveryDisabledLanes = NoLanes; + this.shellSuspendCounter = 0; this.entangledLanes = NoLanes; this.entanglements = createLaneMap(NoLanes); diff --git a/packages/react-reconciler/src/ReactFiberThenable.js b/packages/react-reconciler/src/ReactFiberThenable.js index d759e692dd035..bc2aecf8b3d8a 100644 --- a/packages/react-reconciler/src/ReactFiberThenable.js +++ b/packages/react-reconciler/src/ReactFiberThenable.js @@ -14,6 +14,8 @@ import type { RejectedThenable, } from 'shared/ReactTypes'; +import {getWorkInProgressRoot} from './ReactFiberWorkLoop'; + import ReactSharedInternals from 'shared/ReactSharedInternals'; const {ReactCurrentActQueue} = ReactSharedInternals; @@ -103,6 +105,32 @@ export function trackUsedThenable( // happen. Flight lazily parses JSON when the value is actually awaited. thenable.then(noop, noop); } else { + // This is an uncached thenable that we haven't seen before. + + // Detect infinite ping loops caused by uncached promises. + const root = getWorkInProgressRoot(); + if (root !== null && root.shellSuspendCounter > 100) { + // This root has suspended repeatedly in the shell without making any + // progress (i.e. committing something). This is highly suggestive of + // an infinite ping loop, often caused by an accidental Async Client + // Component. + // + // During a transition, we can suspend the work loop until the promise + // to resolve, but this is a sync render, so that's not an option. We + // also can't show a fallback, because none was provided. So our last + // resort is to throw an error. + // + // TODO: Remove this error in a future release. Other ways of handling + // this case include forcing a concurrent render, or putting the whole + // root into offscreen mode. + throw new Error( + '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.', + ); + } + const pendingThenable: PendingThenable = (thenable: any); pendingThenable.status = 'pending'; pendingThenable.then( diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index f486bf2648dc3..104616527fe97 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -1944,6 +1944,7 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { markRenderStarted(lanes); } + let didSuspendInShell = false; outer: do { try { if ( @@ -1969,6 +1970,13 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { workInProgressRootExitStatus = RootDidNotComplete; break outer; } + case SuspendedOnImmediate: + case SuspendedOnData: { + if (!didSuspendInShell && getSuspenseHandler() === null) { + didSuspendInShell = true; + } + // Intentional fallthrough + } default: { // Unwind then continue with the normal work loop. workInProgressSuspendedReason = NotSuspended; @@ -1984,6 +1992,17 @@ function renderRootSync(root: FiberRoot, lanes: Lanes) { handleThrow(root, thrownValue); } } while (true); + + // Check if something suspended in the shell. We use this to detect an + // infinite ping loop caused by an uncached promise. + // + // Only increment this counter once per synchronous render attempt across the + // whole tree. Even if there are many sibling components that suspend, this + // counter only gets incremented once. + if (didSuspendInShell) { + root.shellSuspendCounter++; + } + resetContextDependencies(); executionContext = prevExecutionContext; diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index eae140ac4ed73..50a0ec85f9f25 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -248,6 +248,7 @@ type BaseFiberRootProperties = { pingedLanes: Lanes, expiredLanes: Lanes, errorRecoveryDisabledLanes: Lanes, + shellSuspendCounter: number, finishedLanes: Lanes, diff --git a/packages/react-reconciler/src/__tests__/ReactUse-test.js b/packages/react-reconciler/src/__tests__/ReactUse-test.js index ce3add950be5f..219fc7d0c5521 100644 --- a/packages/react-reconciler/src/__tests__/ReactUse-test.js +++ b/packages/react-reconciler/src/__tests__/ReactUse-test.js @@ -17,6 +17,7 @@ let waitFor; let waitForPaint; let assertLog; let waitForAll; +let waitForMicrotasks; describe('ReactUse', () => { beforeEach(() => { @@ -40,6 +41,7 @@ describe('ReactUse', () => { assertLog = InternalTestUtils.assertLog; waitForPaint = InternalTestUtils.waitForPaint; waitFor = InternalTestUtils.waitFor; + waitForMicrotasks = InternalTestUtils.waitForMicrotasks; pendingTextRequests = new Map(); }); @@ -1616,4 +1618,95 @@ describe('ReactUse', () => { assertLog(['C']); expect(root).toMatchRenderedOutput('C'); }); + + // @gate !forceConcurrentByDefaultForTesting + test('an async component outside of a Suspense boundary crashes with an error (resolves in microtask)', async () => { + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + return {error}; + } + render() { + if (this.state.error) { + return ; + } + return this.props.children; + } + } + + async function AsyncClientComponent() { + return ; + } + + const root = ReactNoop.createRoot(); + await act(() => { + root.render( + + + , + ); + }); + assertLog([ + '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.', + '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.', + ]); + expect(root).toMatchRenderedOutput( + '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.', + ); + }); + + // @gate !forceConcurrentByDefaultForTesting + test('an async component outside of a Suspense boundary crashes with an error (resolves in macrotask)', async () => { + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + return {error}; + } + render() { + if (this.state.error) { + return ; + } + return this.props.children; + } + } + + async function AsyncClientComponent() { + await waitForMicrotasks(); + return ; + } + + const root = ReactNoop.createRoot(); + await act(() => { + root.render( + + + , + ); + }); + assertLog([ + '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.', + '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.', + ]); + expect(root).toMatchRenderedOutput( + '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.', + ); + }); }); diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index b1ae802b33b56..3423de2fa99b4 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -466,5 +466,6 @@ "478": "Thenable should have already resolved. This is a bug in React.", "479": "Cannot update optimistic state while rendering.", "480": "File/Blob fields are not yet supported in progressive forms. It probably means you are closing over binary data or FormData in a Server Action.", - "481": "Tried to encode a Server Action from a different instance than the encoder is from. This is a bug in React." + "481": "Tried to encode a Server Action from a different instance than the encoder is from. This is a bug in React.", + "482": "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." }