From 1022ee0ec140b8fce47c43ec57ee4a9f80f42eca Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 21 Oct 2019 13:15:37 -0700 Subject: [PATCH] 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. --- .../src/ReactFiberBeginWork.js | 4 +-- .../src/ReactFiberClassComponent.js | 8 +++--- .../src/ReactFiberDevToolsHook.js | 4 +-- .../react-reconciler/src/ReactFiberHooks.js | 4 +-- .../src/ReactFiberReconciler.js | 16 +++++++----- .../src/ReactFiberWorkLoop.js | 12 ++++++--- ...ofilerDevToolsIntegration-test.internal.js | 26 +++++++++++++++++++ 7 files changed, 54 insertions(+), 20 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index dacaf2f21a..f5fd39febf 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -173,7 +173,7 @@ import { } from './ReactFiber'; import { markSpawnedWork, - requestCurrentTime, + requestCurrentTimeForUpdate, retryDehydratedSuspenseBoundary, scheduleWork, renderDidSuspendDelayIfPossible, @@ -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) { diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 3f16fd1e64..d00150d725 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -50,7 +50,7 @@ import { } from './ReactFiberContext'; import {readContext} from './ReactFiberNewContext'; import { - requestCurrentTime, + requestCurrentTimeForUpdate, computeExpirationForFiber, scheduleWork, } from './ReactFiberWorkLoop'; @@ -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, @@ -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, @@ -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, diff --git a/packages/react-reconciler/src/ReactFiberDevToolsHook.js b/packages/react-reconciler/src/ReactFiberDevToolsHook.js index 45a51a3f03..af61b3bb65 100644 --- a/packages/react-reconciler/src/ReactFiberDevToolsHook.js +++ b/packages/react-reconciler/src/ReactFiberDevToolsHook.js @@ -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'; @@ -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, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 707a65ebef..14d70b5c5c 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -39,7 +39,7 @@ import { import { scheduleWork, computeExpirationForFiber, - requestCurrentTime, + requestCurrentTimeForUpdate, warnIfNotCurrentlyActingEffectsInDEV, warnIfNotCurrentlyActingUpdatesInDev, warnIfNotScopedWithMatchingAct, @@ -1273,7 +1273,7 @@ function dispatchAction( lastRenderPhaseUpdate.next = update; } } else { - const currentTime = requestCurrentTime(); + const currentTime = requestCurrentTimeForUpdate(); const suspenseConfig = requestCurrentSuspenseConfig(); const expirationTime = computeExpirationForFiber( currentTime, diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index 2fb1aa5297..d70196c55b 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -50,7 +50,7 @@ import { import {createFiberRoot} from './ReactFiberRoot'; import {injectInternals} from './ReactFiberDevToolsHook'; import { - requestCurrentTime, + requestCurrentTimeForUpdate, computeExpirationForFiber, scheduleWork, flushRoot, @@ -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) { @@ -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; } @@ -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); } @@ -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); } @@ -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); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 898457308a..56faafd858 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -288,7 +288,7 @@ let spawnedWorkDuringRender: null | Array = 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()); @@ -303,6 +303,10 @@ export function requestCurrentTime() { return currentEventTime; } +export function getCurrentTime() { + return msToExpirationTime(now()); +} + export function computeExpirationForFiber( currentTime: ExpirationTime, fiber: Fiber, @@ -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, @@ -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); @@ -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, diff --git a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js index b3efa665d5..d2f52dea1b 100644 --- a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js +++ b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js @@ -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(); + 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(); + + // Update B should not instantly expire. + expect(Scheduler).toFlushExpired([]); + + expect(Scheduler).toFlushAndYield(['B']); + expect(root).toMatchRenderedOutput('B'); + }); });