Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Measure callback timeout relative to current time #15479

Merged
merged 1 commit into from
Apr 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,21 +234,20 @@ let interruptedBy: Fiber | null = null;
// In other words, because expiration times determine how updates are batched,
// we want all updates of like priority that occur within the same event to
// receive the same expiration time. Otherwise we get tearing.
let initialTimeMs: number = now();
let currentEventTime: ExpirationTime = NoWork;

export function requestCurrentTime() {
if (workPhase === RenderPhase || workPhase === CommitPhase) {
// We're inside React, so it's fine to read the actual time.
return msToExpirationTime(now() - initialTimeMs);
return msToExpirationTime(now());
}
// We're not inside React, so we may be in the middle of a browser event.
if (currentEventTime !== NoWork) {
// Use the same start time for all updates until we enter React again.
return currentEventTime;
}
// This is the first update since React yielded. Compute a new start time.
currentEventTime = msToExpirationTime(now() - initialTimeMs);
currentEventTime = msToExpirationTime(now());
return currentEventTime;
}

Expand Down Expand Up @@ -453,10 +452,18 @@ function scheduleCallbackForRoot(
cancelCallback(existingCallbackNode);
}
root.callbackExpirationTime = expirationTime;
const options =
expirationTime === Sync
? null
: {timeout: expirationTimeToMs(expirationTime)};

let options = null;
if (expirationTime !== Sync && expirationTime !== Never) {
let timeout = expirationTimeToMs(expirationTime) - now();
if (timeout > 5000) {
// Sanity check. Should never take longer than 5 seconds.
// TODO: Add internal warning?
timeout = 5000;
}
options = {timeout};
}

root.callbackNode = scheduleCallback(
priorityLevel,
runRootCallback.bind(
Expand Down Expand Up @@ -950,7 +957,7 @@ function inferTimeFromExpirationTime(expirationTime: ExpirationTime): number {
// We don't know exactly when the update was scheduled, but we can infer an
// approximate start time from the expiration time.
const earliestExpirationTimeMs = expirationTimeToMs(expirationTime);
return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION + initialTimeMs;
return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION;
}

function workLoopSync() {
Expand Down Expand Up @@ -1861,7 +1868,7 @@ function computeMsUntilTimeout(

// Compute the time until this render pass would expire.
const timeUntilExpirationMs =
expirationTimeToMs(committedExpirationTime) + initialTimeMs - currentTimeMs;
expirationTimeToMs(committedExpirationTime) - currentTimeMs;

// Clamp the timeout to the expiration time.
// TODO: Once the event time is exact instead of inferred from expiration time
Expand Down
12 changes: 11 additions & 1 deletion packages/react-reconciler/src/SchedulerWithReactIntegration.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,24 @@ export const IdlePriority: ReactPriorityLevel = 95;
// NoPriority is the absence of priority. Also React-only.
export const NoPriority: ReactPriorityLevel = 90;

export const now = Scheduler_now;
export const shouldYield = disableYielding
? () => false // Never yield when `disableYielding` is on
: Scheduler_shouldYield;

let immediateQueue: Array<SchedulerCallback> | null = null;
let immediateQueueCallbackNode: mixed | null = null;
let isFlushingImmediate: boolean = false;
let initialTimeMs: number = Scheduler_now();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be const?


// If the initial timestamp is reasonably small, use Scheduler's `now` directly.
// This will be the case for modern browsers that support `performance.now`. In
// older browsers, Scheduler falls back to `Date.now`, which returns a Unix
// timestamp. In that case, subtract the module initialization time to simulate
// the behavior of performance.now and keep our times small enough to fit
// within 32 bits.
// TODO: Consider lifting this into Scheduler.
export const now =
initialTimeMs < 10000 ? Scheduler_now : () => Scheduler_now() - initialTimeMs;

export function getCurrentPriorityLevel(): ReactPriorityLevel {
switch (Scheduler_getCurrentPriorityLevel()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -272,4 +272,25 @@ describe('ReactExpiration', () => {
expect(Scheduler).toFlushExpired([]);
expect(ReactNoop).toMatchRenderedOutput('Hi');
});

it('should measure callback timeout relative to current time, not start-up time', () => {
// Corresponds to a bugfix: https://github.com/facebook/react/pull/15479
// The bug wasn't caught by other tests because we use virtual times that
// default to 0, and most tests don't advance time.

// Before scheduling an update, advance the current time.
Scheduler.advanceTime(10000);

ReactNoop.render('Hi');
expect(Scheduler).toFlushExpired([]);
expect(ReactNoop).toMatchRenderedOutput(null);

// Advancing by ~5 seconds should be sufficient to expire the update. (I
// used a slightly larger number to allow for possible rounding.)
Scheduler.advanceTime(6000);

ReactNoop.render('Hi');
expect(Scheduler).toFlushExpired([]);
expect(ReactNoop).toMatchRenderedOutput('Hi');
});
});