From 2177c161fcc6440f36be642c7fbf2e6f438e2a40 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 23 Apr 2021 19:31:59 -0500 Subject: [PATCH] Expiration: Do nothing except disable time slicing We have a feature called "expiration" whose purpose is to prevent a concurrent update from being starved by higher priority events. If a lane is CPU-bound for too long, we finish the rest of the work synchronously without allowing further interruptions. In the current implementation, we do this in sort of a roundabout way: once a lane is determined to have expired, we entangle it with SyncLane and switch to the synchronous work loop. There are a few flaws with the approach. One is that SyncLane has a particular semantic meaning besides its non-yieldiness. For example, `flushSync` will force remaining Sync work to finish; currently, that also includes expired work, which isn't an intended behavior, but rather an artifact of the implementation. An event worse example is that passive effects triggered by a Sync update are flushed synchronously, before paint, so that its result is guaranteed to be observed by the next discrete event. But expired work has no such requirement: we're flushing expired effects before paint unnecessarily. Aside from the behaviorial implications, the current implementation has proven to be fragile: more than once, we've accidentally regressed performance due to a subtle change in how expiration is handled. This PR aims to radically simplify how we model starvation protection by scaling back the implementation as much as possible. In this new model, if a lane is expired, we disable time slicing. That's it. We don't entangle it with SyncLane. The only thing we do is skip the call to `shouldYield` in between each time slice. This is identical to how we model synchronous-by-default updates in React 18. --- .../src/ReactFiberLane.new.js | 38 +-- .../src/ReactFiberLane.old.js | 38 +-- .../src/ReactFiberRoot.new.js | 1 + .../src/ReactFiberRoot.old.js | 1 + .../src/ReactFiberWorkLoop.new.js | 38 +-- .../src/ReactFiberWorkLoop.old.js | 38 +-- .../src/ReactInternalTypes.js | 1 + .../src/__tests__/ReactExpiration-test.js | 313 ++++++++---------- .../ReactSchedulerIntegration-test.js | 3 +- .../ReactSuspenseWithNoopRenderer-test.js | 33 +- 10 files changed, 211 insertions(+), 293 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index dbbbc486265b9..878fc433f3c53 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -401,7 +401,6 @@ export function markStarvedLanesAsExpired( // expiration time. If so, we'll assume the update is being starved and mark // it as expired to force it to finish. let lanes = pendingLanes; - let expiredLanes = 0; while (lanes > 0) { const index = pickArbitraryLaneIndex(lanes); const lane = 1 << index; @@ -420,15 +419,11 @@ export function markStarvedLanesAsExpired( } } else if (expirationTime <= currentTime) { // This lane expired - expiredLanes |= lane; + root.expiredLanes |= lane; } lanes &= ~lane; } - - if (expiredLanes !== 0) { - markRootExpired(root, expiredLanes); - } } // This returns the highest priority pending lanes regardless of whether they @@ -459,16 +454,22 @@ export function includesOnlyTransitions(lanes: Lanes) { } export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) { - if (!enableSyncDefaultUpdates) { + if ((lanes & root.expiredLanes) !== NoLanes) { + // At least one of these lanes expired. To prevent additional starvation, + // finish rendering without yielding execution. + return false; + } + if (enableSyncDefaultUpdates) { + const SyncDefaultLanes = + InputContinuousHydrationLane | + InputContinuousLane | + DefaultHydrationLane | + DefaultLane; + // TODO: Check for root override, once that lands + return (lanes & SyncDefaultLanes) === NoLanes; + } else { return true; } - const SyncDefaultLanes = - InputContinuousHydrationLane | - InputContinuousLane | - DefaultHydrationLane | - DefaultLane; - // TODO: Check for root override, once that lands - return (lanes & SyncDefaultLanes) === NoLanes; } export function isTransitionLane(lane: Lane) { @@ -613,14 +614,6 @@ export function markRootPinged( root.pingedLanes |= root.suspendedLanes & pingedLanes; } -export function markRootExpired(root: FiberRoot, expiredLanes: Lanes) { - const entanglements = root.entanglements; - const SyncLaneIndex = 0; - entanglements[SyncLaneIndex] |= expiredLanes; - root.entangledLanes |= SyncLane; - root.pendingLanes |= SyncLane; -} - export function markRootMutableRead(root: FiberRoot, updateLane: Lane) { root.mutableReadLanes |= updateLane & root.pendingLanes; } @@ -634,6 +627,7 @@ export function markRootFinished(root: FiberRoot, remainingLanes: Lanes) { root.suspendedLanes = 0; root.pingedLanes = 0; + root.expiredLanes &= remainingLanes; root.mutableReadLanes &= remainingLanes; root.entangledLanes &= remainingLanes; diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js index af432cc6fd433..d4010468f0d74 100644 --- a/packages/react-reconciler/src/ReactFiberLane.old.js +++ b/packages/react-reconciler/src/ReactFiberLane.old.js @@ -401,7 +401,6 @@ export function markStarvedLanesAsExpired( // expiration time. If so, we'll assume the update is being starved and mark // it as expired to force it to finish. let lanes = pendingLanes; - let expiredLanes = 0; while (lanes > 0) { const index = pickArbitraryLaneIndex(lanes); const lane = 1 << index; @@ -420,15 +419,11 @@ export function markStarvedLanesAsExpired( } } else if (expirationTime <= currentTime) { // This lane expired - expiredLanes |= lane; + root.expiredLanes |= lane; } lanes &= ~lane; } - - if (expiredLanes !== 0) { - markRootExpired(root, expiredLanes); - } } // This returns the highest priority pending lanes regardless of whether they @@ -459,16 +454,22 @@ export function includesOnlyTransitions(lanes: Lanes) { } export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) { - if (!enableSyncDefaultUpdates) { + if ((lanes & root.expiredLanes) !== NoLanes) { + // At least one of these lanes expired. To prevent additional starvation, + // finish rendering without yielding execution. + return false; + } + if (enableSyncDefaultUpdates) { + const SyncDefaultLanes = + InputContinuousHydrationLane | + InputContinuousLane | + DefaultHydrationLane | + DefaultLane; + // TODO: Check for root override, once that lands + return (lanes & SyncDefaultLanes) === NoLanes; + } else { return true; } - const SyncDefaultLanes = - InputContinuousHydrationLane | - InputContinuousLane | - DefaultHydrationLane | - DefaultLane; - // TODO: Check for root override, once that lands - return (lanes & SyncDefaultLanes) === NoLanes; } export function isTransitionLane(lane: Lane) { @@ -613,14 +614,6 @@ export function markRootPinged( root.pingedLanes |= root.suspendedLanes & pingedLanes; } -export function markRootExpired(root: FiberRoot, expiredLanes: Lanes) { - const entanglements = root.entanglements; - const SyncLaneIndex = 0; - entanglements[SyncLaneIndex] |= expiredLanes; - root.entangledLanes |= SyncLane; - root.pendingLanes |= SyncLane; -} - export function markRootMutableRead(root: FiberRoot, updateLane: Lane) { root.mutableReadLanes |= updateLane & root.pendingLanes; } @@ -634,6 +627,7 @@ export function markRootFinished(root: FiberRoot, remainingLanes: Lanes) { root.suspendedLanes = 0; root.pingedLanes = 0; + root.expiredLanes &= remainingLanes; root.mutableReadLanes &= remainingLanes; root.entangledLanes &= remainingLanes; diff --git a/packages/react-reconciler/src/ReactFiberRoot.new.js b/packages/react-reconciler/src/ReactFiberRoot.new.js index 9201a35980753..728769261cbe9 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.new.js +++ b/packages/react-reconciler/src/ReactFiberRoot.new.js @@ -50,6 +50,7 @@ function FiberRootNode(containerInfo, tag, hydrate) { this.pendingLanes = NoLanes; this.suspendedLanes = NoLanes; this.pingedLanes = NoLanes; + this.expiredLanes = NoLanes; this.mutableReadLanes = NoLanes; this.finishedLanes = NoLanes; diff --git a/packages/react-reconciler/src/ReactFiberRoot.old.js b/packages/react-reconciler/src/ReactFiberRoot.old.js index f2968b314ecae..7f338a4dbb21a 100644 --- a/packages/react-reconciler/src/ReactFiberRoot.old.js +++ b/packages/react-reconciler/src/ReactFiberRoot.old.js @@ -50,6 +50,7 @@ function FiberRootNode(containerInfo, tag, hydrate) { this.pendingLanes = NoLanes; this.suspendedLanes = NoLanes; this.pingedLanes = NoLanes; + this.expiredLanes = NoLanes; this.mutableReadLanes = NoLanes; this.finishedLanes = NoLanes; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index c688214a100e5..ddf72f428dfd4 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -159,7 +159,7 @@ import { markRootUpdated, markRootSuspended as markRootSuspended_dontCallThisOneDirectly, markRootPinged, - markRootExpired, + markRootEntangled, markRootFinished, getHighestPriorityLane, addFiberToLanesMap, @@ -787,22 +787,17 @@ function performConcurrentWorkOnRoot(root, didTimeout) { return null; } + // We disable time-slicing in some cases: if the work has been CPU-bound + // for too long ("expired" work, to prevent starvation), or we're in + // sync-updates-by-default mode. // TODO: We only check `didTimeout` defensively, to account for a Scheduler // bug we're still investigating. Once the bug in Scheduler is fixed, // we can remove this, since we track expiration ourselves. - if (!disableSchedulerTimeoutInWorkLoop && didTimeout) { - // Something expired. Flush synchronously until there's no expired - // work left. - markRootExpired(root, lanes); - // This will schedule a synchronous callback. - ensureRootIsScheduled(root, now()); - return null; - } - - let exitStatus = shouldTimeSlice(root, lanes) - ? renderRootConcurrent(root, lanes) - : // Time slicing is disabled for default updates in this root. - renderRootSync(root, lanes); + let exitStatus = + shouldTimeSlice(root, lanes) && + (disableSchedulerTimeoutInWorkLoop || !didTimeout) + ? renderRootConcurrent(root, lanes) + : renderRootSync(root, lanes); if (exitStatus !== RootIncomplete) { if (exitStatus === RootErrored) { executionContext |= RetryAfterError; @@ -990,16 +985,7 @@ function performSyncWorkOnRoot(root) { flushPassiveEffects(); let lanes = getNextLanes(root, NoLanes); - if (includesSomeLane(lanes, SyncLane)) { - if ( - root === workInProgressRoot && - includesSomeLane(lanes, workInProgressRootRenderLanes) - ) { - // There's a partial tree, and at least one of its lanes has expired. Finish - // rendering it before rendering the rest of the expired work. - lanes = workInProgressRootRenderLanes; - } - } else { + if (!includesSomeLane(lanes, SyncLane)) { // There's no remaining sync work left. ensureRootIsScheduled(root, now()); return null; @@ -1052,11 +1038,9 @@ function performSyncWorkOnRoot(root) { return null; } -// TODO: Do we still need this API? I think we can delete it. Was only used -// internally. export function flushRoot(root: FiberRoot, lanes: Lanes) { if (lanes !== NoLanes) { - markRootExpired(root, lanes); + markRootEntangled(root, mergeLanes(lanes, SyncLane)); ensureRootIsScheduled(root, now()); if ((executionContext & (RenderContext | CommitContext)) === NoContext) { resetRenderTimer(); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index fb5ae4c1d7a25..3b0b196e112cd 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -159,7 +159,7 @@ import { markRootUpdated, markRootSuspended as markRootSuspended_dontCallThisOneDirectly, markRootPinged, - markRootExpired, + markRootEntangled, markRootFinished, getHighestPriorityLane, addFiberToLanesMap, @@ -787,22 +787,17 @@ function performConcurrentWorkOnRoot(root, didTimeout) { return null; } + // We disable time-slicing in some cases: if the work has been CPU-bound + // for too long ("expired" work, to prevent starvation), or we're in + // sync-updates-by-default mode. // TODO: We only check `didTimeout` defensively, to account for a Scheduler // bug we're still investigating. Once the bug in Scheduler is fixed, // we can remove this, since we track expiration ourselves. - if (!disableSchedulerTimeoutInWorkLoop && didTimeout) { - // Something expired. Flush synchronously until there's no expired - // work left. - markRootExpired(root, lanes); - // This will schedule a synchronous callback. - ensureRootIsScheduled(root, now()); - return null; - } - - let exitStatus = shouldTimeSlice(root, lanes) - ? renderRootConcurrent(root, lanes) - : // Time slicing is disabled for default updates in this root. - renderRootSync(root, lanes); + let exitStatus = + shouldTimeSlice(root, lanes) && + (disableSchedulerTimeoutInWorkLoop || !didTimeout) + ? renderRootConcurrent(root, lanes) + : renderRootSync(root, lanes); if (exitStatus !== RootIncomplete) { if (exitStatus === RootErrored) { executionContext |= RetryAfterError; @@ -990,16 +985,7 @@ function performSyncWorkOnRoot(root) { flushPassiveEffects(); let lanes = getNextLanes(root, NoLanes); - if (includesSomeLane(lanes, SyncLane)) { - if ( - root === workInProgressRoot && - includesSomeLane(lanes, workInProgressRootRenderLanes) - ) { - // There's a partial tree, and at least one of its lanes has expired. Finish - // rendering it before rendering the rest of the expired work. - lanes = workInProgressRootRenderLanes; - } - } else { + if (!includesSomeLane(lanes, SyncLane)) { // There's no remaining sync work left. ensureRootIsScheduled(root, now()); return null; @@ -1052,11 +1038,9 @@ function performSyncWorkOnRoot(root) { return null; } -// TODO: Do we still need this API? I think we can delete it. Was only used -// internally. export function flushRoot(root: FiberRoot, lanes: Lanes) { if (lanes !== NoLanes) { - markRootExpired(root, lanes); + markRootEntangled(root, mergeLanes(lanes, SyncLane)); ensureRootIsScheduled(root, now()); if ((executionContext & (RenderContext | CommitContext)) === NoContext) { resetRenderTimer(); diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 08f5cbc2d0e9a..6d7d38232e347 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -228,6 +228,7 @@ type BaseFiberRootProperties = {| pendingLanes: Lanes, suspendedLanes: Lanes, pingedLanes: Lanes, + expiredLanes: Lanes, mutableReadLanes: Lanes, finishedLanes: Lanes, diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js index 4c89c341c649f..d6e082f13ac97 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js @@ -15,6 +15,8 @@ let Scheduler; let readText; let resolveText; let startTransition; +let useState; +let useEffect; describe('ReactExpiration', () => { beforeEach(() => { @@ -24,6 +26,8 @@ describe('ReactExpiration', () => { ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); startTransition = React.unstable_startTransition; + useState = React.useState; + useEffect = React.useEffect; const textCache = new Map(); @@ -478,9 +482,7 @@ describe('ReactExpiration', () => { }); // @gate experimental || !enableSyncDefaultUpdates - it('prevents starvation by sync updates', async () => { - const {useState} = React; - + it('prevents starvation by sync updates by disabling time slicing if too much time has elapsed', async () => { let updateSyncPri; let updateNormalPri; function App() { @@ -519,15 +521,17 @@ describe('ReactExpiration', () => { } expect(Scheduler).toFlushAndYieldThrough(['Sync pri: 0']); updateSyncPri(); + expect(Scheduler).toHaveYielded(['Sync pri: 1', 'Normal pri: 0']); + + // The remaining work hasn't expired, so the render phase is time sliced. + // In other words, we can flush just the first child without flushing + // the rest. + Scheduler.unstable_flushNumberOfYields(1); + // Yield right after first child. + expect(Scheduler).toHaveYielded(['Sync pri: 1']); + // Now do the rest. + expect(Scheduler).toFlushAndYield(['Normal pri: 1']); }); - expect(Scheduler).toHaveYielded([ - // Interrupt high pri update to render sync update - 'Sync pri: 1', - 'Normal pri: 0', - // Now render normal pri - 'Sync pri: 1', - 'Normal pri: 1', - ]); expect(root).toMatchRenderedOutput('Sync pri: 1, Normal pri: 1'); // Do the same thing, but starve the first update @@ -547,16 +551,14 @@ describe('ReactExpiration', () => { // starvation of normal priority updates.) Scheduler.unstable_advanceTime(10000); - // So when we get a high pri update, we shouldn't interrupt updateSyncPri(); + expect(Scheduler).toHaveYielded(['Sync pri: 2', 'Normal pri: 1']); + + // The remaining work _has_ expired, so the render phase is _not_ time + // sliced. Attempting to flush just the first child also flushes the rest. + Scheduler.unstable_flushNumberOfYields(1); + expect(Scheduler).toHaveYielded(['Sync pri: 2', 'Normal pri: 2']); }); - expect(Scheduler).toHaveYielded([ - // Finish normal pri update - 'Normal pri: 2', - // Then do high pri update - 'Sync pri: 2', - 'Normal pri: 2', - ]); expect(root).toMatchRenderedOutput('Sync pri: 2, Normal pri: 2'); }); @@ -628,24 +630,19 @@ describe('ReactExpiration', () => { expect(root).toMatchRenderedOutput('Sync pri: 2, Idle pri: 2'); }); - // @gate experimental - it('a single update can expire without forcing all other updates to expire', async () => { - const {useState} = React; - - let updateHighPri; - let updateNormalPri; + it('when multiple lanes expire, we can finish the in-progress one without including the others', async () => { + let setA; + let setB; function App() { - const [highPri, setHighPri] = useState(0); - const [normalPri, setNormalPri] = useState(0); - updateHighPri = () => ReactNoop.flushSync(() => setHighPri(n => n + 1)); - updateNormalPri = () => setNormalPri(n => n + 1); + const [a, _setA] = useState(0); + const [b, _setB] = useState(0); + setA = _setA; + setB = _setB; return ( <> - - {', '} - - {', '} - + + + ); } @@ -654,184 +651,166 @@ describe('ReactExpiration', () => { await ReactNoop.act(async () => { root.render(); }); - expect(Scheduler).toHaveYielded([ - 'High pri: 0', - 'Normal pri: 0', - 'Sibling', - ]); - expect(root).toMatchRenderedOutput('High pri: 0, Normal pri: 0, Sibling'); + expect(Scheduler).toHaveYielded(['A0', 'B0', 'C']); + expect(root).toMatchRenderedOutput('A0B0C'); await ReactNoop.act(async () => { - // Partially render an update startTransition(() => { - updateNormalPri(); + setA(1); }); - expect(Scheduler).toFlushAndYieldThrough(['High pri: 0']); - - // Some time goes by. Schedule another update. - // This will be placed into a separate batch. - Scheduler.unstable_advanceTime(4000); - + expect(Scheduler).toFlushAndYieldThrough(['A1']); startTransition(() => { - updateNormalPri(); + setB(1); }); - // Keep rendering the first update - expect(Scheduler).toFlushAndYieldThrough(['Normal pri: 1']); - // More time goes by. Enough to expire the first batch, but not the - // second one. - Scheduler.unstable_advanceTime(1000); - // Attempt to interrupt with a high pri update. - await ReactNoop.act(async () => { - updateHighPri(); - }); - - expect(Scheduler).toHaveYielded([ - // The first update expired - 'Sibling', - // Then render the high pri update - 'High pri: 1', - 'Normal pri: 1', - 'Sibling', - // Then the second normal pri update - 'High pri: 1', - 'Normal pri: 2', - 'Sibling', - ]); + // Expire both the transitions + Scheduler.unstable_advanceTime(10000); + // Both transitions have expired, but since they aren't related + // (entangled), we should be able to finish the in-progress transition + // without also including the next one. + Scheduler.unstable_flushNumberOfYields(1); + expect(Scheduler).toHaveYielded(['B0', 'C']); + expect(root).toMatchRenderedOutput('A1B0C'); + + // The next transition also finishes without yielding. + Scheduler.unstable_flushNumberOfYields(1); + expect(Scheduler).toHaveYielded(['A1', 'B1', 'C']); + expect(root).toMatchRenderedOutput('A1B1C'); }); }); // @gate experimental || !enableSyncDefaultUpdates - it('detects starvation in multiple batches', async () => { - const {useState} = React; + it('updates do not expire while they are IO-bound', async () => { + const {Suspense} = React; - let updateHighPri; - let updateNormalPri; - function App() { - const [highPri, setHighPri] = useState(0); - const [normalPri, setNormalPri] = useState(0); - updateHighPri = () => { - ReactNoop.flushSync(() => { - setHighPri(n => n + 1); - }); - }; - updateNormalPri = () => setNormalPri(n => n + 1); + function App({step}) { return ( - <> - - {', '} - - {', '} - - + }> + + + + ); } const root = ReactNoop.createRoot(); await ReactNoop.act(async () => { - root.render(); + await resolveText('A0'); + root.render(); }); - expect(Scheduler).toHaveYielded([ - 'High pri: 0', - 'Normal pri: 0', - 'Sibling', - ]); - expect(root).toMatchRenderedOutput('High pri: 0, Normal pri: 0, Sibling'); + expect(Scheduler).toHaveYielded(['A0', 'B', 'C']); + expect(root).toMatchRenderedOutput('A0BC'); await ReactNoop.act(async () => { - // Partially render an update if (gate(flags => flags.enableSyncDefaultUpdates)) { React.unstable_startTransition(() => { - updateNormalPri(); + root.render(); }); } else { - updateNormalPri(); + root.render(); } - expect(Scheduler).toFlushAndYieldThrough(['High pri: 0']); - // Some time goes by. In an interleaved event, schedule another update. - // This will be placed into a separate batch. - Scheduler.unstable_advanceTime(4000); - updateNormalPri(); - // Keep rendering the first update - expect(Scheduler).toFlushAndYieldThrough(['Normal pri: 1']); - // More time goes by. This expires both of the updates just scheduled. + expect(Scheduler).toFlushAndYield([ + 'Suspend! [A1]', + 'B', + 'C', + 'Loading...', + ]); + + // Lots of time elapses before the promise resolves Scheduler.unstable_advanceTime(10000); - expect(Scheduler).toHaveYielded([]); + await resolveText('A1'); + expect(Scheduler).toHaveYielded(['Promise resolved [A1]']); - // Attempt to interrupt with a high pri update. - updateHighPri(); - - // Both normal pri updates should have expired. - // The sync update and the expired normal pri updates render in a - // single batch. - expect(Scheduler).toHaveYielded([ - 'Sibling', - 'High pri: 1', - 'Normal pri: 2', - 'Sibling', - ]); + // But the update doesn't expire, because it was IO bound. So we can + // partially rendering without finishing. + expect(Scheduler).toFlushAndYieldThrough(['A1']); + expect(root).toMatchRenderedOutput('A0BC'); + + // Lots more time elapses. We're CPU-bound now, so we should treat this + // as starvation. + Scheduler.unstable_advanceTime(10000); + + // The rest of the update finishes without yielding. + Scheduler.unstable_flushNumberOfYields(1); + expect(Scheduler).toHaveYielded(['B', 'C']); }); }); - // @gate experimental || !enableSyncDefaultUpdates - it('updates do not expire while they are IO-bound', async () => { - const {Suspense} = React; - - function App({text}) { + it('flushSync should not affect expired work', async () => { + let setA; + let setB; + function App() { + const [a, _setA] = useState(0); + const [b, _setB] = useState(0); + setA = _setA; + setB = _setB; return ( - }> - - {', '} - - + <> + + + ); } const root = ReactNoop.createRoot(); await ReactNoop.act(async () => { - await resolveText('A'); - root.render(); + root.render(); }); - expect(Scheduler).toHaveYielded(['A', 'Sibling']); - expect(root).toMatchRenderedOutput('A, Sibling'); + expect(Scheduler).toHaveYielded(['A0', 'B0']); await ReactNoop.act(async () => { - if (gate(flags => flags.enableSyncDefaultUpdates)) { - React.unstable_startTransition(() => { - root.render(); - }); - } else { - root.render(); - } - expect(Scheduler).toFlushAndYield([ - 'Suspend! [B]', - 'Sibling', - 'Loading...', - ]); + startTransition(() => { + setA(1); + }); + expect(Scheduler).toFlushAndYieldThrough(['A1']); - // Lots of time elapses before the promise resolves + // Expire the in-progress update Scheduler.unstable_advanceTime(10000); - await resolveText('B'); - expect(Scheduler).toHaveYielded(['Promise resolved [B]']); - // But the update doesn't expire, because it was IO bound. So we can - // partially rendering without finishing. - expect(Scheduler).toFlushAndYieldThrough(['B']); - expect(root).toMatchRenderedOutput('A, Sibling'); + ReactNoop.flushSync(() => { + setB(1); + }); + expect(Scheduler).toHaveYielded(['A0', 'B1']); - // Lots more time elapses. We're CPU-bound now, so we should treat this - // as starvation. + // Now flush the original update. Because it expired, it should finish + // without yielding. + Scheduler.unstable_flushNumberOfYields(1); + expect(Scheduler).toHaveYielded(['A1', 'B1']); + }); + }); + + it('passive effects of expired update flush after paint', async () => { + function App({step}) { + useEffect(() => { + Scheduler.unstable_yieldValue('Effect: ' + step); + }, [step]); + return ( + <> + + + + + ); + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['A0', 'B0', 'C0', 'Effect: 0']); + expect(root).toMatchRenderedOutput('A0B0C0'); + + await ReactNoop.act(async () => { + startTransition(() => { + root.render(); + }); + // Expire the update Scheduler.unstable_advanceTime(10000); - // Attempt to interrupt with a sync update. - ReactNoop.flushSync(() => root.render()); - expect(Scheduler).toHaveYielded([ - // Because the previous update had already expired, we don't interrupt - // it. Finish rendering it first. - 'Sibling', - // Then do the sync update. - 'A', - 'Sibling', - ]); + // The update finishes without yielding. But it does not flush the effect. + Scheduler.unstable_flushNumberOfYields(1); + expect(Scheduler).toHaveYielded(['A1', 'B1', 'C1']); }); + // The effect flushes after paint. + expect(Scheduler).toHaveYielded(['Effect: 1']); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js index d0fb1fec858c4..3c09935a5b7cc 100644 --- a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js @@ -300,10 +300,9 @@ describe( ReactNoop.render(); }); - ReactNoop.flushSync(); - // Because the render expired, React should finish the tree without // consulting `shouldYield` again + Scheduler.unstable_flushNumberOfYields(1); expect(Scheduler).toHaveYielded(['B', 'C']); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index 7861b2904f28d..668299b282b9b 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -1958,32 +1958,13 @@ describe('ReactSuspenseWithNoopRenderer', () => { await advanceTimers(5000); // Retry with the new content. - if (gate(flags => flags.disableSchedulerTimeoutInWorkLoop)) { - expect(Scheduler).toFlushAndYield([ - 'A', - // B still suspends - 'Suspend! [B]', - 'Loading more...', - ]); - } else { - // In this branch, right as we start rendering, we detect that the work - // has expired (via Scheduler's didTimeout argument) and re-schedule the - // work as synchronous. Since sync work does not flow through Scheduler, - // we need to use `flushSync`. - // - // Usually we would use `act`, which fluses both sync work and Scheduler - // work, but that would also force the fallback to display, and this test - // is specifically about whether we delay or show the fallback. - expect(Scheduler).toFlushAndYield([]); - // This will flush the synchronous callback we just scheduled. - ReactNoop.flushSync(); - expect(Scheduler).toHaveYielded([ - 'A', - // B still suspends - 'Suspend! [B]', - 'Loading more...', - ]); - } + expect(Scheduler).toFlushAndYield([ + 'A', + // B still suspends + 'Suspend! [B]', + 'Loading more...', + ]); + // Because we've already been waiting for so long we've exceeded // our threshold and we show the next level immediately. expect(ReactNoop.getChildren()).toEqual([