Skip to content

Commit

Permalink
[Fiber] Don't work on scheduled tasks while we're in an async commit …
Browse files Browse the repository at this point in the history
…but flush it eagerly if we're sync (#31987)

This is a follow up to #31930 and a prerequisite for #31975.

With View Transitions, the commit phase becomes async which means that
other work can sneak in between. We need to be resilient to that.

This PR first refactors the flushMutationEffects and flushLayoutEffects
to use module scope variables to track its arguments so we can defer
them. It shares these with how we were already doing it for
flushPendingEffects.

We also track how far along the commit phase we are so we know what we
have left to flush.

Then callers of flushPassiveEffects become flushPendingEffects. That
helper synchronously flushes any remaining phases we've yet to commit.
That ensure that things are at least consistent if that happens.

Finally, when we are using a scheduled task, we don't do any work. This
ensures that we're not flushing any work too early if we could've
deferred it. This still ensures that we always do flush it before
starting any new work on any root so new roots observe the committed
state.

There are some unfortunate effects that could happen from allowing
things to flush eagerly. Such as if a flushSync sneaks in before
startViewTransition, it'll skip the animation. If it's during a
suspensey font it'll start the transition before the font has loaded
which might be better than breaking flushSync. It'll also potentially
flush passive effects inside the startViewTransition which should
typically be ok.
  • Loading branch information
sebmarkbage authored Jan 6, 2025
1 parent 3ce77d5 commit defffdb
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 105 deletions.
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberHotReloading.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import type {ReactNodeList} from 'shared/ReactTypes';
import {
flushSyncWork,
scheduleUpdateOnFiber,
flushPassiveEffects,
flushPendingEffects,
} from './ReactFiberWorkLoop';
import {enqueueConcurrentRenderForLane} from './ReactFiberConcurrentUpdates';
import {updateContainerSync} from './ReactFiberReconciler';
Expand Down Expand Up @@ -229,7 +229,7 @@ export const scheduleRefresh: ScheduleRefresh = (
return;
}
const {staleFamilies, updatedFamilies} = update;
flushPassiveEffects();
flushPendingEffects();
scheduleFibersWithFamiliesRecursively(
root.current,
updatedFamilies,
Expand Down
9 changes: 5 additions & 4 deletions packages/react-reconciler/src/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import isArray from 'shared/isArray';
import {
enableSchedulingProfiler,
enableHydrationLaneScheduling,
disableLegacyMode,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
Expand Down Expand Up @@ -75,7 +76,7 @@ import {
isAlreadyRendering,
deferredUpdates,
discreteUpdates,
flushPassiveEffects,
flushPendingEffects,
} from './ReactFiberWorkLoop';
import {enqueueConcurrentRenderForLane} from './ReactFiberConcurrentUpdates';
import {
Expand Down Expand Up @@ -364,8 +365,8 @@ export function updateContainerSync(
parentComponent: ?React$Component<any, any>,
callback: ?Function,
): Lane {
if (container.tag === LegacyRoot) {
flushPassiveEffects();
if (!disableLegacyMode && container.tag === LegacyRoot) {
flushPendingEffects();
}
const current = container.current;
updateContainerImpl(
Expand Down Expand Up @@ -453,7 +454,7 @@ export {
flushSyncFromReconciler,
flushSyncWork,
isAlreadyRendering,
flushPassiveEffects,
flushPendingEffects as flushPassiveEffects,
};

export function getPublicRootInstance(
Expand Down
20 changes: 17 additions & 3 deletions packages/react-reconciler/src/ReactFiberRootScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,13 @@ import {
CommitContext,
NoContext,
RenderContext,
flushPassiveEffects,
flushPendingEffects,
getExecutionContext,
getWorkInProgressRoot,
getWorkInProgressRootRenderLanes,
getRootWithPendingPassiveEffects,
getPendingPassiveEffectsLanes,
hasPendingCommitEffects,
isWorkLoopSuspendedOnData,
performWorkOnRoot,
} from './ReactFiberWorkLoop';
Expand Down Expand Up @@ -466,10 +467,23 @@ function performWorkOnRootViaSchedulerTask(
trackSchedulerEvent();
}

if (hasPendingCommitEffects()) {
// We are currently in the middle of an async committing (such as a View Transition).
// We could force these to flush eagerly but it's better to defer any work until
// it finishes. This may not be the same root as we're waiting on.
// TODO: This relies on the commit eventually calling ensureRootIsScheduled which
// always calls processRootScheduleInMicrotask which in turn always loops through
// all the roots to figure out. This is all a bit inefficient and if optimized
// it'll need to consider rescheduling a task for any skipped roots.
root.callbackNode = null;
root.callbackPriority = NoLane;
return null;
}

// Flush any pending passive effects before deciding which lanes to work on,
// in case they schedule additional work.
const originalCallbackNode = root.callbackNode;
const didFlushPassiveEffects = flushPassiveEffects();
const didFlushPassiveEffects = flushPendingEffects(true);
if (didFlushPassiveEffects) {
// Something in the passive effect phase may have canceled the current task.
// Check if the task node for this root was changed.
Expand Down Expand Up @@ -534,7 +548,7 @@ function performWorkOnRootViaSchedulerTask(
function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes) {
// This is the entry point for synchronous tasks that don't go
// through Scheduler.
const didFlushPassiveEffects = flushPassiveEffects();
const didFlushPassiveEffects = flushPendingEffects();
if (didFlushPassiveEffects) {
// If passive effects were flushed, exit to the outer work loop in the root
// scheduler, so we can recompute the priority.
Expand Down
Loading

0 comments on commit defffdb

Please sign in to comment.