Skip to content

Commit e2fd460

Browse files
authored
Bailout in sync task if work is not sync (#20813)
Because we don't cancel synchronous tasks, sometimes more than one synchronous task ends up being scheduled. This is an artifact of the fact that we have two different lanes that schedule sync tasks: discrete and sync. So what can happen is that a discrete update gets scheduled, then a sync update right after that. Because sync is encoded as higher priority than discrete, we schedule a second sync task. And since we don't cancel the first one, there are now two separate sync tasks. As a next step, what we should do is merge InputDiscreteLane with SyncLane, then (I believe) this extra bailout wouldn't be necessary, because there's nothing higher priority than sync that would cause us to cancel it. Though we may want to add logging to be sure.
1 parent 9e8f3c8 commit e2fd460

File tree

3 files changed

+30
-2
lines changed

3 files changed

+30
-2
lines changed

packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -477,8 +477,8 @@ describe('SimpleEventPlugin', function() {
477477
// At the end, both counters should equal the total number of clicks
478478
expect(Scheduler).toHaveYielded([
479479
'High-pri count: 8, Low-pri count: 0',
480-
481-
// TODO: with cancellation, this required another flush?
480+
]);
481+
expect(Scheduler).toFlushAndYield([
482482
'High-pri count: 8, Low-pri count: 8',
483483
]);
484484
} else {

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

+14
Original file line numberDiff line numberDiff line change
@@ -1028,6 +1028,20 @@ function performSyncWorkOnRoot(root) {
10281028
exitStatus = renderRootSync(root, lanes);
10291029
} else {
10301030
lanes = getNextLanes(root, NoLanes);
1031+
// Because we don't cancel synchronous tasks, sometimes more than one
1032+
// synchronous task ends up being scheduled. This is an artifact of the fact
1033+
// that we have two different lanes that schedule sync tasks: discrete and
1034+
// sync. If we had only one, then (I believe) this extra check wouldn't be
1035+
// necessary, because there's nothing higher priority than sync that would
1036+
// cause us to cancel it.
1037+
// TODO: Merge InputDiscreteLanePriority with SyncLanePriority, then delete
1038+
// this bailout.
1039+
if (supportsMicrotasks) {
1040+
const nextLanesPriority = returnNextLanesPriority();
1041+
if (nextLanesPriority < InputDiscreteLanePriority) {
1042+
return null;
1043+
}
1044+
}
10311045
exitStatus = renderRootSync(root, lanes);
10321046
}
10331047

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

+14
Original file line numberDiff line numberDiff line change
@@ -1028,6 +1028,20 @@ function performSyncWorkOnRoot(root) {
10281028
exitStatus = renderRootSync(root, lanes);
10291029
} else {
10301030
lanes = getNextLanes(root, NoLanes);
1031+
// Because we don't cancel synchronous tasks, sometimes more than one
1032+
// synchronous task ends up being scheduled. This is an artifact of the fact
1033+
// that we have two different lanes that schedule sync tasks: discrete and
1034+
// sync. If we had only one, then (I believe) this extra check wouldn't be
1035+
// necessary, because there's nothing higher priority than sync that would
1036+
// cause us to cancel it.
1037+
// TODO: Merge InputDiscreteLanePriority with SyncLanePriority, then delete
1038+
// this bailout.
1039+
if (supportsMicrotasks) {
1040+
const nextLanesPriority = returnNextLanesPriority();
1041+
if (nextLanesPriority < InputDiscreteLanePriority) {
1042+
return null;
1043+
}
1044+
}
10311045
exitStatus = renderRootSync(root, lanes);
10321046
}
10331047

0 commit comments

Comments
 (0)