Remove `didTimeout` check from work loop
No longer need this, since we have starvation protection in userspace. This will also allow us to remove the concept from the Scheduler package, which is nice because `postTask` doesn't currently support it.
Andrew Clark committed
Aug 13, 2020 at 18:02 UTC
3f8115cdd1e6ba237619cf8a7d433900dcf413c2
3 files changed
+10
-30
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+1
-14
@@ -754,7 +754,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
754
755
// This is the entry point for every concurrent task, i.e. anything that
756
// goes through Scheduler.
757
-function performConcurrentWorkOnRoot(root, didTimeout) {
757
+function performConcurrentWorkOnRoot(root) {
758
// Since we know we're in a React event, we can clear the current
759
// event time. The next update will compute a new event time.
760
currentEventTime = NoTimestamp;
@@ -794,19 +794,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
794
return null;
795
}
796
797
- // TODO: We only check `didTimeout` defensively, to account for a Scheduler
798
- // bug where `shouldYield` sometimes returns `true` even if `didTimeout` is
799
- // true, which leads to an infinite loop. Once the bug in Scheduler is
800
- // fixed, we can remove this, since we track expiration ourselves.
801
- if (didTimeout) {
802
- // Something expired. Flush synchronously until there's no expired
803
- // work left.
804
- markRootExpired(root, lanes);
805
- // This will schedule a synchronous callback.
806
- ensureRootIsScheduled(root, now());
807
- return null;
808
- }
809
-
797
let exitStatus = renderRootConcurrent(root, lanes);
798
799
if (
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+1
-14
@@ -741,7 +741,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
741
742
// This is the entry point for every concurrent task, i.e. anything that
743
// goes through Scheduler.
744
-function performConcurrentWorkOnRoot(root, didTimeout) {
744
+function performConcurrentWorkOnRoot(root) {
745
// Since we know we're in a React event, we can clear the current
746
// event time. The next update will compute a new event time.
747
currentEventTime = NoTimestamp;
@@ -781,19 +781,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
781
return null;
782
}
783
784
- // TODO: We only check `didTimeout` defensively, to account for a Scheduler
785
- // bug where `shouldYield` sometimes returns `true` even if `didTimeout` is
786
- // true, which leads to an infinite loop. Once the bug in Scheduler is
787
- // fixed, we can remove this, since we track expiration ourselves.
788
- if (didTimeout) {
789
- // Something expired. Flush synchronously until there's no expired
790
- // work left.
791
- markRootExpired(root, lanes);
792
- // This will schedule a synchronous callback.
793
- ensureRootIsScheduled(root, now());
794
- return null;
795
- }
796
-
784
let exitStatus = renderRootConcurrent(root, lanes);
785
786
if (
packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js
+8
-2
@@ -446,8 +446,6 @@ describe(
446
React = require('react');
447
ReactNoop = require('react-noop-renderer');
448
Scheduler = require('scheduler');
449
-
450
- React = require('react');
449
});
450
451
afterEach(() => {
@@ -531,6 +529,14 @@ describe(
529
530
// Expire the task
531
Scheduler.unstable_advanceTime(10000);
532
+ // Scheduling a new update is a trick to force the expiration to kick
533
+ // in. We don't check if a update has been starved at the beginning of
534
+ // working on it, since there's no point — we're already working on it.
535
+ // We only check before yielding to the main thread (to avoid starvation
536
+ // by other main thread work) or when receiving an update (to avoid
537
+ // starvation by incoming updates).
538
+ ReactNoop.render(<App />);
539
+
540
// Because the render expired, React should finish the tree without
541
// consulting `shouldYield` again
542
expect(Scheduler).toFlushExpired(['B', 'C']);