Skip to content

Commit 1022ee0

Browse files
authored
Read current time without marking event start time (#17160)
* Failing test: DevTools hook freezes timeline The DevTools hook calls `requestCurrentTime` after the commit phase has ended, which has the accidnental consequence of freezing the start time for subsequent updates. If enough time goes by, the next update will instantly expire. I'll push a fix in the next commit. * Read current time without marking event start time `requestCurrentTime` is only meant to be used for updates, because subsequent calls within the same event will receive the same time. Messing this up has bad consequences. I renamed it to `requestCurrentTimeForUpdate` and created a new function that returns the current time without the batching heuristic, called `getCurrentTime`. Swapping `requestCurrentTime` for `getCurrentTime` in the DevTools hook fixes the regression test added in the previous commit.
1 parent 349cf5a commit 1022ee0

File tree

7 files changed

+54
-20
lines changed

7 files changed

+54
-20
lines changed

packages/react-reconciler/src/ReactFiberBeginWork.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ import {
173173
} from './ReactFiber';
174174
import {
175175
markSpawnedWork,
176-
requestCurrentTime,
176+
requestCurrentTimeForUpdate,
177177
retryDehydratedSuspenseBoundary,
178178
scheduleWork,
179179
renderDidSuspendDelayIfPossible,
@@ -1990,7 +1990,7 @@ function mountDehydratedSuspenseComponent(
19901990
// a protocol to transfer that time, we'll just estimate it by using the current
19911991
// time. This will mean that Suspense timeouts are slightly shifted to later than
19921992
// they should be.
1993-
let serverDisplayTime = requestCurrentTime();
1993+
let serverDisplayTime = requestCurrentTimeForUpdate();
19941994
// Schedule a normal pri update to render this content.
19951995
let newExpirationTime = computeAsyncExpiration(serverDisplayTime);
19961996
if (enableSchedulerTracing) {

packages/react-reconciler/src/ReactFiberClassComponent.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ import {
5050
} from './ReactFiberContext';
5151
import {readContext} from './ReactFiberNewContext';
5252
import {
53-
requestCurrentTime,
53+
requestCurrentTimeForUpdate,
5454
computeExpirationForFiber,
5555
scheduleWork,
5656
} from './ReactFiberWorkLoop';
@@ -183,7 +183,7 @@ const classComponentUpdater = {
183183
isMounted,
184184
enqueueSetState(inst, payload, callback) {
185185
const fiber = getInstance(inst);
186-
const currentTime = requestCurrentTime();
186+
const currentTime = requestCurrentTimeForUpdate();
187187
const suspenseConfig = requestCurrentSuspenseConfig();
188188
const expirationTime = computeExpirationForFiber(
189189
currentTime,
@@ -205,7 +205,7 @@ const classComponentUpdater = {
205205
},
206206
enqueueReplaceState(inst, payload, callback) {
207207
const fiber = getInstance(inst);
208-
const currentTime = requestCurrentTime();
208+
const currentTime = requestCurrentTimeForUpdate();
209209
const suspenseConfig = requestCurrentSuspenseConfig();
210210
const expirationTime = computeExpirationForFiber(
211211
currentTime,
@@ -229,7 +229,7 @@ const classComponentUpdater = {
229229
},
230230
enqueueForceUpdate(inst, callback) {
231231
const fiber = getInstance(inst);
232-
const currentTime = requestCurrentTime();
232+
const currentTime = requestCurrentTimeForUpdate();
233233
const suspenseConfig = requestCurrentSuspenseConfig();
234234
const expirationTime = computeExpirationForFiber(
235235
currentTime,

packages/react-reconciler/src/ReactFiberDevToolsHook.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*/
99

1010
import {enableProfilerTimer} from 'shared/ReactFeatureFlags';
11-
import {requestCurrentTime} from './ReactFiberWorkLoop';
11+
import {getCurrentTime} from './ReactFiberWorkLoop';
1212
import {inferPriorityFromExpirationTime} from './ReactFiberExpirationTime';
1313

1414
import type {Fiber} from './ReactFiber';
@@ -58,7 +58,7 @@ export function injectInternals(internals: Object): boolean {
5858
try {
5959
const didError = (root.current.effectTag & DidCapture) === DidCapture;
6060
if (enableProfilerTimer) {
61-
const currentTime = requestCurrentTime();
61+
const currentTime = getCurrentTime();
6262
const priorityLevel = inferPriorityFromExpirationTime(
6363
currentTime,
6464
expirationTime,

packages/react-reconciler/src/ReactFiberHooks.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ import {
3939
import {
4040
scheduleWork,
4141
computeExpirationForFiber,
42-
requestCurrentTime,
42+
requestCurrentTimeForUpdate,
4343
warnIfNotCurrentlyActingEffectsInDEV,
4444
warnIfNotCurrentlyActingUpdatesInDev,
4545
warnIfNotScopedWithMatchingAct,
@@ -1273,7 +1273,7 @@ function dispatchAction<S, A>(
12731273
lastRenderPhaseUpdate.next = update;
12741274
}
12751275
} else {
1276-
const currentTime = requestCurrentTime();
1276+
const currentTime = requestCurrentTimeForUpdate();
12771277
const suspenseConfig = requestCurrentSuspenseConfig();
12781278
const expirationTime = computeExpirationForFiber(
12791279
currentTime,

packages/react-reconciler/src/ReactFiberReconciler.js

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ import {
5050
import {createFiberRoot} from './ReactFiberRoot';
5151
import {injectInternals} from './ReactFiberDevToolsHook';
5252
import {
53-
requestCurrentTime,
53+
requestCurrentTimeForUpdate,
5454
computeExpirationForFiber,
5555
scheduleWork,
5656
flushRoot,
@@ -231,7 +231,7 @@ export function updateContainer(
231231
callback: ?Function,
232232
): ExpirationTime {
233233
const current = container.current;
234-
const currentTime = requestCurrentTime();
234+
const currentTime = requestCurrentTimeForUpdate();
235235
if (__DEV__) {
236236
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
237237
if ('undefined' !== typeof jest) {
@@ -348,7 +348,9 @@ export function attemptSynchronousHydration(fiber: Fiber): void {
348348
// If we're still blocked after this, we need to increase
349349
// the priority of any promises resolving within this
350350
// boundary so that they next attempt also has higher pri.
351-
let retryExpTime = computeInteractiveExpiration(requestCurrentTime());
351+
let retryExpTime = computeInteractiveExpiration(
352+
requestCurrentTimeForUpdate(),
353+
);
352354
markRetryTimeIfNotHydrated(fiber, retryExpTime);
353355
break;
354356
}
@@ -380,7 +382,7 @@ export function attemptUserBlockingHydration(fiber: Fiber): void {
380382
// Suspense.
381383
return;
382384
}
383-
let expTime = computeInteractiveExpiration(requestCurrentTime());
385+
let expTime = computeInteractiveExpiration(requestCurrentTimeForUpdate());
384386
scheduleWork(fiber, expTime);
385387
markRetryTimeIfNotHydrated(fiber, expTime);
386388
}
@@ -393,7 +395,9 @@ export function attemptContinuousHydration(fiber: Fiber): void {
393395
// Suspense.
394396
return;
395397
}
396-
let expTime = computeContinuousHydrationExpiration(requestCurrentTime());
398+
let expTime = computeContinuousHydrationExpiration(
399+
requestCurrentTimeForUpdate(),
400+
);
397401
scheduleWork(fiber, expTime);
398402
markRetryTimeIfNotHydrated(fiber, expTime);
399403
}
@@ -404,7 +408,7 @@ export function attemptHydrationAtCurrentPriority(fiber: Fiber): void {
404408
// their priority other than synchronously flush it.
405409
return;
406410
}
407-
const currentTime = requestCurrentTime();
411+
const currentTime = requestCurrentTimeForUpdate();
408412
const expTime = computeExpirationForFiber(currentTime, fiber, null);
409413
scheduleWork(fiber, expTime);
410414
markRetryTimeIfNotHydrated(fiber, expTime);

packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,7 @@ let spawnedWorkDuringRender: null | Array<ExpirationTime> = null;
288288
// receive the same expiration time. Otherwise we get tearing.
289289
let currentEventTime: ExpirationTime = NoWork;
290290

291-
export function requestCurrentTime() {
291+
export function requestCurrentTimeForUpdate() {
292292
if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
293293
// We're inside React, so it's fine to read the actual time.
294294
return msToExpirationTime(now());
@@ -303,6 +303,10 @@ export function requestCurrentTime() {
303303
return currentEventTime;
304304
}
305305

306+
export function getCurrentTime() {
307+
return msToExpirationTime(now());
308+
}
309+
306310
export function computeExpirationForFiber(
307311
currentTime: ExpirationTime,
308312
fiber: Fiber,
@@ -571,7 +575,7 @@ function ensureRootIsScheduled(root: FiberRoot) {
571575

572576
// TODO: If this is an update, we already read the current time. Pass the
573577
// time as an argument.
574-
const currentTime = requestCurrentTime();
578+
const currentTime = requestCurrentTimeForUpdate();
575579
const priorityLevel = inferPriorityFromExpirationTime(
576580
currentTime,
577581
expirationTime,
@@ -632,7 +636,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
632636
if (didTimeout) {
633637
// The render task took too long to complete. Mark the current time as
634638
// expired to synchronously render all expired work in a single batch.
635-
const currentTime = requestCurrentTime();
639+
const currentTime = requestCurrentTimeForUpdate();
636640
markRootExpiredAtTime(root, currentTime);
637641
// This will schedule a synchronous callback.
638642
ensureRootIsScheduled(root);
@@ -2380,7 +2384,7 @@ function retryTimedOutBoundary(
23802384
// likely unblocked. Try rendering again, at a new expiration time.
23812385
if (retryTime === NoWork) {
23822386
const suspenseConfig = null; // Retries don't carry over the already committed update.
2383-
const currentTime = requestCurrentTime();
2387+
const currentTime = requestCurrentTimeForUpdate();
23842388
retryTime = computeExpirationForFiber(
23852389
currentTime,
23862390
boundaryFiber,

packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,4 +177,30 @@ describe('ReactProfiler DevTools integration', () => {
177177
{name: 'some event', timestamp: eventTime},
178178
]);
179179
});
180+
181+
it('regression test: #17159', () => {
182+
function Text({text}) {
183+
Scheduler.unstable_yieldValue(text);
184+
return text;
185+
}
186+
187+
const root = ReactTestRenderer.create(null, {unstable_isConcurrent: true});
188+
189+
// Commit something
190+
root.update(<Text text="A" />);
191+
expect(Scheduler).toFlushAndYield(['A']);
192+
expect(root).toMatchRenderedOutput('A');
193+
194+
// Advance time by many seconds, larger than the default expiration time
195+
// for updates.
196+
Scheduler.unstable_advanceTime(10000);
197+
// Schedule an update.
198+
root.update(<Text text="B" />);
199+
200+
// Update B should not instantly expire.
201+
expect(Scheduler).toFlushExpired([]);
202+
203+
expect(Scheduler).toFlushAndYield(['B']);
204+
expect(root).toMatchRenderedOutput('B');
205+
});
180206
});

0 commit comments

Comments
 (0)