Try rendering again if a timed out tree receives an update (#13921)
Found a bug related to suspending inside an already mounted tree. While investigating this I noticed we really don't have much coverage of suspended updates. I think this would greatly benefit from some fuzz testing; still haven't thought of a good test case, though.
Andrew Clark committed
Oct 22, 2018 at 22:37 UTC
55444a6f490ce84550ba664b9a82f1724134fb76
5 files changed
+224
-135
packages/jest-react/src/JestReact.js
+5
-7
@@ -35,19 +35,17 @@ function assertYieldsWereCleared(root) {
35
}
36
37
export function toFlushAndYield(root, expectedYields) {
38
+ assertYieldsWereCleared(root);
39
+ const actualYields = root.unstable_flushAll();
40
return captureAssertion(() => {
39
- assertYieldsWereCleared(root);
40
- const actualYields = root.unstable_flushAll();
41
expect(actualYields).toEqual(expectedYields);
42
});
43
}
44
45
export function toFlushAndYieldThrough(root, expectedYields) {
46
+ assertYieldsWereCleared(root);
47
+ const actualYields = root.unstable_flushNumberOfYields(expectedYields.length);
48
return captureAssertion(() => {
47
- assertYieldsWereCleared(root);
48
- const actualYields = root.unstable_flushNumberOfYields(
49
- expectedYields.length,
50
- );
49
expect(actualYields).toEqual(expectedYields);
50
});
51
}
@@ -76,8 +74,8 @@ export function toHaveYielded(ReactTestRenderer, expectedYields) {
74
}
75
76
export function toFlushAndThrow(root, ...rest) {
77
+ assertYieldsWereCleared(root);
78
return captureAssertion(() => {
80
- assertYieldsWereCleared(root);
79
expect(() => {
80
root.unstable_flushAll();
81
}).toThrow(...rest);
packages/react-reconciler/src/ReactFiberBeginWork.js
+39
-20
@@ -1018,20 +1018,16 @@ function updateSuspenseComponent(
1018
// We should attempt to render the primary children unless this boundary
1019
// already suspended during this render (`alreadyCaptured` is true).
1020
let nextState: SuspenseState | null = workInProgress.memoizedState;
1021
- let nextDidTimeout;
1021
if (nextState === null) {
1022
// An empty suspense state means this boundary has not yet timed out.
1024
- nextDidTimeout = false;
1023
} else {
1024
if (!nextState.alreadyCaptured) {
1025
// Since we haven't already suspended during this commit, clear the
1026
// existing suspense state. We'll try rendering again.
1029
- nextDidTimeout = false;
1027
nextState = null;
1028
} else {
1029
// Something in this boundary's subtree already suspended. Switch to
1030
// rendering the fallback children. Set `alreadyCaptured` to true.
1034
- nextDidTimeout = true;
1031
if (current !== null && nextState === current.memoizedState) {
1032
// Create a new suspense state to avoid mutating the current tree's.
1033
nextState = {
@@ -1046,6 +1042,7 @@ function updateSuspenseComponent(
1042
}
1043
}
1044
}
1045
+ const nextDidTimeout = nextState !== null && nextState.didTimeout;
1046
1047
// This next part is a bit confusing. If the children timeout, we switch to
1048
// showing the fallback children in place of the "primary" children.
@@ -1127,8 +1124,8 @@ function updateSuspenseComponent(
1124
// its fragment. We're going to skip over these entirely.
1125
const nextFallbackChildren = nextProps.fallback;
1126
const primaryChildFragment = createWorkInProgress(
1130
- currentFallbackChildFragment,
1131
- currentFallbackChildFragment.pendingProps,
1127
+ currentPrimaryChildFragment,
1128
+ currentPrimaryChildFragment.pendingProps,
1129
NoWork,
1130
);
1131
primaryChildFragment.effectTag |= Placement;
@@ -1481,23 +1478,45 @@ function beginWork(
1478
}
1479
break;
1480
case SuspenseComponent: {
1484
- const child = bailoutOnAlreadyFinishedWork(
1485
- current,
1486
- workInProgress,
1487
- renderExpirationTime,
1488
- );
1489
- if (child !== null) {
1490
- const nextState = workInProgress.memoizedState;
1491
- const nextDidTimeout = nextState !== null && nextState.didTimeout;
1492
- if (nextDidTimeout) {
1493
- child.childExpirationTime = NoWork;
1494
- return child.sibling;
1481
+ const state: SuspenseState | null = workInProgress.memoizedState;
1482
+ const didTimeout = state !== null && state.didTimeout;
1483
+ if (didTimeout) {
1484
+ // If this boundary is currently timed out, we need to decide
1485
+ // whether to retry the primary children, or to skip over it and
1486
+ // go straight to the fallback. Check the priority of the primary
1487
+ // child fragment.
1488
+ const primaryChildFragment: Fiber = (workInProgress.child: any);
1489
+ const primaryChildExpirationTime =
1490
+ primaryChildFragment.childExpirationTime;
1491
+ if (
1492
+ primaryChildExpirationTime !== NoWork &&
1493
+ primaryChildExpirationTime <= renderExpirationTime
1494
+ ) {
1495
+ // The primary children have pending work. Use the normal path
1496
+ // to attempt to render the primary children again.
1497
+ return updateSuspenseComponent(
1498
+ current,
1499
+ workInProgress,
1500
+ renderExpirationTime,
1501
+ );
1502
} else {
1496
- return child;
1503
+ // The primary children do not have pending work with sufficient
1504
+ // priority. Bailout.
1505
+ const child = bailoutOnAlreadyFinishedWork(
1506
+ current,
1507
+ workInProgress,
1508
+ renderExpirationTime,
1509
+ );
1510
+ if (child !== null) {
1511
+ // The fallback children have pending work. Skip over the
1512
+ // primary children and work on the fallback.
1513
+ return child.sibling;
1514
+ } else {
1515
+ return null;
1516
+ }
1517
}
1498
- } else {
1499
- return null;
1518
}
1519
+ break;
1520
}
1521
}
1522
return bailoutOnAlreadyFinishedWork(
packages/react-reconciler/src/ReactFiberSuspenseComponent.js
+15
@@ -7,6 +7,7 @@
7
* @flow
8
*/
9
10
+import type {Fiber} from './ReactFiber';
11
import type {ExpirationTime} from './ReactFiberExpirationTime';
12
13
export type SuspenseState = {|
@@ -23,3 +24,17 @@ export type SuspenseState = {|
24
// `didTimeout` because it's not set unless the boundary actually commits.
25
timedOutAt: ExpirationTime,
26
|};
27
+
28
+export function shouldCaptureSuspense(
29
+ current: Fiber | null,
30
+ workInProgress: Fiber,
31
+): boolean {
32
+ // In order to capture, the Suspense component must have a fallback prop.
33
+ if (workInProgress.memoizedProps.fallback === undefined) {
34
+ return false;
35
+ }
36
+ // If it was the primary children that just suspended, capture and render the
37
+ // fallback. Otherwise, don't capture and bubble to the next boundary.
38
+ const nextState: SuspenseState | null = workInProgress.memoizedState;
39
+ return nextState === null || !nextState.didTimeout;
40
+}
packages/react-reconciler/src/ReactFiberUnwindWork.js
+103
-103
@@ -13,6 +13,7 @@ import type {ExpirationTime} from './ReactFiberExpirationTime';
13
import type {CapturedValue} from './ReactCapturedValue';
14
import type {Update} from './ReactUpdateQueue';
15
import type {Thenable} from './ReactFiberScheduler';
16
+import type {SuspenseState} from './ReactFiberSuspenseComponent';
17
18
import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';
19
import getComponentName from 'shared/getComponentName';
@@ -35,6 +36,7 @@ import {
36
} from 'shared/ReactSideEffectTags';
37
import {enableSchedulerTracing} from 'shared/ReactFeatureFlags';
38
import {ConcurrentMode} from './ReactTypeOfMode';
39
+import {shouldCaptureSuspense} from './ReactFiberSuspenseComponent';
40
41
import {createCapturedValue} from './ReactCapturedValue';
42
import {
@@ -170,7 +172,7 @@ function throwException(
172
if (workInProgress.tag === SuspenseComponent) {
173
const current = workInProgress.alternate;
174
if (current !== null) {
173
- const currentState = current.memoizedState;
175
+ const currentState: SuspenseState | null = current.memoizedState;
176
if (currentState !== null && currentState.didTimeout) {
177
// Reached a boundary that already timed out. Do not search
178
// any further.
@@ -198,116 +200,113 @@ function throwException(
200
// Schedule the nearest Suspense to re-render the timed out view.
201
workInProgress = returnFiber;
202
do {
201
- if (workInProgress.tag === SuspenseComponent) {
202
- const didTimeout = workInProgress.memoizedState;
203
- if (
204
- !didTimeout &&
205
- workInProgress.memoizedProps.fallback !== undefined
206
- ) {
207
- // Found the nearest boundary.
203
+ if (
204
+ workInProgress.tag === SuspenseComponent &&
205
+ shouldCaptureSuspense(workInProgress.alternate, workInProgress)
206
+ ) {
207
+ // Found the nearest boundary.
208
209
- // If the boundary is not in concurrent mode, we should not suspend, and
210
- // likewise, when the promise resolves, we should ping synchronously.
211
- const pingTime =
212
- (workInProgress.mode & ConcurrentMode) === NoEffect
213
- ? Sync
214
- : renderExpirationTime;
209
+ // If the boundary is not in concurrent mode, we should not suspend, and
210
+ // likewise, when the promise resolves, we should ping synchronously.
211
+ const pingTime =
212
+ (workInProgress.mode & ConcurrentMode) === NoEffect
213
+ ? Sync
214
+ : renderExpirationTime;
215
216
- // Attach a listener to the promise to "ping" the root and retry.
217
- let onResolveOrReject = retrySuspendedRoot.bind(
218
- null,
219
- root,
220
- workInProgress,
221
- sourceFiber,
222
- pingTime,
223
- );
224
- if (enableSchedulerTracing) {
225
- onResolveOrReject = Schedule_tracing_wrap(onResolveOrReject);
226
- }
227
- thenable.then(onResolveOrReject, onResolveOrReject);
216
+ // Attach a listener to the promise to "ping" the root and retry.
217
+ let onResolveOrReject = retrySuspendedRoot.bind(
218
+ null,
219
+ root,
220
+ workInProgress,
221
+ sourceFiber,
222
+ pingTime,
223
+ );
224
+ if (enableSchedulerTracing) {
225
+ onResolveOrReject = Schedule_tracing_wrap(onResolveOrReject);
226
+ }
227
+ thenable.then(onResolveOrReject, onResolveOrReject);
228
229
- // If the boundary is outside of concurrent mode, we should *not*
230
- // suspend the commit. Pretend as if the suspended component rendered
231
- // null and keep rendering. In the commit phase, we'll schedule a
232
- // subsequent synchronous update to re-render the Suspense.
233
- //
234
- // Note: It doesn't matter whether the component that suspended was
235
- // inside a concurrent mode tree. If the Suspense is outside of it, we
236
- // should *not* suspend the commit.
237
- if ((workInProgress.mode & ConcurrentMode) === NoEffect) {
238
- workInProgress.effectTag |= CallbackEffect;
229
+ // If the boundary is outside of concurrent mode, we should *not*
230
+ // suspend the commit. Pretend as if the suspended component rendered
231
+ // null and keep rendering. In the commit phase, we'll schedule a
232
+ // subsequent synchronous update to re-render the Suspense.
233
+ //
234
+ // Note: It doesn't matter whether the component that suspended was
235
+ // inside a concurrent mode tree. If the Suspense is outside of it, we
236
+ // should *not* suspend the commit.
237
+ if ((workInProgress.mode & ConcurrentMode) === NoEffect) {
238
+ workInProgress.effectTag |= CallbackEffect;
239
240
- // Unmount the source fiber's children
241
- const nextChildren = null;
242
- reconcileChildren(
243
- sourceFiber.alternate,
244
- sourceFiber,
245
- nextChildren,
246
- renderExpirationTime,
247
- );
248
- sourceFiber.effectTag &= ~Incomplete;
240
+ // Unmount the source fiber's children
241
+ const nextChildren = null;
242
+ reconcileChildren(
243
+ sourceFiber.alternate,
244
+ sourceFiber,
245
+ nextChildren,
246
+ renderExpirationTime,
247
+ );
248
+ sourceFiber.effectTag &= ~Incomplete;
249
250
- if (sourceFiber.tag === ClassComponent) {
251
- // We're going to commit this fiber even though it didn't
252
- // complete. But we shouldn't call any lifecycle methods or
253
- // callbacks. Remove all lifecycle effect tags.
254
- sourceFiber.effectTag &= ~LifecycleEffectMask;
255
- if (sourceFiber.alternate === null) {
256
- // Set the instance back to null. We use this as a heuristic to
257
- // detect that the fiber mounted in an inconsistent state.
258
- sourceFiber.stateNode = null;
259
- }
250
+ if (sourceFiber.tag === ClassComponent) {
251
+ // We're going to commit this fiber even though it didn't complete.
252
+ // But we shouldn't call any lifecycle methods or callbacks. Remove
253
+ // all lifecycle effect tags.
254
+ sourceFiber.effectTag &= ~LifecycleEffectMask;
255
+ if (sourceFiber.alternate === null) {
256
+ // Set the instance back to null. We use this as a heuristic to
257
+ // detect that the fiber mounted in an inconsistent state.
258
+ sourceFiber.stateNode = null;
259
}
261
-
262
- // Exit without suspending.
263
- return;
260
}
261
266
- // Confirmed that the boundary is in a concurrent mode tree. Continue
267
- // with the normal suspend path.
262
+ // Exit without suspending.
263
+ return;
264
+ }
265
269
- let absoluteTimeoutMs;
270
- if (earliestTimeoutMs === -1) {
271
- // If no explicit threshold is given, default to an abitrarily large
272
- // value. The actual size doesn't matter because the threshold for the
273
- // whole tree will be clamped to the expiration time.
274
- absoluteTimeoutMs = maxSigned31BitInt;
275
- } else {
276
- if (startTimeMs === -1) {
277
- // This suspend happened outside of any already timed-out
278
- // placeholders. We don't know exactly when the update was scheduled,
279
- // but we can infer an approximate start time from the expiration
280
- // time. First, find the earliest uncommitted expiration time in the
281
- // tree, including work that is suspended. Then subtract the offset
282
- // used to compute an async update's expiration time. This will cause
283
- // high priority (interactive) work to expire earlier than necessary,
284
- // but we can account for this by adjusting for the Just Noticeable
285
- // Difference.
286
- const earliestExpirationTime = findEarliestOutstandingPriorityLevel(
287
- root,
288
- renderExpirationTime,
289
- );
290
- const earliestExpirationTimeMs = expirationTimeToMs(
291
- earliestExpirationTime,
292
- );
293
- startTimeMs = earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION;
294
- }
295
- absoluteTimeoutMs = startTimeMs + earliestTimeoutMs;
266
+ // Confirmed that the boundary is in a concurrent mode tree. Continue
267
+ // with the normal suspend path.
268
+
269
+ let absoluteTimeoutMs;
270
+ if (earliestTimeoutMs === -1) {
271
+ // If no explicit threshold is given, default to an abitrarily large
272
+ // value. The actual size doesn't matter because the threshold for the
273
+ // whole tree will be clamped to the expiration time.
274
+ absoluteTimeoutMs = maxSigned31BitInt;
275
+ } else {
276
+ if (startTimeMs === -1) {
277
+ // This suspend happened outside of any already timed-out
278
+ // placeholders. We don't know exactly when the update was
279
+ // scheduled, but we can infer an approximate start time from the
280
+ // expiration time. First, find the earliest uncommitted expiration
281
+ // time in the tree, including work that is suspended. Then subtract
282
+ // the offset used to compute an async update's expiration time.
283
+ // This will cause high priority (interactive) work to expire
284
+ // earlier than necessary, but we can account for this by adjusting
285
+ // for the Just Noticeable Difference.
286
+ const earliestExpirationTime = findEarliestOutstandingPriorityLevel(
287
+ root,
288
+ renderExpirationTime,
289
+ );
290
+ const earliestExpirationTimeMs = expirationTimeToMs(
291
+ earliestExpirationTime,
292
+ );
293
+ startTimeMs = earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION;
294
}
295
+ absoluteTimeoutMs = startTimeMs + earliestTimeoutMs;
296
+ }
297
298
- // Mark the earliest timeout in the suspended fiber's ancestor path.
299
- // After completing the root, we'll take the largest of all the
300
- // suspended fiber's timeouts and use it to compute a timeout for the
301
- // whole tree.
302
- renderDidSuspend(root, absoluteTimeoutMs, renderExpirationTime);
298
+ // Mark the earliest timeout in the suspended fiber's ancestor path.
299
+ // After completing the root, we'll take the largest of all the
300
+ // suspended fiber's timeouts and use it to compute a timeout for the
301
+ // whole tree.
302
+ renderDidSuspend(root, absoluteTimeoutMs, renderExpirationTime);
303
304
- workInProgress.effectTag |= ShouldCapture;
305
- workInProgress.expirationTime = renderExpirationTime;
306
- return;
307
- }
308
- // This boundary already captured during this render. Continue to the
309
- // next boundary.
304
+ workInProgress.effectTag |= ShouldCapture;
305
+ workInProgress.expirationTime = renderExpirationTime;
306
+ return;
307
}
308
+ // This boundary already captured during this render. Continue to the next
309
+ // boundary.
310
workInProgress = workInProgress.return;
311
} while (workInProgress !== null);
312
// No boundary was found. Fallthrough to error mode.
@@ -407,9 +406,10 @@ function unwindWork(
406
// Captured a suspense effect. Set the boundary's `alreadyCaptured`
407
// state to true so we know to render the fallback.
408
const current = workInProgress.alternate;
410
- const currentState = current !== null ? current.memoizedState : null;
411
- let nextState = workInProgress.memoizedState;
412
- if (currentState === null) {
409
+ const currentState: SuspenseState | null =
410
+ current !== null ? current.memoizedState : null;
411
+ let nextState: SuspenseState | null = workInProgress.memoizedState;
412
+ if (nextState === null) {
413
// No existing state. Create a new object.
414
nextState = {
415
alreadyCaptured: true,
@@ -421,8 +421,8 @@ function unwindWork(
421
// Clone the object.
422
nextState = {
423
alreadyCaptured: true,
424
- didTimeout: currentState.didTimeout,
425
- timedOutAt: currentState.timedOutAt,
424
+ didTimeout: nextState.didTimeout,
425
+ timedOutAt: nextState.timedOutAt,
426
};
427
} else {
428
// Already have a clone, so it's safe to mutate.
packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
+62
-5
@@ -442,7 +442,7 @@ describe('ReactSuspense', () => {
442
state = {step: 1};
443
render() {
444
instance = this;
445
- return <Text text="Stateful" />;
445
+ return <Text text={`Stateful: ${this.state.step}`} />;
446
}
447
}
448
@@ -458,30 +458,87 @@ describe('ReactSuspense', () => {
458
const root = ReactTestRenderer.create(<App text="A" />);
459
460
expect(ReactTestRenderer).toHaveYielded([
461
- 'Stateful',
461
+ 'Stateful: 1',
462
'Suspend! [A]',
463
'Loading...',
464
]);
465
466
jest.advanceTimersByTime(1000);
467
expect(ReactTestRenderer).toHaveYielded(['Promise resolved [A]', 'A']);
468
- expect(root).toMatchRenderedOutput('StatefulA');
468
+ expect(root).toMatchRenderedOutput('Stateful: 1A');
469
470
root.update(<App text="B" />);
471
expect(ReactTestRenderer).toHaveYielded([
472
- 'Stateful',
472
+ 'Stateful: 1',
473
'Suspend! [B]',
474
'Loading...',
475
]);
476
+ expect(root).toMatchRenderedOutput('Loading...');
477
478
instance.setState({step: 2});
479
+ expect(ReactTestRenderer).toHaveYielded([
480
+ 'Stateful: 2',
481
+ 'Suspend! [B]',
482
+ 'Loading...',
483
+ ]);
484
+ expect(root).toMatchRenderedOutput('Loading...');
485
486
jest.advanceTimersByTime(1000);
487
expect(ReactTestRenderer).toHaveYielded([
488
'Promise resolved [B]',
482
- 'Stateful',
489
'B',
490
+ 'B',
491
+ ]);
492
+ expect(root).toMatchRenderedOutput('Stateful: 2B');
493
+ });
494
+
495
+ it('retries when an update is scheduled on a timed out tree', () => {
496
+ let instance;
497
+ class Stateful extends React.Component {
498
+ state = {step: 1};
499
+ render() {
500
+ instance = this;
501
+ return <AsyncText ms={1000} text={`Step: ${this.state.step}`} />;
502
+ }
503
+ }
504
+
505
+ function App(props) {
506
+ return (
507
+ <Suspense maxDuration={10} fallback={<Text text="Loading..." />}>
508
+ <Stateful />
509
+ </Suspense>
510
+ );
511
+ }
512
+
513
+ const root = ReactTestRenderer.create(<App />, {
514
+ unstable_isConcurrent: true,
515
+ });
516
+
517
+ // Initial render
518
+ expect(root).toFlushAndYield(['Suspend! [Step: 1]', 'Loading...']);
519
+ jest.advanceTimersByTime(1000);
520
+ expect(ReactTestRenderer).toHaveYielded(['Promise resolved [Step: 1]']);
521
+ expect(root).toFlushAndYield(['Step: 1']);
522
+ expect(root).toMatchRenderedOutput('Step: 1');
523
+
524
+ // Update that suspends
525
+ instance.setState({step: 2});
526
+ expect(root).toFlushAndYield(['Suspend! [Step: 2]', 'Loading...']);
527
+ jest.advanceTimersByTime(500);
528
+ expect(root).toMatchRenderedOutput('Loading...');
529
+
530
+ // Update while still suspended
531
+ instance.setState({step: 3});
532
+ expect(root).toFlushAndYield(['Suspend! [Step: 3]']);
533
+ expect(root).toMatchRenderedOutput('Loading...');
534
+
535
+ jest.advanceTimersByTime(1000);
536
+ expect(ReactTestRenderer).toHaveYielded([
537
+ 'Promise resolved [Step: 2]',
538
+ 'Promise resolved [Step: 3]',
539
]);
540
+ expect(root).toFlushAndYield(['Step: 3']);
541
+ expect(root).toMatchRenderedOutput('Step: 3');
542
});
543
});
544
});