From 946f58891c0ae5117fa2d74a925ba5c84730ae2a Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Tue, 16 Feb 2021 14:38:11 -0600
Subject: [PATCH] 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.
---
 .../plugins/__tests__/SimpleEventPlugin-test.js    |  4 ++--
 .../react-reconciler/src/ReactFiberWorkLoop.new.js | 14 ++++++++++++++
 .../react-reconciler/src/ReactFiberWorkLoop.old.js | 14 ++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js b/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js
index 377fa61e507d7..795eda90c83cc 100644
--- a/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js
+++ b/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js
@@ -477,8 +477,8 @@ describe('SimpleEventPlugin', function() {
         // At the end, both counters should equal the total number of clicks
         expect(Scheduler).toHaveYielded([
           'High-pri count: 8, Low-pri count: 0',
-
-          // TODO: with cancellation, this required another flush?
+        ]);
+        expect(Scheduler).toFlushAndYield([
           'High-pri count: 8, Low-pri count: 8',
         ]);
       } else {
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
index 6935bb4b1f18d..918261a2e3562 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -1028,6 +1028,20 @@ function performSyncWorkOnRoot(root) {
     exitStatus = renderRootSync(root, lanes);
   } else {
     lanes = getNextLanes(root, NoLanes);
+    // 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. If we had only one, then (I believe) this extra check wouldn't be
+    // necessary, because there's nothing higher priority than sync that would
+    // cause us to cancel it.
+    // TODO: Merge InputDiscreteLanePriority with SyncLanePriority, then delete
+    // this bailout.
+    if (supportsMicrotasks) {
+      const nextLanesPriority = returnNextLanesPriority();
+      if (nextLanesPriority < InputDiscreteLanePriority) {
+        return null;
+      }
+    }
     exitStatus = renderRootSync(root, lanes);
   }
 
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
index 2b57becf8bd79..12ef384a47527 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -1028,6 +1028,20 @@ function performSyncWorkOnRoot(root) {
     exitStatus = renderRootSync(root, lanes);
   } else {
     lanes = getNextLanes(root, NoLanes);
+    // 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. If we had only one, then (I believe) this extra check wouldn't be
+    // necessary, because there's nothing higher priority than sync that would
+    // cause us to cancel it.
+    // TODO: Merge InputDiscreteLanePriority with SyncLanePriority, then delete
+    // this bailout.
+    if (supportsMicrotasks) {
+      const nextLanesPriority = returnNextLanesPriority();
+      if (nextLanesPriority < InputDiscreteLanePriority) {
+        return null;
+      }
+    }
     exitStatus = renderRootSync(root, lanes);
   }