Skip to content

Commit 3f8115c

Browse files
committed
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.
1 parent 9abc278 commit 3f8115c

File tree

3 files changed

+10
-30
lines changed

3 files changed

+10
-30
lines changed

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

+1-14
Original file line numberDiff line numberDiff line change
@@ -754,7 +754,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
754754

755755
// This is the entry point for every concurrent task, i.e. anything that
756756
// goes through Scheduler.
757-
function performConcurrentWorkOnRoot(root, didTimeout) {
757+
function performConcurrentWorkOnRoot(root) {
758758
// Since we know we're in a React event, we can clear the current
759759
// event time. The next update will compute a new event time.
760760
currentEventTime = NoTimestamp;
@@ -794,19 +794,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
794794
return null;
795795
}
796796

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-
810797
let exitStatus = renderRootConcurrent(root, lanes);
811798

812799
if (

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

+1-14
Original file line numberDiff line numberDiff line change
@@ -741,7 +741,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
741741

742742
// This is the entry point for every concurrent task, i.e. anything that
743743
// goes through Scheduler.
744-
function performConcurrentWorkOnRoot(root, didTimeout) {
744+
function performConcurrentWorkOnRoot(root) {
745745
// Since we know we're in a React event, we can clear the current
746746
// event time. The next update will compute a new event time.
747747
currentEventTime = NoTimestamp;
@@ -781,19 +781,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
781781
return null;
782782
}
783783

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-
797784
let exitStatus = renderRootConcurrent(root, lanes);
798785

799786
if (

packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js

+8-2
Original file line numberDiff line numberDiff line change
@@ -446,8 +446,6 @@ describe(
446446
React = require('react');
447447
ReactNoop = require('react-noop-renderer');
448448
Scheduler = require('scheduler');
449-
450-
React = require('react');
451449
});
452450

453451
afterEach(() => {
@@ -531,6 +529,14 @@ describe(
531529

532530
// Expire the task
533531
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+
534540
// Because the render expired, React should finish the tree without
535541
// consulting `shouldYield` again
536542
expect(Scheduler).toFlushExpired(['B', 'C']);

0 commit comments

Comments
 (0)