Skip to content

Commit

Permalink
Read current time without marking event start time (#17160)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
acdlite committed Oct 21, 2019
1 parent 349cf5a commit 1022ee0
Show file tree
Hide file tree
Showing 7 changed files with 54 additions and 20 deletions.
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ import {
} from './ReactFiber';
import {
markSpawnedWork,
requestCurrentTime,
requestCurrentTimeForUpdate,
retryDehydratedSuspenseBoundary,
scheduleWork,
renderDidSuspendDelayIfPossible,
Expand Down Expand Up @@ -1990,7 +1990,7 @@ function mountDehydratedSuspenseComponent(
// a protocol to transfer that time, we'll just estimate it by using the current
// time. This will mean that Suspense timeouts are slightly shifted to later than
// they should be.
let serverDisplayTime = requestCurrentTime();
let serverDisplayTime = requestCurrentTimeForUpdate();
// Schedule a normal pri update to render this content.
let newExpirationTime = computeAsyncExpiration(serverDisplayTime);
if (enableSchedulerTracing) {
Expand Down
8 changes: 4 additions & 4 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ import {
} from './ReactFiberContext';
import {readContext} from './ReactFiberNewContext';
import {
requestCurrentTime,
requestCurrentTimeForUpdate,
computeExpirationForFiber,
scheduleWork,
} from './ReactFiberWorkLoop';
Expand Down Expand Up @@ -183,7 +183,7 @@ const classComponentUpdater = {
isMounted,
enqueueSetState(inst, payload, callback) {
const fiber = getInstance(inst);
const currentTime = requestCurrentTime();
const currentTime = requestCurrentTimeForUpdate();
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
Expand All @@ -205,7 +205,7 @@ const classComponentUpdater = {
},
enqueueReplaceState(inst, payload, callback) {
const fiber = getInstance(inst);
const currentTime = requestCurrentTime();
const currentTime = requestCurrentTimeForUpdate();
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
Expand All @@ -229,7 +229,7 @@ const classComponentUpdater = {
},
enqueueForceUpdate(inst, callback) {
const fiber = getInstance(inst);
const currentTime = requestCurrentTime();
const currentTime = requestCurrentTimeForUpdate();
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberDevToolsHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import {enableProfilerTimer} from 'shared/ReactFeatureFlags';
import {requestCurrentTime} from './ReactFiberWorkLoop';
import {getCurrentTime} from './ReactFiberWorkLoop';
import {inferPriorityFromExpirationTime} from './ReactFiberExpirationTime';

import type {Fiber} from './ReactFiber';
Expand Down Expand Up @@ -58,7 +58,7 @@ export function injectInternals(internals: Object): boolean {
try {
const didError = (root.current.effectTag & DidCapture) === DidCapture;
if (enableProfilerTimer) {
const currentTime = requestCurrentTime();
const currentTime = getCurrentTime();
const priorityLevel = inferPriorityFromExpirationTime(
currentTime,
expirationTime,
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import {
import {
scheduleWork,
computeExpirationForFiber,
requestCurrentTime,
requestCurrentTimeForUpdate,
warnIfNotCurrentlyActingEffectsInDEV,
warnIfNotCurrentlyActingUpdatesInDev,
warnIfNotScopedWithMatchingAct,
Expand Down Expand Up @@ -1273,7 +1273,7 @@ function dispatchAction<S, A>(
lastRenderPhaseUpdate.next = update;
}
} else {
const currentTime = requestCurrentTime();
const currentTime = requestCurrentTimeForUpdate();
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
Expand Down
16 changes: 10 additions & 6 deletions packages/react-reconciler/src/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ import {
import {createFiberRoot} from './ReactFiberRoot';
import {injectInternals} from './ReactFiberDevToolsHook';
import {
requestCurrentTime,
requestCurrentTimeForUpdate,
computeExpirationForFiber,
scheduleWork,
flushRoot,
Expand Down Expand Up @@ -231,7 +231,7 @@ export function updateContainer(
callback: ?Function,
): ExpirationTime {
const current = container.current;
const currentTime = requestCurrentTime();
const currentTime = requestCurrentTimeForUpdate();
if (__DEV__) {
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
if ('undefined' !== typeof jest) {
Expand Down Expand Up @@ -348,7 +348,9 @@ export function attemptSynchronousHydration(fiber: Fiber): void {
// If we're still blocked after this, we need to increase
// the priority of any promises resolving within this
// boundary so that they next attempt also has higher pri.
let retryExpTime = computeInteractiveExpiration(requestCurrentTime());
let retryExpTime = computeInteractiveExpiration(
requestCurrentTimeForUpdate(),
);
markRetryTimeIfNotHydrated(fiber, retryExpTime);
break;
}
Expand Down Expand Up @@ -380,7 +382,7 @@ export function attemptUserBlockingHydration(fiber: Fiber): void {
// Suspense.
return;
}
let expTime = computeInteractiveExpiration(requestCurrentTime());
let expTime = computeInteractiveExpiration(requestCurrentTimeForUpdate());
scheduleWork(fiber, expTime);
markRetryTimeIfNotHydrated(fiber, expTime);
}
Expand All @@ -393,7 +395,9 @@ export function attemptContinuousHydration(fiber: Fiber): void {
// Suspense.
return;
}
let expTime = computeContinuousHydrationExpiration(requestCurrentTime());
let expTime = computeContinuousHydrationExpiration(
requestCurrentTimeForUpdate(),
);
scheduleWork(fiber, expTime);
markRetryTimeIfNotHydrated(fiber, expTime);
}
Expand All @@ -404,7 +408,7 @@ export function attemptHydrationAtCurrentPriority(fiber: Fiber): void {
// their priority other than synchronously flush it.
return;
}
const currentTime = requestCurrentTime();
const currentTime = requestCurrentTimeForUpdate();
const expTime = computeExpirationForFiber(currentTime, fiber, null);
scheduleWork(fiber, expTime);
markRetryTimeIfNotHydrated(fiber, expTime);
Expand Down
12 changes: 8 additions & 4 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ let spawnedWorkDuringRender: null | Array<ExpirationTime> = null;
// receive the same expiration time. Otherwise we get tearing.
let currentEventTime: ExpirationTime = NoWork;

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

export function getCurrentTime() {
return msToExpirationTime(now());
}

export function computeExpirationForFiber(
currentTime: ExpirationTime,
fiber: Fiber,
Expand Down Expand Up @@ -571,7 +575,7 @@ function ensureRootIsScheduled(root: FiberRoot) {

// TODO: If this is an update, we already read the current time. Pass the
// time as an argument.
const currentTime = requestCurrentTime();
const currentTime = requestCurrentTimeForUpdate();
const priorityLevel = inferPriorityFromExpirationTime(
currentTime,
expirationTime,
Expand Down Expand Up @@ -632,7 +636,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
if (didTimeout) {
// The render task took too long to complete. Mark the current time as
// expired to synchronously render all expired work in a single batch.
const currentTime = requestCurrentTime();
const currentTime = requestCurrentTimeForUpdate();
markRootExpiredAtTime(root, currentTime);
// This will schedule a synchronous callback.
ensureRootIsScheduled(root);
Expand Down Expand Up @@ -2380,7 +2384,7 @@ function retryTimedOutBoundary(
// likely unblocked. Try rendering again, at a new expiration time.
if (retryTime === NoWork) {
const suspenseConfig = null; // Retries don't carry over the already committed update.
const currentTime = requestCurrentTime();
const currentTime = requestCurrentTimeForUpdate();
retryTime = computeExpirationForFiber(
currentTime,
boundaryFiber,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,4 +177,30 @@ describe('ReactProfiler DevTools integration', () => {
{name: 'some event', timestamp: eventTime},
]);
});

it('regression test: #17159', () => {
function Text({text}) {
Scheduler.unstable_yieldValue(text);
return text;
}

const root = ReactTestRenderer.create(null, {unstable_isConcurrent: true});

// Commit something
root.update(<Text text="A" />);
expect(Scheduler).toFlushAndYield(['A']);
expect(root).toMatchRenderedOutput('A');

// Advance time by many seconds, larger than the default expiration time
// for updates.
Scheduler.unstable_advanceTime(10000);
// Schedule an update.
root.update(<Text text="B" />);

// Update B should not instantly expire.
expect(Scheduler).toFlushExpired([]);

expect(Scheduler).toFlushAndYield(['B']);
expect(root).toMatchRenderedOutput('B');
});
});

0 comments on commit 1022ee0

Please sign in to comment.