Bugfix: Expired partial tree infinite loops (#17949)
* Bugfix: Expiring a partially completed tree (#17926) * Failing test: Expiring a partially completed tree We should not throw out a partially completed tree if it expires in the middle of rendering. We should finish the rest of the tree without yielding, then finish any remaining expired levels in a single batch. * Check if there's a partial tree before restarting If a partial render expires, we should stay in the concurrent path (performConcurrentWorkOnRoot); we'll stop yielding, but the rest of the behavior remains the same. We will only revert to the sync path (performSyncWorkOnRoot) when starting on a new level. This approach prevents partially completed concurrent work from being discarded. * New test: retry after error during expired render * Regression: Expired partial tree infinite loops Adds regression tests that reproduce a scenario where a partially completed tree expired then fell into an infinite loop. The code change that exposed this bug made the assumption that if you call Scheduler's `shouldYield` from inside an expired task, Scheduler will always return `false`. But in fact, Scheduler sometimes returns `true` in that scenario, which is a bug. The reason it worked before is that once a task timed out, React would always switch to a synchronous work loop without checking `shouldYield`. My rationale for relying on `shouldYield` was to unify the code paths between a partially concurrent render (i.e. expires midway through) and a fully concurrent render, as opposed to a render that was synchronous the whole time. However, this bug indicates that we need a stronger guarantee within React for when tasks expire, given that the failure case is so catastrophic. Instead of relying on the result of a dynamic method call, we should use control flow to guarantee that the work is synchronously executed. (We should also fix the Scheduler bug so that `shouldYield` always returns false inside an expired task, but I'll address that separately.) * Always switch to sync work loop when task expires Refactors the `didTimeout` check so that it always switches to the synchronous work loop, like it did before the regression. This breaks the error handling behavior that I added in 5f7361f (an error during a partially concurrent render should retry once, synchronously). I'll fix this next. I need to change that behavior, anyway, to support retries that occur as a result of `flushSync`. * Retry once after error even for sync renders Except in legacy mode. This is to support the `useOpaqueReference` hook, which uses an error to trigger a retry at lower priority. * Move partial tree check to performSyncWorkOnRoot * Factor out render phase Splits the work loop and its surrounding enter/exit code into their own functions. Now we can do perform multiple render phase passes within a single call to performConcurrentWorkOnRoot or performSyncWorkOnRoot. This lets us get rid of the `didError` field.
Andrew Clark committed
Mar 3, 2020 at 13:42 UTC
ec652f4daf7245e9ff4ca9b57b020c4026164eba
5 files changed
+494
-153
packages/react-reconciler/src/ReactFiberWorkLoop.js
+184
-149
@@ -90,6 +90,7 @@ import {
90
SimpleMemoComponent,
91
Block,
92
} from 'shared/ReactWorkTags';
93
+import {LegacyRoot} from 'shared/ReactRootTags';
94
import {
95
NoEffect,
96
PerformedWork,
@@ -643,6 +644,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
644
// event time. The next update will compute a new event time.
645
currentEventTime = NoWork;
646
647
+ // Check if the render expired.
648
if (didTimeout) {
649
// The render task took too long to complete. Mark the current time as
650
// expired to synchronously render all expired work in a single batch.
@@ -655,84 +657,52 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
657
658
// Determine the next expiration time to work on, using the fields stored
659
// on the root.
658
- const expirationTime = getNextRootExpirationTimeToWorkOn(root);
659
- if (expirationTime !== NoWork) {
660
- const originalCallbackNode = root.callbackNode;
661
- invariant(
662
- (executionContext & (RenderContext | CommitContext)) === NoContext,
663
- 'Should not already be working.',
664
- );
665
-
666
- flushPassiveEffects();
660
+ let expirationTime = getNextRootExpirationTimeToWorkOn(root);
661
+ if (expirationTime === NoWork) {
662
+ return null;
663
+ }
664
+ const originalCallbackNode = root.callbackNode;
665
+ invariant(
666
+ (executionContext & (RenderContext | CommitContext)) === NoContext,
667
+ 'Should not already be working.',
668
+ );
669
668
- // If the root or expiration time have changed, throw out the existing stack
669
- // and prepare a fresh one. Otherwise we'll continue where we left off.
670
- if (
671
- root !== workInProgressRoot ||
672
- expirationTime !== renderExpirationTime
673
- ) {
674
- prepareFreshStack(root, expirationTime);
675
- startWorkOnPendingInteractions(root, expirationTime);
676
- }
677
-
678
- // If we have a work-in-progress fiber, it means there's still work to do
679
- // in this root.
680
- if (workInProgress !== null) {
681
- const prevExecutionContext = executionContext;
682
- executionContext |= RenderContext;
683
- const prevDispatcher = pushDispatcher(root);
684
- const prevInteractions = pushInteractions(root);
685
- startWorkLoopTimer(workInProgress);
686
- do {
687
- try {
688
- workLoopConcurrent();
689
- break;
690
- } catch (thrownValue) {
691
- handleError(root, thrownValue);
692
- }
693
- } while (true);
694
- resetContextDependencies();
695
- executionContext = prevExecutionContext;
696
- popDispatcher(prevDispatcher);
697
- if (enableSchedulerTracing) {
698
- popInteractions(((prevInteractions: any): Set<Interaction>));
699
- }
670
+ flushPassiveEffects();
671
701
- if (workInProgressRootExitStatus === RootFatalErrored) {
702
- const fatalError = workInProgressRootFatalError;
703
- stopInterruptedWorkLoopTimer();
704
- prepareFreshStack(root, expirationTime);
705
- markRootSuspendedAtTime(root, expirationTime);
706
- ensureRootIsScheduled(root);
707
- throw fatalError;
708
- }
672
+ let exitStatus = renderRootConcurrent(root, expirationTime);
673
710
- if (workInProgress !== null) {
711
- // There's still work left over. Exit without committing.
712
- stopInterruptedWorkLoopTimer();
713
- } else {
714
- // We now have a consistent tree. The next step is either to commit it,
715
- // or, if something suspended, wait to commit it after a timeout.
716
- stopFinishedWorkLoopTimer();
717
-
718
- const finishedWork: Fiber = ((root.finishedWork =
719
- root.current.alternate): any);
720
- root.finishedExpirationTime = expirationTime;
721
- finishConcurrentRender(
722
- root,
723
- finishedWork,
724
- workInProgressRootExitStatus,
725
- expirationTime,
726
- );
727
- }
674
+ if (exitStatus !== RootIncomplete) {
675
+ if (exitStatus === RootErrored) {
676
+ // If something threw an error, try rendering one more time. We'll
677
+ // render synchronously to block concurrent data mutations, and we'll
678
+ // render at Idle (or lower) so that all pending updates are included.
679
+ // If it still fails after the second attempt, we'll give up and commit
680
+ // the resulting tree.
681
+ expirationTime = expirationTime > Idle ? Idle : expirationTime;
682
+ exitStatus = renderRootSync(root, expirationTime);
683
+ }
684
685
+ if (exitStatus === RootFatalErrored) {
686
+ const fatalError = workInProgressRootFatalError;
687
+ prepareFreshStack(root, expirationTime);
688
+ markRootSuspendedAtTime(root, expirationTime);
689
ensureRootIsScheduled(root);
730
- if (root.callbackNode === originalCallbackNode) {
731
- // The task node scheduled for this root is the same one that's
732
- // currently executed. Need to return a continuation.
733
- return performConcurrentWorkOnRoot.bind(null, root);
734
- }
690
+ throw fatalError;
691
}
692
+
693
+ // We now have a consistent tree. The next step is either to commit it,
694
+ // or, if something suspended, wait to commit it after a timeout.
695
+ const finishedWork: Fiber = ((root.finishedWork =
696
+ root.current.alternate): any);
697
+ root.finishedExpirationTime = expirationTime;
698
+ finishConcurrentRender(root, finishedWork, exitStatus, expirationTime);
699
+ }
700
+
701
+ ensureRootIsScheduled(root);
702
+ if (root.callbackNode === originalCallbackNode) {
703
+ // The task node scheduled for this root is the same one that's
704
+ // currently executed. Need to return a continuation.
705
+ return performConcurrentWorkOnRoot.bind(null, root);
706
}
707
return null;
708
}
@@ -743,9 +713,6 @@ function finishConcurrentRender(
713
exitStatus,
714
expirationTime,
715
) {
746
- // Set this to null to indicate there's no in-progress render.
747
- workInProgressRoot = null;
748
-
716
switch (exitStatus) {
717
case RootIncomplete:
718
case RootFatalErrored: {
@@ -755,19 +722,9 @@ function finishConcurrentRender(
722
// statement, but eslint doesn't know about invariant, so it complains
723
// if I do. eslint-disable-next-line no-fallthrough
724
case RootErrored: {
758
- // If this was an async render, the error may have happened due to
759
- // a mutation in a concurrent event. Try rendering one more time,
760
- // synchronously, to see if the error goes away. If there are
761
- // lower priority updates, let's include those, too, in case they
762
- // fix the inconsistency. Render at Idle to include all updates.
763
- // If it was Idle or Never or some not-yet-invented time, render
764
- // at that time.
765
- markRootExpiredAtTime(
766
- root,
767
- expirationTime > Idle ? Idle : expirationTime,
768
- );
769
- // We assume that this second render pass will be synchronous
770
- // and therefore not hit this path again.
725
+ // We should have already attempted to retry this tree. If we reached
726
+ // this point, it errored again. Commit it.
727
+ commitRoot(root);
728
break;
729
}
730
case RootSuspended: {
@@ -983,9 +940,6 @@ function finishConcurrentRender(
940
// This is the entry point for synchronous tasks that don't go
941
// through Scheduler
942
function performSyncWorkOnRoot(root) {
986
- // Check if there's expired work on this root. Otherwise, render at Sync.
987
- const lastExpiredTime = root.lastExpiredTime;
988
- const expirationTime = lastExpiredTime !== NoWork ? lastExpiredTime : Sync;
943
invariant(
944
(executionContext & (RenderContext | CommitContext)) === NoContext,
945
'Should not already be working.',
@@ -993,74 +947,61 @@ function performSyncWorkOnRoot(root) {
947
948
flushPassiveEffects();
949
996
- // If the root or expiration time have changed, throw out the existing stack
997
- // and prepare a fresh one. Otherwise we'll continue where we left off.
998
- if (root !== workInProgressRoot || expirationTime !== renderExpirationTime) {
999
- prepareFreshStack(root, expirationTime);
1000
- startWorkOnPendingInteractions(root, expirationTime);
1001
- }
1002
-
1003
- // If we have a work-in-progress fiber, it means there's still work to do
1004
- // in this root.
1005
- if (workInProgress !== null) {
1006
- const prevExecutionContext = executionContext;
1007
- executionContext |= RenderContext;
1008
- const prevDispatcher = pushDispatcher(root);
1009
- const prevInteractions = pushInteractions(root);
1010
- startWorkLoopTimer(workInProgress);
950
+ const lastExpiredTime = root.lastExpiredTime;
951
1012
- do {
1013
- try {
1014
- workLoopSync();
1015
- break;
1016
- } catch (thrownValue) {
1017
- handleError(root, thrownValue);
1018
- }
1019
- } while (true);
1020
- resetContextDependencies();
1021
- executionContext = prevExecutionContext;
1022
- popDispatcher(prevDispatcher);
1023
- if (enableSchedulerTracing) {
1024
- popInteractions(((prevInteractions: any): Set<Interaction>));
952
+ let expirationTime;
953
+ if (lastExpiredTime !== NoWork) {
954
+ // There's expired work on this root. Check if we have a partial tree
955
+ // that we can reuse.
956
+ if (
957
+ root === workInProgressRoot &&
958
+ renderExpirationTime >= lastExpiredTime
959
+ ) {
960
+ // There's a partial tree with equal or greater than priority than the
961
+ // expired level. Finish rendering it before rendering the rest of the
962
+ // expired work.
963
+ expirationTime = renderExpirationTime;
964
+ } else {
965
+ // Start a fresh tree.
966
+ expirationTime = lastExpiredTime;
967
}
968
+ } else {
969
+ // There's no expired work. This must be a new, synchronous render.
970
+ expirationTime = Sync;
971
+ }
972
1027
- if (workInProgressRootExitStatus === RootFatalErrored) {
1028
- const fatalError = workInProgressRootFatalError;
1029
- stopInterruptedWorkLoopTimer();
1030
- prepareFreshStack(root, expirationTime);
1031
- markRootSuspendedAtTime(root, expirationTime);
1032
- ensureRootIsScheduled(root);
1033
- throw fatalError;
1034
- }
973
+ let exitStatus = renderRootSync(root, expirationTime);
974
1036
- if (workInProgress !== null) {
1037
- // This is a sync render, so we should have finished the whole tree.
1038
- invariant(
1039
- false,
1040
- 'Cannot commit an incomplete root. This error is likely caused by a ' +
1041
- 'bug in React. Please file an issue.',
1042
- );
1043
- } else {
1044
- // We now have a consistent tree. Because this is a sync render, we
1045
- // will commit it even if something suspended.
1046
- stopFinishedWorkLoopTimer();
1047
- root.finishedWork = (root.current.alternate: any);
1048
- root.finishedExpirationTime = expirationTime;
1049
- finishSyncRender(root);
1050
- }
975
+ if (root.tag !== LegacyRoot && exitStatus === RootErrored) {
976
+ // If something threw an error, try rendering one more time. We'll
977
+ // render synchronously to block concurrent data mutations, and we'll
978
+ // render at Idle (or lower) so that all pending updates are included.
979
+ // If it still fails after the second attempt, we'll give up and commit
980
+ // the resulting tree.
981
+ expirationTime = expirationTime > Idle ? Idle : expirationTime;
982
+ exitStatus = renderRootSync(root, expirationTime);
983
+ }
984
1052
- // Before exiting, make sure there's a callback scheduled for the next
1053
- // pending level.
985
+ if (exitStatus === RootFatalErrored) {
986
+ const fatalError = workInProgressRootFatalError;
987
+ prepareFreshStack(root, expirationTime);
988
+ markRootSuspendedAtTime(root, expirationTime);
989
ensureRootIsScheduled(root);
990
+ throw fatalError;
991
}
992
1057
- return null;
1058
-}
993
+ // We now have a consistent tree. Because this is a sync render, we
994
+ // will commit it even if something suspended.
995
+ root.finishedWork = (root.current.alternate: any);
996
+ root.finishedExpirationTime = expirationTime;
997
1060
-function finishSyncRender(root) {
1061
- // Set this to null to indicate there's no in-progress render.
1062
- workInProgressRoot = null;
998
commitRoot(root);
999
+
1000
+ // Before exiting, make sure there's a callback scheduled for the next
1001
+ // pending level.
1002
+ ensureRootIsScheduled(root);
1003
+
1004
+ return null;
1005
}
1006
1007
export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) {
@@ -1449,6 +1390,53 @@ function inferTimeFromExpirationTimeWithSuspenseConfig(
1390
);
1391
}
1392
1393
+function renderRootSync(root, expirationTime) {
1394
+ const prevExecutionContext = executionContext;
1395
+ executionContext |= RenderContext;
1396
+ const prevDispatcher = pushDispatcher(root);
1397
+
1398
+ // If the root or expiration time have changed, throw out the existing stack
1399
+ // and prepare a fresh one. Otherwise we'll continue where we left off.
1400
+ if (root !== workInProgressRoot || expirationTime !== renderExpirationTime) {
1401
+ prepareFreshStack(root, expirationTime);
1402
+ startWorkOnPendingInteractions(root, expirationTime);
1403
+ }
1404
+
1405
+ const prevInteractions = pushInteractions(root);
1406
+ startWorkLoopTimer(workInProgress);
1407
+ do {
1408
+ try {
1409
+ workLoopSync();
1410
+ break;
1411
+ } catch (thrownValue) {
1412
+ handleError(root, thrownValue);
1413
+ }
1414
+ } while (true);
1415
+ resetContextDependencies();
1416
+ if (enableSchedulerTracing) {
1417
+ popInteractions(((prevInteractions: any): Set<Interaction>));
1418
+ }
1419
+
1420
+ executionContext = prevExecutionContext;
1421
+ popDispatcher(prevDispatcher);
1422
+
1423
+ if (workInProgress !== null) {
1424
+ // This is a sync render, so we should have finished the whole tree.
1425
+ invariant(
1426
+ false,
1427
+ 'Cannot commit an incomplete root. This error is likely caused by a ' +
1428
+ 'bug in React. Please file an issue.',
1429
+ );
1430
+ }
1431
+
1432
+ stopFinishedWorkLoopTimer();
1433
+
1434
+ // Set this to null to indicate there's no in-progress render.
1435
+ workInProgressRoot = null;
1436
+
1437
+ return workInProgressRootExitStatus;
1438
+}
1439
+
1440
// The work loop is an extremely hot path. Tell Closure not to inline it.
1441
/** @noinline */
1442
function workLoopSync() {
@@ -1458,6 +1446,53 @@ function workLoopSync() {
1446
}
1447
}
1448
1449
+function renderRootConcurrent(root, expirationTime) {
1450
+ const prevExecutionContext = executionContext;
1451
+ executionContext |= RenderContext;
1452
+ const prevDispatcher = pushDispatcher(root);
1453
+
1454
+ // If the root or expiration time have changed, throw out the existing stack
1455
+ // and prepare a fresh one. Otherwise we'll continue where we left off.
1456
+ if (root !== workInProgressRoot || expirationTime !== renderExpirationTime) {
1457
+ prepareFreshStack(root, expirationTime);
1458
+ startWorkOnPendingInteractions(root, expirationTime);
1459
+ }
1460
+
1461
+ const prevInteractions = pushInteractions(root);
1462
+ startWorkLoopTimer(workInProgress);
1463
+ do {
1464
+ try {
1465
+ workLoopConcurrent();
1466
+ break;
1467
+ } catch (thrownValue) {
1468
+ handleError(root, thrownValue);
1469
+ }
1470
+ } while (true);
1471
+ resetContextDependencies();
1472
+ if (enableSchedulerTracing) {
1473
+ popInteractions(((prevInteractions: any): Set<Interaction>));
1474
+ }
1475
+
1476
+ popDispatcher(prevDispatcher);
1477
+ executionContext = prevExecutionContext;
1478
+
1479
+ // Check if the tree has completed.
1480
+ if (workInProgress !== null) {
1481
+ // Still work remaining.
1482
+ stopInterruptedWorkLoopTimer();
1483
+ return RootIncomplete;
1484
+ } else {
1485
+ // Completed the tree.
1486
+ stopFinishedWorkLoopTimer();
1487
+
1488
+ // Set this to null to indicate there's no in-progress render.
1489
+ workInProgressRoot = null;
1490
+
1491
+ // Return the final exit status.
1492
+ return workInProgressRootExitStatus;
1493
+ }
1494
+}
1495
+
1496
/** @noinline */
1497
function workLoopConcurrent() {
1498
// Perform work until Scheduler asks us to yield
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
+74
-4
@@ -381,6 +381,56 @@ describe('ReactIncrementalErrorHandling', () => {
381
expect(ReactNoop.getChildren()).toEqual([]);
382
});
383
384
+ it('retries one more time if an error occurs during a render that expires midway through the tree', () => {
385
+ function Oops() {
386
+ Scheduler.unstable_yieldValue('Oops');
387
+ throw new Error('Oops');
388
+ }
389
+
390
+ function Text({text}) {
391
+ Scheduler.unstable_yieldValue(text);
392
+ return text;
393
+ }
394
+
395
+ function App() {
396
+ return (
397
+ <>
398
+ <Text text="A" />
399
+ <Text text="B" />
400
+ <Oops />
401
+ <Text text="C" />
402
+ <Text text="D" />
403
+ </>
404
+ );
405
+ }
406
+
407
+ ReactNoop.render(<App />);
408
+
409
+ // Render part of the tree
410
+ expect(Scheduler).toFlushAndYieldThrough(['A', 'B']);
411
+
412
+ // Expire the render midway through
413
+ Scheduler.unstable_advanceTime(10000);
414
+ expect(() => Scheduler.unstable_flushExpired()).toThrow('Oops');
415
+
416
+ expect(Scheduler).toHaveYielded([
417
+ // The render expired, but we shouldn't throw out the partial work.
418
+ // Finish the current level.
419
+ 'Oops',
420
+ 'C',
421
+ 'D',
422
+
423
+ // Since the error occured during a partially concurrent render, we should
424
+ // retry one more time, synchonrously.
425
+ 'A',
426
+ 'B',
427
+ 'Oops',
428
+ 'C',
429
+ 'D',
430
+ ]);
431
+ expect(ReactNoop.getChildren()).toEqual([]);
432
+ });
433
+
434
it('calls componentDidCatch multiple times for multiple errors', () => {
435
let id = 0;
436
class BadMount extends React.Component {
@@ -539,7 +589,12 @@ describe('ReactIncrementalErrorHandling', () => {
589
expect(ops).toEqual([
590
'ErrorBoundary render success',
591
'BrokenRender',
542
- // React doesn't retry because we're already rendering synchronously.
592
+
593
+ // React retries one more time
594
+ 'ErrorBoundary render success',
595
+ 'BrokenRender',
596
+
597
+ // Errored again on retry. Now handle it.
598
'ErrorBoundary componentDidCatch',
599
'ErrorBoundary render error',
600
]);
@@ -583,7 +638,12 @@ describe('ReactIncrementalErrorHandling', () => {
638
expect(ops).toEqual([
639
'ErrorBoundary render success',
640
'BrokenRender',
586
- // React doesn't retry because we're already rendering synchronously.
641
+
642
+ // React retries one more time
643
+ 'ErrorBoundary render success',
644
+ 'BrokenRender',
645
+
646
+ // Errored again on retry. Now handle it.
647
'ErrorBoundary componentDidCatch',
648
'ErrorBoundary render error',
649
]);
@@ -702,7 +762,12 @@ describe('ReactIncrementalErrorHandling', () => {
762
expect(ops).toEqual([
763
'RethrowErrorBoundary render',
764
'BrokenRender',
705
- // React doesn't retry because we're already rendering synchronously.
765
+
766
+ // React retries one more time
767
+ 'RethrowErrorBoundary render',
768
+ 'BrokenRender',
769
+
770
+ // Errored again on retry. Now handle it.
771
'RethrowErrorBoundary componentDidCatch',
772
]);
773
expect(ReactNoop.getChildren()).toEqual([]);
@@ -741,7 +806,12 @@ describe('ReactIncrementalErrorHandling', () => {
806
expect(ops).toEqual([
807
'RethrowErrorBoundary render',
808
'BrokenRender',
744
- // React doesn't retry because we're already rendering synchronously.
809
+
810
+ // React retries one more time
811
+ 'RethrowErrorBoundary render',
812
+ 'BrokenRender',
813
+
814
+ // Errored again on retry. Now handle it.
815
'RethrowErrorBoundary componentDidCatch',
816
]);
817
expect(ReactNoop.getChildren()).toEqual([]);
packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js
+6
@@ -189,8 +189,14 @@ describe('ReactIncrementalErrorLogging', () => {
189
[
190
'render: 0',
191
__DEV__ && 'render: 0', // replay
192
+
193
'render: 1',
194
__DEV__ && 'render: 1', // replay
195
+
196
+ // Retry one more time before handling error
197
+ 'render: 1',
198
+ __DEV__ && 'render: 1', // replay
199
+
200
'componentWillUnmount: 0',
201
].filter(Boolean),
202
);
packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js
+110
@@ -655,6 +655,116 @@ describe('ReactIncrementalUpdates', () => {
655
});
656
});
657
658
+ it('does not throw out partially completed tree if it expires midway through', () => {
659
+ function Text({text}) {
660
+ Scheduler.unstable_yieldValue(text);
661
+ return text;
662
+ }
663
+
664
+ function App({step}) {
665
+ return (
666
+ <>
667
+ <Text text={`A${step}`} />
668
+ <Text text={`B${step}`} />
669
+ <Text text={`C${step}`} />
670
+ </>
671
+ );
672
+ }
673
+
674
+ function interrupt() {
675
+ ReactNoop.flushSync(() => {
676
+ ReactNoop.renderToRootWithID(null, 'other-root');
677
+ });
678
+ }
679
+
680
+ // First, as a sanity check, assert what happens when four low pri
681
+ // updates in separate batches are all flushed in the same callback
682
+ ReactNoop.act(() => {
683
+ ReactNoop.render(<App step={1} />);
684
+ Scheduler.unstable_advanceTime(1000);
685
+ expect(Scheduler).toFlushAndYieldThrough(['A1']);
686
+
687
+ interrupt();
688
+
689
+ ReactNoop.render(<App step={2} />);
690
+ Scheduler.unstable_advanceTime(1000);
691
+ expect(Scheduler).toFlushAndYieldThrough(['A1']);
692
+
693
+ interrupt();
694
+
695
+ ReactNoop.render(<App step={3} />);
696
+ Scheduler.unstable_advanceTime(1000);
697
+ expect(Scheduler).toFlushAndYieldThrough(['A1']);
698
+
699
+ interrupt();
700
+
701
+ ReactNoop.render(<App step={4} />);
702
+ Scheduler.unstable_advanceTime(1000);
703
+ expect(Scheduler).toFlushAndYieldThrough(['A1']);
704
+
705
+ // Each update flushes in a separate commit.
706
+ // Note: This isn't necessarily the ideal behavior. It might be better to
707
+ // batch all of these updates together. The fact that they don't is an
708
+ // implementation detail. The important part of this unit test is what
709
+ // happens when they expire, in which case they really should be batched to
710
+ // avoid blocking the main thread for a long time.
711
+ expect(Scheduler).toFlushAndYield([
712
+ // A1 already completed. Finish rendering the first level.
713
+ 'B1',
714
+ 'C1',
715
+ // The remaining two levels complete sequentially.
716
+ 'A2',
717
+ 'B2',
718
+ 'C2',
719
+ 'A3',
720
+ 'B3',
721
+ 'C3',
722
+ 'A4',
723
+ 'B4',
724
+ 'C4',
725
+ ]);
726
+ });
727
+
728
+ ReactNoop.act(() => {
729
+ // Now do the same thing over again, but this time, expire all the updates
730
+ // instead of flushing them normally.
731
+ ReactNoop.render(<App step={1} />);
732
+ Scheduler.unstable_advanceTime(1000);
733
+ expect(Scheduler).toFlushAndYieldThrough(['A1']);
734
+
735
+ interrupt();
736
+
737
+ ReactNoop.render(<App step={2} />);
738
+ Scheduler.unstable_advanceTime(1000);
739
+ expect(Scheduler).toFlushAndYieldThrough(['A1']);
740
+
741
+ interrupt();
742
+
743
+ ReactNoop.render(<App step={3} />);
744
+ Scheduler.unstable_advanceTime(1000);
745
+ expect(Scheduler).toFlushAndYieldThrough(['A1']);
746
+
747
+ interrupt();
748
+
749
+ ReactNoop.render(<App step={4} />);
750
+ Scheduler.unstable_advanceTime(1000);
751
+ expect(Scheduler).toFlushAndYieldThrough(['A1']);
752
+
753
+ // Expire all the updates
754
+ ReactNoop.expire(10000);
755
+
756
+ expect(Scheduler).toFlushExpired([
757
+ // A1 already completed. Finish rendering the first level.
758
+ 'B1',
759
+ 'C1',
760
+ // Then render the remaining two levels in a single batch
761
+ 'A4',
762
+ 'B4',
763
+ 'C4',
764
+ ]);
765
+ });
766
+ });
767
+
768
it('when rebasing, does not exclude updates that were already committed, regardless of priority', async () => {
769
const {useState, useLayoutEffect} = React;
770
packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js
+120
@@ -408,3 +408,123 @@ describe('ReactSchedulerIntegration', () => {
408
);
409
});
410
});
411
+
412
+describe(
413
+ 'regression test: does not infinite loop if `shouldYield` returns ' +
414
+ 'true after a partial tree expires',
415
+ () => {
416
+ let logDuringShouldYield = false;
417
+
418
+ beforeEach(() => {
419
+ jest.resetModules();
420
+ ReactFeatureFlags = require('shared/ReactFeatureFlags');
421
+ ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
422
+
423
+ jest.mock('scheduler', () => {
424
+ const actual = require.requireActual('scheduler/unstable_mock');
425
+ return {
426
+ ...actual,
427
+ unstable_shouldYield() {
428
+ if (logDuringShouldYield) {
429
+ actual.unstable_yieldValue('shouldYield');
430
+ }
431
+ return actual.unstable_shouldYield();
432
+ },
433
+ };
434
+ });
435
+
436
+ React = require('react');
437
+ ReactNoop = require('react-noop-renderer');
438
+ Scheduler = require('scheduler');
439
+
440
+ React = require('react');
441
+ });
442
+
443
+ afterEach(() => {
444
+ jest.mock('scheduler', () =>
445
+ require.requireActual('scheduler/unstable_mock'),
446
+ );
447
+ });
448
+
449
+ it('using public APIs to trigger real world scenario', async () => {
450
+ // This test reproduces a case where React's Scheduler task timed out but
451
+ // the `shouldYield` method returned true. The bug was that React fell
452
+ // into an infinite loop, because it would enter the work loop then
453
+ // immediately yield back to Scheduler.
454
+ //
455
+ // (The next test in this suite covers the same case. The difference is
456
+ // that this test only uses public APIs, whereas the next test mocks
457
+ // `shouldYield` to check when it is called.)
458
+ function Text({text}) {
459
+ return text;
460
+ }
461
+
462
+ function App({step}) {
463
+ return (
464
+ <>
465
+ <Text text="A" />
466
+ <TriggerErstwhileSchedulerBug />
467
+ <Text text="B" />
468
+ <TriggerErstwhileSchedulerBug />
469
+ <Text text="C" />
470
+ </>
471
+ );
472
+ }
473
+
474
+ function TriggerErstwhileSchedulerBug() {
475
+ // This triggers a once-upon-a-time bug in Scheduler that caused
476
+ // `shouldYield` to return true even though the current task expired.
477
+ Scheduler.unstable_advanceTime(10000);
478
+ Scheduler.unstable_requestPaint();
479
+ return null;
480
+ }
481
+
482
+ await ReactNoop.act(async () => {
483
+ ReactNoop.render(<App />);
484
+ expect(Scheduler).toFlushUntilNextPaint([]);
485
+ expect(Scheduler).toFlushUntilNextPaint([]);
486
+ });
487
+ });
488
+
489
+ it('mock Scheduler module to check if `shouldYield` is called', async () => {
490
+ // This test reproduces a bug where React's Scheduler task timed out but
491
+ // the `shouldYield` method returned true. Usually we try not to mock
492
+ // internal methods, but I've made an exception here since the point is
493
+ // specifically to test that React is reslient to the behavior of a
494
+ // Scheduler API. That being said, feel free to rewrite or delete this
495
+ // test if/when the API changes.
496
+ function Text({text}) {
497
+ Scheduler.unstable_yieldValue(text);
498
+ return text;
499
+ }
500
+
501
+ function App({step}) {
502
+ return (
503
+ <>
504
+ <Text text="A" />
505
+ <Text text="B" />
506
+ <Text text="C" />
507
+ </>
508
+ );
509
+ }
510
+
511
+ await ReactNoop.act(async () => {
512
+ // Partially render the tree, then yield
513
+ ReactNoop.render(<App />);
514
+ expect(Scheduler).toFlushAndYieldThrough(['A']);
515
+
516
+ // Start logging whenever shouldYield is called
517
+ logDuringShouldYield = true;
518
+ // Let's call it once to confirm the mock actually works
519
+ Scheduler.unstable_shouldYield();
520
+ expect(Scheduler).toHaveYielded(['shouldYield']);
521
+
522
+ // Expire the task
523
+ Scheduler.unstable_advanceTime(10000);
524
+ // Because the render expired, React should finish the tree without
525
+ // consulting `shouldYield` again
526
+ expect(Scheduler).toFlushExpired(['B', 'C']);
527
+ });
528
+ });
529
+ },
530
+);