Don't rely on didTimeout for SyncBatched (facebook#19469)

acdlite · web-flow · commit 722bc046dcd7 · 2020-07-27T14:42:44.000-07:00
Tasks with SyncBatchedPriority — used by Blocking Mode — should always
be rendered by the `peformSyncWorkOnRoot` path, not
`performConcurrentWorkOnRoot`.

Currently, they go through the `performConcurrentWorkOnRoot` callback.
Then, we check `didTimeout` to see if the task expired. Since
SyncBatchedPriority translates to ImmediatePriority in the Scheduler,
`didTimeout` is always `true`, so we mark it as expired. Then it exits
and re-enters in the `performSyncWorkOnRoot` path.

Aside from being overly convoluted, we shouldn't rely on Scheduler to
tell us that SyncBatchedPriority work is synchronous. We should handle
that ourselves.

This will allow us to remove the `didTimeout` check. And it further
decouples us from the Scheduler priority, so we can eventually remove
that, too.
diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js
@@ -44,7 +44,7 @@ import {
 } from './SchedulerWithReactIntegration.new';
 
 export const SyncLanePriority: LanePriority = 17;
-const SyncBatchedLanePriority: LanePriority = 16;
+export const SyncBatchedLanePriority: LanePriority = 16;
 
 const InputDiscreteHydrationLanePriority: LanePriority = 15;
 export const InputDiscreteLanePriority: LanePriority = 14;
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -144,6 +144,7 @@ import {
 import {
   NoLanePriority,
   SyncLanePriority,
+  SyncBatchedLanePriority,
   InputDiscreteLanePriority,
   TransitionShortLanePriority,
   TransitionLongLanePriority,
@@ -726,6 +727,11 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
     newCallbackNode = scheduleSyncCallback(
       performSyncWorkOnRoot.bind(null, root),
     );
+  } else if (newCallbackPriority === SyncBatchedLanePriority) {
+    newCallbackNode = scheduleCallback(
+      ImmediateSchedulerPriority,
+      performSyncWorkOnRoot.bind(null, root),
+    );
   } else {
     const schedulerPriorityLevel = lanePriorityToSchedulerPriority(
       newCallbackPriority,
@@ -756,7 +762,20 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
 
   // Flush any pending passive effects before deciding which lanes to work on,
   // in case they schedule additional work.
-  flushPassiveEffects();
+  const originalCallbackNode = root.callbackNode;
+  const didFlushPassiveEffects = flushPassiveEffects();
+  if (didFlushPassiveEffects) {
+    // Something in the passive effect phase may have canceled the current task.
+    // Check if the task node for this root was changed.
+    if (root.callbackNode !== originalCallbackNode) {
+      // The current task was canceled. Exit. We don't need to call
+      // `ensureRootIsScheduled` because the check above implies either that
+      // there's a new task, or that there's no remaining work on this root.
+      return null;
+    } else {
+      // Current task was not canceled. Continue.
+    }
+  }
 
   // Determine the next expiration time to work on, using the fields stored
   // on the root.
@@ -765,6 +784,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
     root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
   );
   if (lanes === NoLanes) {
+    // Defensive coding. This is never expected to happen.
     return null;
   }
 
@@ -781,8 +801,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
     return null;
   }
 
-  const originalCallbackNode = root.callbackNode;
-
   let exitStatus = renderRootConcurrent(root, lanes);
 
   if (
@@ -2593,7 +2611,8 @@ function commitLayoutEffectsImpl(
   resetCurrentDebugFiberInDEV();
 }
 
-export function flushPassiveEffects() {
+export function flushPassiveEffects(): boolean {
+  // Returns whether passive effects were flushed.
   if (pendingPassiveEffectsRenderPriority !== NoSchedulerPriority) {
     const priorityLevel =
       pendingPassiveEffectsRenderPriority > NormalSchedulerPriority
@@ -2610,6 +2629,7 @@ export function flushPassiveEffects() {
       setCurrentUpdateLanePriority(previousLanePriority);
     }
   }
+  return false;
 }
 
 export function enqueuePendingPassiveProfilerEffect(fiber: Fiber): void {
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -136,6 +136,7 @@ import {
 import {
   NoLanePriority,
   SyncLanePriority,
+  SyncBatchedLanePriority,
   InputDiscreteLanePriority,
   TransitionShortLanePriority,
   TransitionLongLanePriority,
@@ -719,6 +720,11 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
     newCallbackNode = scheduleSyncCallback(
       performSyncWorkOnRoot.bind(null, root),
     );
+  } else if (newCallbackPriority === SyncBatchedLanePriority) {
+    newCallbackNode = scheduleCallback(
+      ImmediateSchedulerPriority,
+      performSyncWorkOnRoot.bind(null, root),
+    );
   } else {
     const schedulerPriorityLevel = lanePriorityToSchedulerPriority(
       newCallbackPriority,
@@ -749,7 +755,20 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
 
   // Flush any pending passive effects before deciding which lanes to work on,
   // in case they schedule additional work.
-  flushPassiveEffects();
+  const originalCallbackNode = root.callbackNode;
+  const didFlushPassiveEffects = flushPassiveEffects();
+  if (didFlushPassiveEffects) {
+    // Something in the passive effect phase may have canceled the current task.
+    // Check if the task node for this root was changed.
+    if (root.callbackNode !== originalCallbackNode) {
+      // The current task was canceled. Exit. We don't need to call
+      // `ensureRootIsScheduled` because the check above implies either that
+      // there's a new task, or that there's no remaining work on this root.
+      return null;
+    } else {
+      // Current task was not canceled. Continue.
+    }
+  }
 
   // Determine the next expiration time to work on, using the fields stored
   // on the root.
@@ -758,6 +777,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
     root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
   );
   if (lanes === NoLanes) {
+    // Defensive coding. This is never expected to happen.
     return null;
   }
 
@@ -774,8 +794,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
     return null;
   }
 
-  const originalCallbackNode = root.callbackNode;
-
   let exitStatus = renderRootConcurrent(root, lanes);
 
   if (
@@ -2437,7 +2455,8 @@ function commitLayoutEffects(root: FiberRoot, committedLanes: Lanes) {
   }
 }
 
-export function flushPassiveEffects() {
+export function flushPassiveEffects(): boolean {
+  // Returns whether passive effects were flushed.
   if (pendingPassiveEffectsRenderPriority !== NoSchedulerPriority) {
     const priorityLevel =
       pendingPassiveEffectsRenderPriority > NormalSchedulerPriority
@@ -2454,6 +2473,7 @@ export function flushPassiveEffects() {
       setCurrentUpdateLanePriority(previousLanePriority);
     }
   }
+  return false;
 }
 
 export function enqueuePendingPassiveProfilerEffect(fiber: Fiber): void {
