Skip to content

Commit 71c8759

Browse files
authored
Measure callback timeout relative to current time (#15479)
Fixes a bug where the timeout passed to `scheduleCallback` represented an absolute timestamp, instead of the amount of time until that timestamp is reached. The solution is to subtract the current time from the expiration. The bug wasn't caught by other tests because we use virtual times that default to 0, and most tests don't advance time. I also moved the `initialTimeMs` offset to the `SchedulerWithReactIntegration` module so that we don't have to remember to subtract the offset every time. (We should consider upstreaming this to the Scheduler package.)
1 parent 9c6ff13 commit 71c8759

File tree

3 files changed

+48
-10
lines changed

3 files changed

+48
-10
lines changed

packages/react-reconciler/src/ReactFiberScheduler.js

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -234,21 +234,20 @@ let interruptedBy: Fiber | null = null;
234234
// In other words, because expiration times determine how updates are batched,
235235
// we want all updates of like priority that occur within the same event to
236236
// receive the same expiration time. Otherwise we get tearing.
237-
let initialTimeMs: number = now();
238237
let currentEventTime: ExpirationTime = NoWork;
239238

240239
export function requestCurrentTime() {
241240
if (workPhase === RenderPhase || workPhase === CommitPhase) {
242241
// We're inside React, so it's fine to read the actual time.
243-
return msToExpirationTime(now() - initialTimeMs);
242+
return msToExpirationTime(now());
244243
}
245244
// We're not inside React, so we may be in the middle of a browser event.
246245
if (currentEventTime !== NoWork) {
247246
// Use the same start time for all updates until we enter React again.
248247
return currentEventTime;
249248
}
250249
// This is the first update since React yielded. Compute a new start time.
251-
currentEventTime = msToExpirationTime(now() - initialTimeMs);
250+
currentEventTime = msToExpirationTime(now());
252251
return currentEventTime;
253252
}
254253

@@ -453,10 +452,18 @@ function scheduleCallbackForRoot(
453452
cancelCallback(existingCallbackNode);
454453
}
455454
root.callbackExpirationTime = expirationTime;
456-
const options =
457-
expirationTime === Sync
458-
? null
459-
: {timeout: expirationTimeToMs(expirationTime)};
455+
456+
let options = null;
457+
if (expirationTime !== Sync && expirationTime !== Never) {
458+
let timeout = expirationTimeToMs(expirationTime) - now();
459+
if (timeout > 5000) {
460+
// Sanity check. Should never take longer than 5 seconds.
461+
// TODO: Add internal warning?
462+
timeout = 5000;
463+
}
464+
options = {timeout};
465+
}
466+
460467
root.callbackNode = scheduleCallback(
461468
priorityLevel,
462469
runRootCallback.bind(
@@ -949,7 +956,7 @@ function inferTimeFromExpirationTime(expirationTime: ExpirationTime): number {
949956
// We don't know exactly when the update was scheduled, but we can infer an
950957
// approximate start time from the expiration time.
951958
const earliestExpirationTimeMs = expirationTimeToMs(expirationTime);
952-
return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION + initialTimeMs;
959+
return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION;
953960
}
954961

955962
function workLoopSync() {
@@ -1860,7 +1867,7 @@ function computeMsUntilTimeout(
18601867

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

18651872
// Clamp the timeout to the expiration time.
18661873
// TODO: Once the event time is exact instead of inferred from expiration time

packages/react-reconciler/src/SchedulerWithReactIntegration.js

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,24 @@ export const IdlePriority: ReactPriorityLevel = 95;
6565
// NoPriority is the absence of priority. Also React-only.
6666
export const NoPriority: ReactPriorityLevel = 90;
6767

68-
export const now = Scheduler_now;
6968
export const shouldYield = disableYielding
7069
? () => false // Never yield when `disableYielding` is on
7170
: Scheduler_shouldYield;
7271

7372
let immediateQueue: Array<SchedulerCallback> | null = null;
7473
let immediateQueueCallbackNode: mixed | null = null;
7574
let isFlushingImmediate: boolean = false;
75+
let initialTimeMs: number = Scheduler_now();
76+
77+
// If the initial timestamp is reasonably small, use Scheduler's `now` directly.
78+
// This will be the case for modern browsers that support `performance.now`. In
79+
// older browsers, Scheduler falls back to `Date.now`, which returns a Unix
80+
// timestamp. In that case, subtract the module initialization time to simulate
81+
// the behavior of performance.now and keep our times small enough to fit
82+
// within 32 bits.
83+
// TODO: Consider lifting this into Scheduler.
84+
export const now =
85+
initialTimeMs < 10000 ? Scheduler_now : () => Scheduler_now() - initialTimeMs;
7686

7787
export function getCurrentPriorityLevel(): ReactPriorityLevel {
7888
switch (Scheduler_getCurrentPriorityLevel()) {

packages/react-reconciler/src/__tests__/ReactExpiration-test.internal.js

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,4 +272,25 @@ describe('ReactExpiration', () => {
272272
expect(Scheduler).toFlushExpired([]);
273273
expect(ReactNoop).toMatchRenderedOutput('Hi');
274274
});
275+
276+
it('should measure callback timeout relative to current time, not start-up time', () => {
277+
// Corresponds to a bugfix: https://github.com/facebook/react/pull/15479
278+
// The bug wasn't caught by other tests because we use virtual times that
279+
// default to 0, and most tests don't advance time.
280+
281+
// Before scheduling an update, advance the current time.
282+
Scheduler.advanceTime(10000);
283+
284+
ReactNoop.render('Hi');
285+
expect(Scheduler).toFlushExpired([]);
286+
expect(ReactNoop).toMatchRenderedOutput(null);
287+
288+
// Advancing by ~5 seconds should be sufficient to expire the update. (I
289+
// used a slightly larger number to allow for possible rounding.)
290+
Scheduler.advanceTime(6000);
291+
292+
ReactNoop.render('Hi');
293+
expect(Scheduler).toFlushExpired([]);
294+
expect(ReactNoop).toMatchRenderedOutput('Hi');
295+
});
275296
});

0 commit comments

Comments
 (0)