From 18e7d91b283328bbf078392cb5c19dff94977122 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Oct 2023 22:00:52 -0400 Subject: [PATCH 1/2] Clarify desired behavior of test case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I updated one of the tests related to synchronous popstate transitions to clarify what the desired behavior is. It's really about what happens if a popstate transition suspends. It should not cause an error, and the transition should be allowed to finish once the promise resolves. Ideally, if the transition suspends, it should completely revert back to the default behavior for transitions — i.e. it should no longer be treated as synchronous. Currently we don't do that, and when the promise resolve the transition will still be rendered synchronously, but we can improve this later. --- .../src/__tests__/ReactDOMFiberAsync-test.js | 94 ++++++++++++++----- 1 file changed, 68 insertions(+), 26 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index 79bf34160534..145949bf7ce6 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js @@ -16,6 +16,8 @@ let ReactDOMClient; let Scheduler; let act; let waitForAll; +let waitFor; +let waitForMicrotasks; let assertLog; const setUntrackedInputValue = Object.getOwnPropertyDescriptor( @@ -36,6 +38,8 @@ describe('ReactDOMFiberAsync', () => { const InternalTestUtils = require('internal-test-utils'); waitForAll = InternalTestUtils.waitForAll; + waitFor = InternalTestUtils.waitFor; + waitForMicrotasks = InternalTestUtils.waitForMicrotasks; assertLog = InternalTestUtils.assertLog; document.body.appendChild(container); @@ -653,52 +657,90 @@ describe('ReactDOMFiberAsync', () => { }); }); - it('transition lane in popState should yield if it suspends', async () => { - const never = {then() {}}; - let _setText; + it('transition lane in popState should be allowed to suspend', async () => { + let resolvePromise; + const promise = new Promise(res => { + resolvePromise = res; + }); + + function Text({text}) { + Scheduler.log(text); + return text; + } function App() { - const [shouldSuspend, setShouldSuspend] = React.useState(false); - const [text, setText] = React.useState('0'); - _setText = setText; - if (shouldSuspend) { - Scheduler.log('Suspend!'); - throw never; - } - function onPopstate() { - React.startTransition(() => { - setShouldSuspend(val => !val); - }); + const [pathname, setPathname] = React.useState('/path/a'); + + if (pathname !== '/path/a') { + try { + React.use(promise); + } catch (e) { + Scheduler.log(`Suspend! [${pathname}]`); + throw e; + } } + React.useEffect(() => { + function onPopstate() { + React.startTransition(() => { + setPathname('/path/b'); + }); + } window.addEventListener('popstate', onPopstate); return () => window.removeEventListener('popstate', onPopstate); }, []); - Scheduler.log(`Child:${shouldSuspend}/${text}`); - return text; + + return ( + <> + +
+ +
+ + + ); } const root = ReactDOMClient.createRoot(container); await act(async () => { root.render(); }); - assertLog(['Child:false/0']); + assertLog(['Before', '/path/a', 'After']); - await act(() => { + const div = container.getElementsByTagName('div')[0]; + expect(div.textContent).toBe('/path/a'); + + // Simulate a popstate event + await act(async () => { const popStateEvent = new Event('popstate'); + + // Simulate a popstate event window.event = popStateEvent; window.dispatchEvent(popStateEvent); - queueMicrotask(() => { - window.event = undefined; - }); + await waitForMicrotasks(); + window.event = undefined; + + // The transition lane should have been attempted synchronously (in + // a microtask) + assertLog(['Suspend! [/path/b]']); + // Because it suspended, it remains on the current path + expect(div.textContent).toBe('/path/a'); }); - assertLog(['Suspend!']); await act(async () => { - _setText('1'); + resolvePromise(); + + // TODO: Since the transition previously suspended, there's no need for + // this transition to be rendered synchronously on susbequent attempts; + // if we fail to commit synchronously the first time, the scroll + // restoration state won't be restored anyway. We can improve this later. + // + // Once this is implemented, update this test to yield in between each + // child to prove that it's concurrent. + await waitForMicrotasks(); + assertLog(['Before', '/path/b', 'After']); }); - assertLog(['Child:false/1', 'Suspend!']); - - root.unmount(); + assertLog([]); + expect(div.textContent).toBe('/path/b'); }); }); From 717595f8c22d30f492e97d619cd8b79bdab13627 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Oct 2023 22:22:35 -0400 Subject: [PATCH 2/2] Track entangled lanes separately from update lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A small refactor to how the lane entanglement mechanism works. We can now distinguish between the lane that "spawned" a render task (i.e. a new update) versus the lanes that it's entangled with. Both the update lane and the entangled lanes will be included while rendering, but by keeping them separate, we don't lose the original priority. In practical terms, this means we can now entangle a low priority update with a higher priority lane while rendering at the lower priority. To do this, lanes that are entangled at the root are now tracked using the same variable that we use to track the "base lanes" when revealing a previously hidden tree — conceptually, they are the same thing. I also renamed this variable (from subtreeLanes to entangledRenderLanes) to better reflect how it's used. My primary motivation is related to useDeferredValue, which I'll address in a later PR. --- .../src/__tests__/ReactDOMFiberAsync-test.js | 18 ++++--- .../src/ReactFiberHiddenContext.js | 21 +++++--- .../react-reconciler/src/ReactFiberHooks.js | 3 +- .../react-reconciler/src/ReactFiberLane.js | 50 ++++++++++++++++--- .../src/ReactFiberRootScheduler.js | 8 +-- .../src/ReactFiberWorkLoop.js | 42 ++++++++++------ 6 files changed, 100 insertions(+), 42 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index 145949bf7ce6..b8b8847a9dff 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js @@ -726,19 +726,23 @@ describe('ReactDOMFiberAsync', () => { // Because it suspended, it remains on the current path expect(div.textContent).toBe('/path/a'); }); + assertLog(['Suspend! [/path/b]']); await act(async () => { resolvePromise(); - // TODO: Since the transition previously suspended, there's no need for - // this transition to be rendered synchronously on susbequent attempts; - // if we fail to commit synchronously the first time, the scroll - // restoration state won't be restored anyway. We can improve this later. + // Since the transition previously suspended, there's no need for this + // transition to be rendered synchronously on susbequent attempts; if we + // fail to commit synchronously the first time, the scroll restoration + // state won't be restored anyway. // - // Once this is implemented, update this test to yield in between each - // child to prove that it's concurrent. + // Yield in between each child to prove that it's concurrent. await waitForMicrotasks(); - assertLog(['Before', '/path/b', 'After']); + assertLog([]); + + await waitFor(['Before']); + await waitFor(['/path/b']); + await waitFor(['After']); }); assertLog([]); expect(div.textContent).toBe('/path/b'); diff --git a/packages/react-reconciler/src/ReactFiberHiddenContext.js b/packages/react-reconciler/src/ReactFiberHiddenContext.js index 4b94760eded3..def05ba91ddc 100644 --- a/packages/react-reconciler/src/ReactFiberHiddenContext.js +++ b/packages/react-reconciler/src/ReactFiberHiddenContext.js @@ -13,7 +13,10 @@ import type {Lanes} from './ReactFiberLane'; import {createCursor, push, pop} from './ReactFiberStack'; -import {getRenderLanes, setRenderLanes} from './ReactFiberWorkLoop'; +import { + getEntangledRenderLanes, + setEntangledRenderLanes, +} from './ReactFiberWorkLoop'; import {NoLanes, mergeLanes} from './ReactFiberLane'; // TODO: Remove `renderLanes` context in favor of hidden context @@ -29,26 +32,28 @@ type HiddenContext = { // InvisibleParentContext that is currently managed by SuspenseContext. export const currentTreeHiddenStackCursor: StackCursor = createCursor(null); -export const prevRenderLanesStackCursor: StackCursor = +export const prevEntangledRenderLanesCursor: StackCursor = createCursor(NoLanes); export function pushHiddenContext(fiber: Fiber, context: HiddenContext): void { - const prevRenderLanes = getRenderLanes(); - push(prevRenderLanesStackCursor, prevRenderLanes, fiber); + const prevEntangledRenderLanes = getEntangledRenderLanes(); + push(prevEntangledRenderLanesCursor, prevEntangledRenderLanes, fiber); push(currentTreeHiddenStackCursor, context, fiber); // When rendering a subtree that's currently hidden, we must include all // lanes that would have rendered if the hidden subtree hadn't been deferred. // That is, in order to reveal content from hidden -> visible, we must commit // all the updates that we skipped when we originally hid the tree. - setRenderLanes(mergeLanes(prevRenderLanes, context.baseLanes)); + setEntangledRenderLanes( + mergeLanes(prevEntangledRenderLanes, context.baseLanes), + ); } export function reuseHiddenContextOnStack(fiber: Fiber): void { // This subtree is not currently hidden, so we don't need to add any lanes // to the render lanes. But we still need to push something to avoid a // context mismatch. Reuse the existing context on the stack. - push(prevRenderLanesStackCursor, getRenderLanes(), fiber); + push(prevEntangledRenderLanesCursor, getEntangledRenderLanes(), fiber); push( currentTreeHiddenStackCursor, currentTreeHiddenStackCursor.current, @@ -58,10 +63,10 @@ export function reuseHiddenContextOnStack(fiber: Fiber): void { export function popHiddenContext(fiber: Fiber): void { // Restore the previous render lanes from the stack - setRenderLanes(prevRenderLanesStackCursor.current); + setEntangledRenderLanes(prevEntangledRenderLanesCursor.current); pop(currentTreeHiddenStackCursor, fiber); - pop(prevRenderLanesStackCursor, fiber); + pop(prevEntangledRenderLanesCursor, fiber); } export function isCurrentTreeHidden(): boolean { diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index e0f5d268bcf5..d2c456a5f9c9 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1525,7 +1525,8 @@ function mountSyncExternalStore( ); } - if (!includesBlockingLane(root, renderLanes)) { + const rootRenderLanes = getWorkInProgressRootRenderLanes(); + if (!includesBlockingLane(root, rootRenderLanes)) { pushStoreConsistencyCheck(fiber, getSnapshot, nextSnapshot); } } diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js index 22d8377face0..21055ac82f7e 100644 --- a/packages/react-reconciler/src/ReactFiberLane.js +++ b/packages/react-reconciler/src/ReactFiberLane.js @@ -39,6 +39,7 @@ export const NoLane: Lane = /* */ 0b0000000000000000000 export const SyncHydrationLane: Lane = /* */ 0b0000000000000000000000000000001; export const SyncLane: Lane = /* */ 0b0000000000000000000000000000010; +export const SyncLaneIndex: number = 1; export const InputContinuousHydrationLane: Lane = /* */ 0b0000000000000000000000000000100; export const InputContinuousLane: Lane = /* */ 0b0000000000000000000000000001000; @@ -274,17 +275,23 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes { } } + return nextLanes; +} + +export function getEntangledLanes(root: FiberRoot, renderLanes: Lanes): Lanes { + let entangledLanes = renderLanes; + if ( allowConcurrentByDefault && (root.current.mode & ConcurrentUpdatesByDefaultMode) !== NoMode ) { // Do nothing, use the lanes as they were assigned. - } else if ((nextLanes & InputContinuousLane) !== NoLanes) { + } else if ((entangledLanes & InputContinuousLane) !== NoLanes) { // When updates are sync by default, we entangle continuous priority updates // and default updates, so they render in the same batch. The only reason // they use separate lanes is because continuous updates should interrupt // transitions, but default updates should not. - nextLanes |= pendingLanes & DefaultLane; + entangledLanes |= entangledLanes & DefaultLane; } // Check for entangled lanes and add them to the batch. @@ -309,21 +316,21 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes { // For those exceptions where entanglement is semantically important, // we should ensure that there is no partial work at the // time we apply the entanglement. - const entangledLanes = root.entangledLanes; - if (entangledLanes !== NoLanes) { + const allEntangledLanes = root.entangledLanes; + if (allEntangledLanes !== NoLanes) { const entanglements = root.entanglements; - let lanes = nextLanes & entangledLanes; + let lanes = entangledLanes & allEntangledLanes; while (lanes > 0) { const index = pickArbitraryLaneIndex(lanes); const lane = 1 << index; - nextLanes |= entanglements[index]; + entangledLanes |= entanglements[index]; lanes &= ~lane; } } - return nextLanes; + return entangledLanes; } function computeExpirationTime(lane: Lane, currentTime: number) { @@ -404,6 +411,7 @@ export function markStarvedLanesAsExpired( // Iterate through the pending lanes and check if we've reached their // expiration time. If so, we'll assume the update is being starved and mark // it as expired to force it to finish. + // TODO: We should be able to replace this with upgradePendingLanesToSync // // We exclude retry lanes because those must always be time sliced, in order // to unwrap uncached promises. @@ -708,6 +716,34 @@ export function markRootEntangled(root: FiberRoot, entangledLanes: Lanes) { } } +export function upgradePendingLaneToSync(root: FiberRoot, lane: Lane) { + // Since we're upgrading the priority of the given lane, there is now pending + // sync work. + root.pendingLanes |= SyncLane; + + // Entangle the sync lane with the lane we're upgrading. This means SyncLane + // will not be allowed to finish without also finishing the given lane. + root.entangledLanes |= SyncLane; + root.entanglements[SyncLaneIndex] |= lane; +} + +export function upgradePendingLanesToSync( + root: FiberRoot, + lanesToUpgrade: Lanes, +) { + // Same as upgradePendingLaneToSync but accepts multiple lanes, so it's a + // bit slower. + root.pendingLanes |= SyncLane; + root.entangledLanes |= SyncLane; + let lanes = lanesToUpgrade; + while (lanes) { + const index = pickArbitraryLaneIndex(lanes); + const lane = 1 << index; + root.entanglements[SyncLaneIndex] |= lane; + lanes &= ~lane; + } +} + export function markHiddenUpdate( root: FiberRoot, update: ConcurrentUpdate, diff --git a/packages/react-reconciler/src/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index a8542d0a0ea0..19294e7d4880 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -20,8 +20,7 @@ import { getNextLanes, includesSyncLane, markStarvedLanesAsExpired, - markRootEntangled, - mergeLanes, + upgradePendingLaneToSync, claimNextTransitionLane, } from './ReactFiberLane'; import { @@ -250,7 +249,10 @@ function processRootScheduleInMicrotask() { currentEventTransitionLane !== NoLane && shouldAttemptEagerTransition() ) { - markRootEntangled(root, mergeLanes(currentEventTransitionLane, SyncLane)); + // A transition was scheduled during an event, but we're going to try to + // render it synchronously anyway. We do this during a popstate event to + // preserve the scroll position of the previous page. + upgradePendingLaneToSync(root, currentEventTransitionLane); } const nextLanes = scheduleTaskForRootDuringMicrotask(root, currentTime); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index b3953369803b..cb217d04eba1 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -141,11 +141,12 @@ import { includesBlockingLane, includesExpiredLane, getNextLanes, + getEntangledLanes, getLanesToRetrySynchronouslyOnError, markRootUpdated, markRootSuspended as markRootSuspended_dontCallThisOneDirectly, markRootPinged, - markRootEntangled, + upgradePendingLanesToSync, markRootFinished, addFiberToLanesMap, movePendingFibersToMemoized, @@ -349,8 +350,8 @@ let workInProgressRootDidAttachPingListener: boolean = false; // HiddenContext module. // // Most things in the work loop should deal with workInProgressRootRenderLanes. -// Most things in begin/complete phases should deal with renderLanes. -export let renderLanes: Lanes = NoLanes; +// Most things in begin/complete phases should deal with entangledRenderLanes. +export let entangledRenderLanes: Lanes = NoLanes; // Whether to root completed, errored, suspended, etc. let workInProgressRootExitStatus: RootExitStatus = RootInProgress; @@ -1335,7 +1336,7 @@ export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null { export function flushRoot(root: FiberRoot, lanes: Lanes) { if (lanes !== NoLanes) { - markRootEntangled(root, mergeLanes(lanes, SyncLane)); + upgradePendingLanesToSync(root, lanes); ensureRootIsScheduled(root); if ((executionContext & (RenderContext | CommitContext)) === NoContext) { resetRenderTimer(); @@ -1471,12 +1472,12 @@ export function isInvalidExecutionContextForEventFunction(): boolean { // hidden subtree. The stack logic is managed there because that's the only // place that ever modifies it. Which module it lives in doesn't matter for // performance because this function will get inlined regardless -export function setRenderLanes(subtreeRenderLanes: Lanes) { - renderLanes = subtreeRenderLanes; +export function setEntangledRenderLanes(newEntangledRenderLanes: Lanes) { + entangledRenderLanes = newEntangledRenderLanes; } -export function getRenderLanes(): Lanes { - return renderLanes; +export function getEntangledRenderLanes(): Lanes { + return entangledRenderLanes; } function resetWorkInProgressStack() { @@ -1526,7 +1527,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { workInProgressRoot = root; const rootWorkInProgress = createWorkInProgress(root.current, null); workInProgress = rootWorkInProgress; - workInProgressRootRenderLanes = renderLanes = lanes; + workInProgressRootRenderLanes = lanes; workInProgressSuspendedReason = NotSuspended; workInProgressThrownValue = null; workInProgressRootDidAttachPingListener = false; @@ -1539,6 +1540,15 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { workInProgressRootConcurrentErrors = null; workInProgressRootRecoverableErrors = null; + // Get the lanes that are entangled with whatever we're about to render. We + // track these separately so we can distinguish the priority of the render + // task from the priority of the lanes it is entangled with. For example, a + // transition may not be allowed to finish unless it includes the Sync lane, + // which is currently suspended. We should be able to render the Transition + // and Sync lane in the same batch, but at Transition priority, because the + // Sync lane already suspended. + entangledRenderLanes = getEntangledLanes(root, lanes); + finishQueueingConcurrentUpdates(); if (__DEV__) { @@ -2249,10 +2259,10 @@ function performUnitOfWork(unitOfWork: Fiber): void { let next; if (enableProfilerTimer && (unitOfWork.mode & ProfileMode) !== NoMode) { startProfilerTimer(unitOfWork); - next = beginWork(current, unitOfWork, renderLanes); + next = beginWork(current, unitOfWork, entangledRenderLanes); stopProfilerTimerIfRunningAndRecordDelta(unitOfWork, true); } else { - next = beginWork(current, unitOfWork, renderLanes); + next = beginWork(current, unitOfWork, entangledRenderLanes); } resetCurrentDebugFiberInDEV(); @@ -2359,9 +2369,9 @@ function replaySuspendedUnitOfWork(unitOfWork: Fiber): void { unwindInterruptedWork(current, unitOfWork, workInProgressRootRenderLanes); unitOfWork = workInProgress = resetWorkInProgress( unitOfWork, - renderLanes, + entangledRenderLanes, ); - next = beginWork(current, unitOfWork, renderLanes); + next = beginWork(current, unitOfWork, entangledRenderLanes); break; } } @@ -2471,10 +2481,10 @@ function completeUnitOfWork(unitOfWork: Fiber): void { setCurrentDebugFiberInDEV(completedWork); let next; if (!enableProfilerTimer || (completedWork.mode & ProfileMode) === NoMode) { - next = completeWork(current, completedWork, renderLanes); + next = completeWork(current, completedWork, entangledRenderLanes); } else { startProfilerTimer(completedWork); - next = completeWork(current, completedWork, renderLanes); + next = completeWork(current, completedWork, entangledRenderLanes); // Update render duration assuming we didn't error. stopProfilerTimerIfRunningAndRecordDelta(completedWork, false); } @@ -2516,7 +2526,7 @@ function unwindUnitOfWork(unitOfWork: Fiber): void { // This fiber did not complete because something threw. Pop values off // the stack without entering the complete phase. If this is a boundary, // capture values if possible. - const next = unwindWork(current, incompleteWork, renderLanes); + const next = unwindWork(current, incompleteWork, entangledRenderLanes); // Because this fiber did not complete, don't reset its lanes.