Don't rely on `didTimeout` for SyncBatched (#19469)
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.
Andrew Clark committed
Jul 27, 2020 at 16:42 UTC
722bc046dcd748dde7109bc959318d3b14cf5196
3 files changed
+49
-9
packages/react-reconciler/src/ReactFiberLane.js
+1
-1
@@ -44,7 +44,7 @@ import {
44
} from './SchedulerWithReactIntegration.new';
45
46
export const SyncLanePriority: LanePriority = 17;
47
-const SyncBatchedLanePriority: LanePriority = 16;
47
+export const SyncBatchedLanePriority: LanePriority = 16;
48
49
const InputDiscreteHydrationLanePriority: LanePriority = 15;
50
export const InputDiscreteLanePriority: LanePriority = 14;
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+24
-4
@@ -144,6 +144,7 @@ import {
144
import {
145
NoLanePriority,
146
SyncLanePriority,
147
+ SyncBatchedLanePriority,
148
InputDiscreteLanePriority,
149
TransitionShortLanePriority,
150
TransitionLongLanePriority,
@@ -726,6 +727,11 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
727
newCallbackNode = scheduleSyncCallback(
728
performSyncWorkOnRoot.bind(null, root),
729
);
730
+ } else if (newCallbackPriority === SyncBatchedLanePriority) {
731
+ newCallbackNode = scheduleCallback(
732
+ ImmediateSchedulerPriority,
733
+ performSyncWorkOnRoot.bind(null, root),
734
+ );
735
} else {
736
const schedulerPriorityLevel = lanePriorityToSchedulerPriority(
737
newCallbackPriority,
@@ -756,7 +762,20 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
762
763
// Flush any pending passive effects before deciding which lanes to work on,
764
// in case they schedule additional work.
759
- flushPassiveEffects();
765
+ const originalCallbackNode = root.callbackNode;
766
+ const didFlushPassiveEffects = flushPassiveEffects();
767
+ if (didFlushPassiveEffects) {
768
+ // Something in the passive effect phase may have canceled the current task.
769
+ // Check if the task node for this root was changed.
770
+ if (root.callbackNode !== originalCallbackNode) {
771
+ // The current task was canceled. Exit. We don't need to call
772
+ // `ensureRootIsScheduled` because the check above implies either that
773
+ // there's a new task, or that there's no remaining work on this root.
774
+ return null;
775
+ } else {
776
+ // Current task was not canceled. Continue.
777
+ }
778
+ }
779
780
// Determine the next expiration time to work on, using the fields stored
781
// on the root.
@@ -765,6 +784,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
784
root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
785
);
786
if (lanes === NoLanes) {
787
+ // Defensive coding. This is never expected to happen.
788
return null;
789
}
790
@@ -781,8 +801,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
801
return null;
802
}
803
784
- const originalCallbackNode = root.callbackNode;
785
-
804
let exitStatus = renderRootConcurrent(root, lanes);
805
806
if (
@@ -2593,7 +2611,8 @@ function commitLayoutEffectsImpl(
2611
resetCurrentDebugFiberInDEV();
2612
}
2613
2596
-export function flushPassiveEffects() {
2614
+export function flushPassiveEffects(): boolean {
2615
+ // Returns whether passive effects were flushed.
2616
if (pendingPassiveEffectsRenderPriority !== NoSchedulerPriority) {
2617
const priorityLevel =
2618
pendingPassiveEffectsRenderPriority > NormalSchedulerPriority
@@ -2610,6 +2629,7 @@ export function flushPassiveEffects() {
2629
setCurrentUpdateLanePriority(previousLanePriority);
2630
}
2631
}
2632
+ return false;
2633
}
2634
2635
export function enqueuePendingPassiveProfilerEffect(fiber: Fiber): void {
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+24
-4
@@ -136,6 +136,7 @@ import {
136
import {
137
NoLanePriority,
138
SyncLanePriority,
139
+ SyncBatchedLanePriority,
140
InputDiscreteLanePriority,
141
TransitionShortLanePriority,
142
TransitionLongLanePriority,
@@ -719,6 +720,11 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
720
newCallbackNode = scheduleSyncCallback(
721
performSyncWorkOnRoot.bind(null, root),
722
);
723
+ } else if (newCallbackPriority === SyncBatchedLanePriority) {
724
+ newCallbackNode = scheduleCallback(
725
+ ImmediateSchedulerPriority,
726
+ performSyncWorkOnRoot.bind(null, root),
727
+ );
728
} else {
729
const schedulerPriorityLevel = lanePriorityToSchedulerPriority(
730
newCallbackPriority,
@@ -749,7 +755,20 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
755
756
// Flush any pending passive effects before deciding which lanes to work on,
757
// in case they schedule additional work.
752
- flushPassiveEffects();
758
+ const originalCallbackNode = root.callbackNode;
759
+ const didFlushPassiveEffects = flushPassiveEffects();
760
+ if (didFlushPassiveEffects) {
761
+ // Something in the passive effect phase may have canceled the current task.
762
+ // Check if the task node for this root was changed.
763
+ if (root.callbackNode !== originalCallbackNode) {
764
+ // The current task was canceled. Exit. We don't need to call
765
+ // `ensureRootIsScheduled` because the check above implies either that
766
+ // there's a new task, or that there's no remaining work on this root.
767
+ return null;
768
+ } else {
769
+ // Current task was not canceled. Continue.
770
+ }
771
+ }
772
773
// Determine the next expiration time to work on, using the fields stored
774
// on the root.
@@ -758,6 +777,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
777
root === workInProgressRoot ? workInProgressRootRenderLanes : NoLanes,
778
);
779
if (lanes === NoLanes) {
780
+ // Defensive coding. This is never expected to happen.
781
return null;
782
}
783
@@ -774,8 +794,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
794
return null;
795
}
796
777
- const originalCallbackNode = root.callbackNode;
778
-
797
let exitStatus = renderRootConcurrent(root, lanes);
798
799
if (
@@ -2437,7 +2455,8 @@ function commitLayoutEffects(root: FiberRoot, committedLanes: Lanes) {
2455
}
2456
}
2457
2440
-export function flushPassiveEffects() {
2458
+export function flushPassiveEffects(): boolean {
2459
+ // Returns whether passive effects were flushed.
2460
if (pendingPassiveEffectsRenderPriority !== NoSchedulerPriority) {
2461
const priorityLevel =
2462
pendingPassiveEffectsRenderPriority > NormalSchedulerPriority
@@ -2454,6 +2473,7 @@ export function flushPassiveEffects() {
2473
setCurrentUpdateLanePriority(previousLanePriority);
2474
}
2475
}
2476
+ return false;
2477
}
2478
2479
export function enqueuePendingPassiveProfilerEffect(fiber: Fiber): void {