From 36426e6cb6b6998cba934d239ba6274c9119118c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 1 Nov 2022 12:00:34 -0700 Subject: [PATCH] Allow uncached IO to stablize (#25561) Initial draft. I need to test this more. If a promise is passed to `use`, but the I/O isn't cached, we should still be able to unwrap it. This already worked in Server Components, and during SSR. For Fiber (in the browser), before this fix the state would get lost between attempts unless the promise resolved immediately in a microtask, which requires IO to be cached. This was due to an implementation quirk of Fiber where the state is reset as soon as the stack unwinds. The workaround is to suspend the entire Fiber work loop until the promise resolves. The Server Components and SSR runtimes don't require a workaround: they can maintain multiple parallel child tasks and reuse the state indefinitely across attempts. That's ideally how Fiber should work, too, but it will require larger refactor. The downside of our approach in Fiber is that it won't "warm up" the siblings while you're suspended, but to avoid waterfalls you're supposed to hoist data fetches higher in the tree regardless. But we have other ideas for how we can add this back in the future. (Though again, this doesn't affect Server Components, which already have the ideal behavior.) --- .../src/ReactFiberWorkLoop.new.js | 130 +++++++++++-- .../src/ReactFiberWorkLoop.old.js | 130 +++++++++++-- .../src/__tests__/ReactThenable-test.js | 173 ++++++++++++++++-- 3 files changed, 388 insertions(+), 45 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index b5271151a5414..c7326aaafa28c 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -9,10 +9,13 @@ import {REACT_STRICT_MODE_TYPE} from 'shared/ReactSymbols'; -import type {Wakeable} from 'shared/ReactTypes'; +import type {Wakeable, Thenable} from 'shared/ReactTypes'; import type {Fiber, FiberRoot} from './ReactInternalTypes'; import type {Lanes, Lane} from './ReactFiberLane.new'; -import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; +import type { + SuspenseProps, + SuspenseState, +} from './ReactFiberSuspenseComponent.new'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new'; import type {EventPriority} from './ReactEventPriorities.new'; import type { @@ -271,6 +274,10 @@ import { isThenableStateResolved, } from './ReactFiberThenable.new'; import {schedulePostPaintCallback} from './ReactPostPaintCallback'; +import { + getSuspenseHandler, + isBadSuspenseFallback, +} from './ReactFiberSuspenseContext.new'; const ceil = Math.ceil; @@ -312,7 +319,7 @@ let workInProgressRootRenderLanes: Lanes = NoLanes; opaque type SuspendedReason = 0 | 1 | 2 | 3 | 4; const NotSuspended: SuspendedReason = 0; const SuspendedOnError: SuspendedReason = 1; -// const SuspendedOnData: SuspendedReason = 2; +const SuspendedOnData: SuspendedReason = 2; const SuspendedOnImmediate: SuspendedReason = 3; const SuspendedAndReadyToUnwind: SuspendedReason = 4; @@ -706,6 +713,18 @@ export function scheduleUpdateOnFiber( } } + // Check if the work loop is currently suspended and waiting for data to + // finish loading. + if ( + workInProgressSuspendedReason === SuspendedOnData && + root === workInProgressRoot + ) { + // The incoming update might unblock the current render. Interrupt the + // current attempt and restart from the top. + prepareFreshStack(root, NoLanes); + markRootSuspended(root, workInProgressRootRenderLanes); + } + // Mark that the root has a pending update. markRootUpdated(root, lane, eventTime); @@ -1130,6 +1149,20 @@ function performConcurrentWorkOnRoot(root, didTimeout) { if (root.callbackNode === originalCallbackNode) { // The task node scheduled for this root is the same one that's // currently executed. Need to return a continuation. + if ( + workInProgressSuspendedReason === SuspendedOnData && + workInProgressRoot === root + ) { + // Special case: The work loop is currently suspended and waiting for + // data to resolve. Unschedule the current task. + // + // TODO: The factoring is a little weird. Arguably this should be checked + // in ensureRootIsScheduled instead. I went back and forth, not totally + // sure yet. + root.callbackPriority = NoLane; + root.callbackNode = null; + return null; + } return performConcurrentWorkOnRoot.bind(null, root); } return null; @@ -1739,7 +1772,9 @@ function handleThrow(root, thrownValue): void { // deprecate the old API in favor of `use`. thrownValue = getSuspendedThenable(); workInProgressSuspendedThenableState = getThenableStateAfterSuspending(); - workInProgressSuspendedReason = SuspendedOnImmediate; + workInProgressSuspendedReason = shouldAttemptToSuspendUntilDataResolves() + ? SuspendedOnData + : SuspendedOnImmediate; } else { // This is a regular error. If something earlier in the component already // suspended, we must clear the thenable state to unblock the work loop. @@ -1796,6 +1831,48 @@ function handleThrow(root, thrownValue): void { } } +function shouldAttemptToSuspendUntilDataResolves() { + // TODO: We should be able to move the + // renderDidSuspend/renderDidSuspendWithDelay logic into this function, + // instead of repeating it in the complete phase. Or something to that effect. + + if (includesOnlyRetries(workInProgressRootRenderLanes)) { + // We can always wait during a retry. + return true; + } + + // TODO: We should be able to remove the equivalent check in + // finishConcurrentRender, and rely just on this one. + if (includesOnlyTransitions(workInProgressRootRenderLanes)) { + const suspenseHandler = getSuspenseHandler(); + if (suspenseHandler !== null && suspenseHandler.tag === SuspenseComponent) { + const currentSuspenseHandler = suspenseHandler.alternate; + const nextProps: SuspenseProps = suspenseHandler.memoizedProps; + if (isBadSuspenseFallback(currentSuspenseHandler, nextProps)) { + // The nearest Suspense boundary is already showing content. We should + // avoid replacing it with a fallback, and instead wait until the + // data finishes loading. + return true; + } else { + // This is not a bad fallback condition. We should show a fallback + // immediately instead of waiting for the data to resolve. This includes + // when suspending inside new trees. + return false; + } + } + + // During a transition, if there is no Suspense boundary (i.e. suspending in + // the "shell" of an application), or if we're inside a hidden tree, then + // we should wait until the data finishes loading. + return true; + } + + // For all other Lanes besides Transitions and Retries, we should not wait + // for the data to load. + // TODO: We should wait during Offscreen prerendering, too. + return false; +} + function pushDispatcher(container) { prepareRendererToRender(container); const prevDispatcher = ReactCurrentDispatcher.current; @@ -2060,7 +2137,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { markRenderStarted(lanes); } - do { + outer: do { try { if ( workInProgressSuspendedReason !== NotSuspended && @@ -2070,19 +2147,48 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { // replay the suspended component. const unitOfWork = workInProgress; const thrownValue = workInProgressThrownValue; - workInProgressSuspendedReason = NotSuspended; - workInProgressThrownValue = null; switch (workInProgressSuspendedReason) { case SuspendedOnError: { // Unwind then continue with the normal work loop. + workInProgressSuspendedReason = NotSuspended; + workInProgressThrownValue = null; unwindSuspendedUnitOfWork(unitOfWork, thrownValue); break; } + case SuspendedOnData: { + const didResolve = + workInProgressSuspendedThenableState !== null && + isThenableStateResolved(workInProgressSuspendedThenableState); + if (didResolve) { + workInProgressSuspendedReason = NotSuspended; + workInProgressThrownValue = null; + replaySuspendedUnitOfWork(unitOfWork, thrownValue); + } else { + // The work loop is suspended on data. We should wait for it to + // resolve before continuing to render. + const thenable: Thenable = (workInProgressThrownValue: any); + const onResolution = () => { + ensureRootIsScheduled(root, now()); + }; + thenable.then(onResolution, onResolution); + break outer; + } + break; + } + case SuspendedOnImmediate: { + // If this fiber just suspended, it's possible the data is already + // cached. Yield to the main thread to give it a chance to ping. If + // it does, we can retry immediately without unwinding the stack. + workInProgressSuspendedReason = SuspendedAndReadyToUnwind; + break outer; + } default: { - const wasPinged = + workInProgressSuspendedReason = NotSuspended; + workInProgressThrownValue = null; + const didResolve = workInProgressSuspendedThenableState !== null && isThenableStateResolved(workInProgressSuspendedThenableState); - if (wasPinged) { + if (didResolve) { replaySuspendedUnitOfWork(unitOfWork, thrownValue); } else { unwindSuspendedUnitOfWork(unitOfWork, thrownValue); @@ -2096,12 +2202,6 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { break; } catch (thrownValue) { handleThrow(root, thrownValue); - if (workInProgressSuspendedThenableState !== null) { - // If this fiber just suspended, it's possible the data is already - // cached. Yield to the main thread to give it a chance to ping. If - // it does, we can retry immediately without unwinding the stack. - break; - } } } while (true); resetContextDependencies(); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 4510f2a31f58f..04f835ee9bf56 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -9,10 +9,13 @@ import {REACT_STRICT_MODE_TYPE} from 'shared/ReactSymbols'; -import type {Wakeable} from 'shared/ReactTypes'; +import type {Wakeable, Thenable} from 'shared/ReactTypes'; import type {Fiber, FiberRoot} from './ReactInternalTypes'; import type {Lanes, Lane} from './ReactFiberLane.old'; -import type {SuspenseState} from './ReactFiberSuspenseComponent.old'; +import type { + SuspenseProps, + SuspenseState, +} from './ReactFiberSuspenseComponent.old'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.old'; import type {EventPriority} from './ReactEventPriorities.old'; import type { @@ -271,6 +274,10 @@ import { isThenableStateResolved, } from './ReactFiberThenable.old'; import {schedulePostPaintCallback} from './ReactPostPaintCallback'; +import { + getSuspenseHandler, + isBadSuspenseFallback, +} from './ReactFiberSuspenseContext.old'; const ceil = Math.ceil; @@ -312,7 +319,7 @@ let workInProgressRootRenderLanes: Lanes = NoLanes; opaque type SuspendedReason = 0 | 1 | 2 | 3 | 4; const NotSuspended: SuspendedReason = 0; const SuspendedOnError: SuspendedReason = 1; -// const SuspendedOnData: SuspendedReason = 2; +const SuspendedOnData: SuspendedReason = 2; const SuspendedOnImmediate: SuspendedReason = 3; const SuspendedAndReadyToUnwind: SuspendedReason = 4; @@ -706,6 +713,18 @@ export function scheduleUpdateOnFiber( } } + // Check if the work loop is currently suspended and waiting for data to + // finish loading. + if ( + workInProgressSuspendedReason === SuspendedOnData && + root === workInProgressRoot + ) { + // The incoming update might unblock the current render. Interrupt the + // current attempt and restart from the top. + prepareFreshStack(root, NoLanes); + markRootSuspended(root, workInProgressRootRenderLanes); + } + // Mark that the root has a pending update. markRootUpdated(root, lane, eventTime); @@ -1130,6 +1149,20 @@ function performConcurrentWorkOnRoot(root, didTimeout) { if (root.callbackNode === originalCallbackNode) { // The task node scheduled for this root is the same one that's // currently executed. Need to return a continuation. + if ( + workInProgressSuspendedReason === SuspendedOnData && + workInProgressRoot === root + ) { + // Special case: The work loop is currently suspended and waiting for + // data to resolve. Unschedule the current task. + // + // TODO: The factoring is a little weird. Arguably this should be checked + // in ensureRootIsScheduled instead. I went back and forth, not totally + // sure yet. + root.callbackPriority = NoLane; + root.callbackNode = null; + return null; + } return performConcurrentWorkOnRoot.bind(null, root); } return null; @@ -1739,7 +1772,9 @@ function handleThrow(root, thrownValue): void { // deprecate the old API in favor of `use`. thrownValue = getSuspendedThenable(); workInProgressSuspendedThenableState = getThenableStateAfterSuspending(); - workInProgressSuspendedReason = SuspendedOnImmediate; + workInProgressSuspendedReason = shouldAttemptToSuspendUntilDataResolves() + ? SuspendedOnData + : SuspendedOnImmediate; } else { // This is a regular error. If something earlier in the component already // suspended, we must clear the thenable state to unblock the work loop. @@ -1796,6 +1831,48 @@ function handleThrow(root, thrownValue): void { } } +function shouldAttemptToSuspendUntilDataResolves() { + // TODO: We should be able to move the + // renderDidSuspend/renderDidSuspendWithDelay logic into this function, + // instead of repeating it in the complete phase. Or something to that effect. + + if (includesOnlyRetries(workInProgressRootRenderLanes)) { + // We can always wait during a retry. + return true; + } + + // TODO: We should be able to remove the equivalent check in + // finishConcurrentRender, and rely just on this one. + if (includesOnlyTransitions(workInProgressRootRenderLanes)) { + const suspenseHandler = getSuspenseHandler(); + if (suspenseHandler !== null && suspenseHandler.tag === SuspenseComponent) { + const currentSuspenseHandler = suspenseHandler.alternate; + const nextProps: SuspenseProps = suspenseHandler.memoizedProps; + if (isBadSuspenseFallback(currentSuspenseHandler, nextProps)) { + // The nearest Suspense boundary is already showing content. We should + // avoid replacing it with a fallback, and instead wait until the + // data finishes loading. + return true; + } else { + // This is not a bad fallback condition. We should show a fallback + // immediately instead of waiting for the data to resolve. This includes + // when suspending inside new trees. + return false; + } + } + + // During a transition, if there is no Suspense boundary (i.e. suspending in + // the "shell" of an application), or if we're inside a hidden tree, then + // we should wait until the data finishes loading. + return true; + } + + // For all other Lanes besides Transitions and Retries, we should not wait + // for the data to load. + // TODO: We should wait during Offscreen prerendering, too. + return false; +} + function pushDispatcher(container) { prepareRendererToRender(container); const prevDispatcher = ReactCurrentDispatcher.current; @@ -2060,7 +2137,7 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { markRenderStarted(lanes); } - do { + outer: do { try { if ( workInProgressSuspendedReason !== NotSuspended && @@ -2070,19 +2147,48 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { // replay the suspended component. const unitOfWork = workInProgress; const thrownValue = workInProgressThrownValue; - workInProgressSuspendedReason = NotSuspended; - workInProgressThrownValue = null; switch (workInProgressSuspendedReason) { case SuspendedOnError: { // Unwind then continue with the normal work loop. + workInProgressSuspendedReason = NotSuspended; + workInProgressThrownValue = null; unwindSuspendedUnitOfWork(unitOfWork, thrownValue); break; } + case SuspendedOnData: { + const didResolve = + workInProgressSuspendedThenableState !== null && + isThenableStateResolved(workInProgressSuspendedThenableState); + if (didResolve) { + workInProgressSuspendedReason = NotSuspended; + workInProgressThrownValue = null; + replaySuspendedUnitOfWork(unitOfWork, thrownValue); + } else { + // The work loop is suspended on data. We should wait for it to + // resolve before continuing to render. + const thenable: Thenable = (workInProgressThrownValue: any); + const onResolution = () => { + ensureRootIsScheduled(root, now()); + }; + thenable.then(onResolution, onResolution); + break outer; + } + break; + } + case SuspendedOnImmediate: { + // If this fiber just suspended, it's possible the data is already + // cached. Yield to the main thread to give it a chance to ping. If + // it does, we can retry immediately without unwinding the stack. + workInProgressSuspendedReason = SuspendedAndReadyToUnwind; + break outer; + } default: { - const wasPinged = + workInProgressSuspendedReason = NotSuspended; + workInProgressThrownValue = null; + const didResolve = workInProgressSuspendedThenableState !== null && isThenableStateResolved(workInProgressSuspendedThenableState); - if (wasPinged) { + if (didResolve) { replaySuspendedUnitOfWork(unitOfWork, thrownValue); } else { unwindSuspendedUnitOfWork(unitOfWork, thrownValue); @@ -2096,12 +2202,6 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) { break; } catch (thrownValue) { handleThrow(root, thrownValue); - if (workInProgressSuspendedThenableState !== null) { - // If this fiber just suspended, it's possible the data is already - // cached. Yield to the main thread to give it a chance to ping. If - // it does, we can retry immediately without unwinding the stack. - break; - } } } while (true); resetContextDependencies(); diff --git a/packages/react-reconciler/src/__tests__/ReactThenable-test.js b/packages/react-reconciler/src/__tests__/ReactThenable-test.js index fd95a43bfab8f..887cd26080d8f 100644 --- a/packages/react-reconciler/src/__tests__/ReactThenable-test.js +++ b/packages/react-reconciler/src/__tests__/ReactThenable-test.js @@ -7,6 +7,7 @@ let act; let use; let Suspense; let startTransition; +let pendingTextRequests; describe('ReactThenable', () => { beforeEach(() => { @@ -19,11 +20,37 @@ describe('ReactThenable', () => { use = React.use; Suspense = React.Suspense; startTransition = React.startTransition; + + pendingTextRequests = new Map(); }); - function Text(props) { - Scheduler.unstable_yieldValue(props.text); - return props.text; + function resolveTextRequests(text) { + const requests = pendingTextRequests.get(text); + if (requests !== undefined) { + pendingTextRequests.delete(text); + requests.forEach(resolve => resolve(text)); + } + } + + function getAsyncText(text) { + // getAsyncText is completely uncached — it performs a new async operation + // every time it's called. During a transition, React should be able to + // unwrap it anyway. + Scheduler.unstable_yieldValue(`Async text requested [${text}]`); + return new Promise(resolve => { + const requests = pendingTextRequests.get(text); + if (requests !== undefined) { + requests.push(resolve); + pendingTextRequests.set(text, requests); + } else { + pendingTextRequests.set(text, [resolve]); + } + }); + } + + function Text({text}) { + Scheduler.unstable_yieldValue(text); + return text; } // This behavior was intentionally disabled to derisk the rollout of `use`. @@ -32,15 +59,15 @@ describe('ReactThenable', () => { // to `use`, so the extra code probably isn't worth it. // @gate TODO test('if suspended fiber is pinged in a microtask, retry immediately without unwinding the stack', async () => { - let resolved = false; + let fulfilled = false; function Async() { - if (resolved) { + if (fulfilled) { return ; } Scheduler.unstable_yieldValue('Suspend!'); throw Promise.resolve().then(() => { Scheduler.unstable_yieldValue('Resolve in microtask'); - resolved = true; + fulfilled = true; }); } @@ -71,15 +98,15 @@ describe('ReactThenable', () => { }); test('if suspended fiber is pinged in a microtask, it does not block a transition from completing', async () => { - let resolved = false; + let fulfilled = false; function Async() { - if (resolved) { + if (fulfilled) { return ; } Scheduler.unstable_yieldValue('Suspend!'); throw Promise.resolve().then(() => { Scheduler.unstable_yieldValue('Resolve in microtask'); - resolved = true; + fulfilled = true; }); } @@ -101,13 +128,13 @@ describe('ReactThenable', () => { expect(root).toMatchRenderedOutput('Async'); }); - test('does not infinite loop if already resolved thenable is thrown', async () => { - // An already resolved promise should never be thrown. Since it already - // resolved, we shouldn't bother trying to render again — doing so would + test('does not infinite loop if already fulfilled thenable is thrown', async () => { + // An already fulfilled promise should never be thrown. Since it already + // fulfilled, we shouldn't bother trying to render again — doing so would // likely lead to an infinite loop. This scenario should only happen if a // userspace Suspense library makes an implementation mistake. - // Create an already resolved thenable + // Create an already fulfilled thenable const thenable = { then(ping) {}, status: 'fulfilled', @@ -120,7 +147,7 @@ describe('ReactThenable', () => { throw new Error('Infinite loop detected'); } Scheduler.unstable_yieldValue('Suspend!'); - // This thenable should never be thrown because it already resolved. + // This thenable should never be thrown because it already fulfilled. // But if it is thrown, React should handle it gracefully. throw thenable; } @@ -365,7 +392,7 @@ describe('ReactThenable', () => { expect(Scheduler).toHaveYielded([ // First attempt. The uncached promise suspends. 'Suspend! [Async]', - // Because the promise already resolved, we're able to unwrap the value + // Because the promise already fulfilled, we're able to unwrap the value // immediately in a microtask. // // Then we proceed to the rest of the component, which throws an error. @@ -497,4 +524,120 @@ describe('ReactThenable', () => { ); } }); + + // @gate enableUseHook + test('during a transition, can unwrap async operations even if nothing is cached', async () => { + function App() { + return ; + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render( + }> + + , + ); + }); + expect(Scheduler).toHaveYielded(['(empty)']); + expect(root).toMatchRenderedOutput('(empty)'); + + await act(async () => { + startTransition(() => { + root.render( + }> + + , + ); + }); + }); + expect(Scheduler).toHaveYielded(['Async text requested [Async]']); + expect(root).toMatchRenderedOutput('(empty)'); + + await act(async () => { + resolveTextRequests('Async'); + }); + expect(Scheduler).toHaveYielded(['Async text requested [Async]', 'Async']); + expect(root).toMatchRenderedOutput('Async'); + }); + + // @gate enableUseHook + test("does not prevent a Suspense fallback from showing if it's a new boundary, even during a transition", async () => { + function App() { + return ; + } + + const root = ReactNoop.createRoot(); + await act(async () => { + startTransition(() => { + root.render( + }> + + , + ); + }); + }); + // Even though the initial render was a transition, it shows a fallback. + expect(Scheduler).toHaveYielded([ + 'Async text requested [Async]', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput('Loading...'); + + // Resolve the original data + await act(async () => { + resolveTextRequests('Async'); + }); + // During the retry, a fresh request is initiated. Now we must wait for this + // one to finish. + // TODO: This is awkward. Intuitively, you might expect for `act` to wait + // until the new request has finished loading. But if it's mock IO, as in + // this test, how would the developer be able to imperatively flush it if it + // wasn't initiated until the current `act` call? Can't think of a better + // strategy at the moment. + expect(Scheduler).toHaveYielded(['Async text requested [Async]']); + expect(root).toMatchRenderedOutput('Loading...'); + + // Flush the second request. + await act(async () => { + resolveTextRequests('Async'); + }); + // This time it finishes because it was during a retry. + expect(Scheduler).toHaveYielded(['Async text requested [Async]', 'Async']); + expect(root).toMatchRenderedOutput('Async'); + }); + + // @gate enableUseHook + test('when waiting for data to resolve, a fresh update will trigger a restart', async () => { + function App() { + return ; + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(} />); + }); + + await act(async () => { + startTransition(() => { + root.render( + }> + + , + ); + }); + }); + expect(Scheduler).toHaveYielded([ + 'Async text requested [Will never resolve]', + ]); + + await act(async () => { + root.render( + }> + + , + ); + }); + expect(Scheduler).toHaveYielded(['Something different']); + }); });