Skip to content

Commit

Permalink
Remove didTimeout check from work loop
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite committed Aug 14, 2020
1 parent 9abc278 commit 3f8115c
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 30 deletions.
15 changes: 1 addition & 14 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {

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

// TODO: We only check `didTimeout` defensively, to account for a Scheduler
// bug where `shouldYield` sometimes returns `true` even if `didTimeout` is
// true, which leads to an infinite loop. Once the bug in Scheduler is
// fixed, we can remove this, since we track expiration ourselves.
if (didTimeout) {

This comment has been minimized.

Copy link
@rickhanlonii

rickhanlonii Sep 3, 2020

Member

@acdlite do we have a different guard for this infinite loop?

This comment has been minimized.

Copy link
@acdlite

acdlite Sep 3, 2020

Author Collaborator

The infinite loop only happened because of the bug in Scheduler where didTimeout and shouldYield contradicted each other. Since those are now decoupled, it shouldn't happen anymore.

// Something expired. Flush synchronously until there's no expired
// work left.
markRootExpired(root, lanes);
// This will schedule a synchronous callback.
ensureRootIsScheduled(root, now());
return null;
}

let exitStatus = renderRootConcurrent(root, lanes);

if (
Expand Down
15 changes: 1 addition & 14 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -741,7 +741,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {

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

// TODO: We only check `didTimeout` defensively, to account for a Scheduler
// bug where `shouldYield` sometimes returns `true` even if `didTimeout` is
// true, which leads to an infinite loop. Once the bug in Scheduler is
// fixed, we can remove this, since we track expiration ourselves.
if (didTimeout) {
// Something expired. Flush synchronously until there's no expired
// work left.
markRootExpired(root, lanes);
// This will schedule a synchronous callback.
ensureRootIsScheduled(root, now());
return null;
}

let exitStatus = renderRootConcurrent(root, lanes);

if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,8 +446,6 @@ describe(
React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');

React = require('react');
});

afterEach(() => {
Expand Down Expand Up @@ -531,6 +529,14 @@ describe(

// Expire the task
Scheduler.unstable_advanceTime(10000);
// Scheduling a new update is a trick to force the expiration to kick
// in. We don't check if a update has been starved at the beginning of
// working on it, since there's no point — we're already working on it.
// We only check before yielding to the main thread (to avoid starvation
// by other main thread work) or when receiving an update (to avoid
// starvation by incoming updates).
ReactNoop.render(<App />);

// Because the render expired, React should finish the tree without
// consulting `shouldYield` again
expect(Scheduler).toFlushExpired(['B', 'C']);
Expand Down

0 comments on commit 3f8115c

Please sign in to comment.