@samitouri / QOS-React-2 / commits / 1022ee0ec1

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.

Andrew Clark committed Oct 21, 2019 at 13:15 UTC 1022ee0ec140b8fce47c43ec57ee4a9f80f42eca

7 files changed +54 -20

packages/react-reconciler/src/ReactFiberBeginWork.js

+2 -2

@@ -173,7 +173,7 @@ import {

173 } from './ReactFiber';

174 import {

175 markSpawnedWork,

176 - requestCurrentTime,

176 + requestCurrentTimeForUpdate,

177 retryDehydratedSuspenseBoundary,

178 scheduleWork,

179 renderDidSuspendDelayIfPossible,

@@ -1990,7 +1990,7 @@ function mountDehydratedSuspenseComponent(

1990 // a protocol to transfer that time, we'll just estimate it by using the current

1991 // time. This will mean that Suspense timeouts are slightly shifted to later than

1992 // they should be.

1993 - let serverDisplayTime = requestCurrentTime();

1993 + let serverDisplayTime = requestCurrentTimeForUpdate();

1994 // Schedule a normal pri update to render this content.

1995 let newExpirationTime = computeAsyncExpiration(serverDisplayTime);

1996 if (enableSchedulerTracing) {

packages/react-reconciler/src/ReactFiberClassComponent.js

+4 -4

@@ -50,7 +50,7 @@ import {

50 } from './ReactFiberContext';

51 import {readContext} from './ReactFiberNewContext';

52 import {

53 - requestCurrentTime,

53 + requestCurrentTimeForUpdate,

54 computeExpirationForFiber,

55 scheduleWork,

56 } from './ReactFiberWorkLoop';

@@ -183,7 +183,7 @@ const classComponentUpdater = {

183 isMounted,

184 enqueueSetState(inst, payload, callback) {

185 const fiber = getInstance(inst);

186 - const currentTime = requestCurrentTime();

186 + const currentTime = requestCurrentTimeForUpdate();

187 const suspenseConfig = requestCurrentSuspenseConfig();

188 const expirationTime = computeExpirationForFiber(

189 currentTime,

@@ -205,7 +205,7 @@ const classComponentUpdater = {

205 },

206 enqueueReplaceState(inst, payload, callback) {

207 const fiber = getInstance(inst);

208 - const currentTime = requestCurrentTime();

208 + const currentTime = requestCurrentTimeForUpdate();

209 const suspenseConfig = requestCurrentSuspenseConfig();

210 const expirationTime = computeExpirationForFiber(

211 currentTime,

@@ -229,7 +229,7 @@ const classComponentUpdater = {

229 },

230 enqueueForceUpdate(inst, callback) {

231 const fiber = getInstance(inst);

232 - const currentTime = requestCurrentTime();

232 + const currentTime = requestCurrentTimeForUpdate();

233 const suspenseConfig = requestCurrentSuspenseConfig();

234 const expirationTime = computeExpirationForFiber(

235 currentTime,

packages/react-reconciler/src/ReactFiberDevToolsHook.js

+2 -2

@@ -8,7 +8,7 @@

8 */

10 import {enableProfilerTimer} from 'shared/ReactFeatureFlags';

11 -import {requestCurrentTime} from './ReactFiberWorkLoop';

11 +import {getCurrentTime} from './ReactFiberWorkLoop';

12 import {inferPriorityFromExpirationTime} from './ReactFiberExpirationTime';

14 import type {Fiber} from './ReactFiber';

@@ -58,7 +58,7 @@ export function injectInternals(internals: Object): boolean {

58 try {

59 const didError = (root.current.effectTag & DidCapture) === DidCapture;

60 if (enableProfilerTimer) {

61 - const currentTime = requestCurrentTime();

61 + const currentTime = getCurrentTime();

62 const priorityLevel = inferPriorityFromExpirationTime(

63 currentTime,

64 expirationTime,

packages/react-reconciler/src/ReactFiberHooks.js

+2 -2

@@ -39,7 +39,7 @@ import {

39 import {

40 scheduleWork,

41 computeExpirationForFiber,

42 - requestCurrentTime,

42 + requestCurrentTimeForUpdate,

43 warnIfNotCurrentlyActingEffectsInDEV,

44 warnIfNotCurrentlyActingUpdatesInDev,

45 warnIfNotScopedWithMatchingAct,

@@ -1273,7 +1273,7 @@ function dispatchAction<S, A>(

1273 lastRenderPhaseUpdate.next = update;

1274 }

1275 } else {

1276 - const currentTime = requestCurrentTime();

1276 + const currentTime = requestCurrentTimeForUpdate();

1277 const suspenseConfig = requestCurrentSuspenseConfig();

1278 const expirationTime = computeExpirationForFiber(

1279 currentTime,

packages/react-reconciler/src/ReactFiberReconciler.js

+10 -6

@@ -50,7 +50,7 @@ import {

50 import {createFiberRoot} from './ReactFiberRoot';

51 import {injectInternals} from './ReactFiberDevToolsHook';

52 import {

53 - requestCurrentTime,

53 + requestCurrentTimeForUpdate,

54 computeExpirationForFiber,

55 scheduleWork,

56 flushRoot,

@@ -231,7 +231,7 @@ export function updateContainer(

231 callback: ?Function,

232 ): ExpirationTime {

233 const current = container.current;

234 - const currentTime = requestCurrentTime();

234 + const currentTime = requestCurrentTimeForUpdate();

235 if (__DEV__) {

236 // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests

237 if ('undefined' !== typeof jest) {

@@ -348,7 +348,9 @@ export function attemptSynchronousHydration(fiber: Fiber): void {

348 // If we're still blocked after this, we need to increase

349 // the priority of any promises resolving within this

350 // boundary so that they next attempt also has higher pri.

351 - let retryExpTime = computeInteractiveExpiration(requestCurrentTime());

351 + let retryExpTime = computeInteractiveExpiration(

352 + requestCurrentTimeForUpdate(),

353 + );

354 markRetryTimeIfNotHydrated(fiber, retryExpTime);

355 break;

356 }

@@ -380,7 +382,7 @@ export function attemptUserBlockingHydration(fiber: Fiber): void {

382 // Suspense.

383 return;

384 }

383 - let expTime = computeInteractiveExpiration(requestCurrentTime());

385 + let expTime = computeInteractiveExpiration(requestCurrentTimeForUpdate());

386 scheduleWork(fiber, expTime);

387 markRetryTimeIfNotHydrated(fiber, expTime);

388 }

@@ -393,7 +395,9 @@ export function attemptContinuousHydration(fiber: Fiber): void {

395 // Suspense.

396 return;

397 }

396 - let expTime = computeContinuousHydrationExpiration(requestCurrentTime());

398 + let expTime = computeContinuousHydrationExpiration(

399 + requestCurrentTimeForUpdate(),

400 + );

401 scheduleWork(fiber, expTime);

402 markRetryTimeIfNotHydrated(fiber, expTime);

403 }

@@ -404,7 +408,7 @@ export function attemptHydrationAtCurrentPriority(fiber: Fiber): void {

408 // their priority other than synchronously flush it.

409 return;

410 }

407 - const currentTime = requestCurrentTime();

411 + const currentTime = requestCurrentTimeForUpdate();

412 const expTime = computeExpirationForFiber(currentTime, fiber, null);

413 scheduleWork(fiber, expTime);

414 markRetryTimeIfNotHydrated(fiber, expTime);

packages/react-reconciler/src/ReactFiberWorkLoop.js

+8 -4

@@ -288,7 +288,7 @@ let spawnedWorkDuringRender: null | Array<ExpirationTime> = null;

288 // receive the same expiration time. Otherwise we get tearing.

289 let currentEventTime: ExpirationTime = NoWork;

290

291 -export function requestCurrentTime() {

291 +export function requestCurrentTimeForUpdate() {

292 if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {

293 // We're inside React, so it's fine to read the actual time.

294 return msToExpirationTime(now());

@@ -303,6 +303,10 @@ export function requestCurrentTime() {

303 return currentEventTime;

304 }

305

306 +export function getCurrentTime() {

307 + return msToExpirationTime(now());

308 +}

309 +

310 export function computeExpirationForFiber(

311 currentTime: ExpirationTime,

312 fiber: Fiber,

@@ -571,7 +575,7 @@ function ensureRootIsScheduled(root: FiberRoot) {

575

576 // TODO: If this is an update, we already read the current time. Pass the

577 // time as an argument.

574 - const currentTime = requestCurrentTime();

578 + const currentTime = requestCurrentTimeForUpdate();

579 const priorityLevel = inferPriorityFromExpirationTime(

580 currentTime,

581 expirationTime,

@@ -632,7 +636,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {

636 if (didTimeout) {

637 // The render task took too long to complete. Mark the current time as

638 // expired to synchronously render all expired work in a single batch.

635 - const currentTime = requestCurrentTime();

639 + const currentTime = requestCurrentTimeForUpdate();

640 markRootExpiredAtTime(root, currentTime);

641 // This will schedule a synchronous callback.

642 ensureRootIsScheduled(root);

@@ -2380,7 +2384,7 @@ function retryTimedOutBoundary(

2384 // likely unblocked. Try rendering again, at a new expiration time.

2385 if (retryTime === NoWork) {

2386 const suspenseConfig = null; // Retries don't carry over the already committed update.

2383 - const currentTime = requestCurrentTime();

2387 + const currentTime = requestCurrentTimeForUpdate();

2388 retryTime = computeExpirationForFiber(

2389 currentTime,

2390 boundaryFiber,

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

+26

@@ -177,4 +177,30 @@ describe('ReactProfiler DevTools integration', () => {

177 {name: 'some event', timestamp: eventTime},

178 ]);

179 });

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 + });

206 });