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
Andrew Clark committed
Jan 29, 2020 at 12:40 UTC
01974a867c543fca6ebe9f4e612dcc3bb8f80528
3 files changed
+169
-3
packages/react-reconciler/src/ReactFiberWorkLoop.js
+10
-3
@@ -644,9 +644,16 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
644
// event time. The next update will compute a new event time.
645
currentEventTime = NoWork;
646
647
- if (didTimeout) {
648
- // The render task took too long to complete. Mark the current time as
649
- // expired to synchronously render all expired work in a single batch.
647
+ // Check if the render expired. If so, restart at the current time so that we
648
+ // can finish all the expired work in a single batch. However, we should only
649
+ // do this if we're starting a new tree. If we're in the middle of an existing
650
+ // tree, we'll continue working on that (without yielding) so that the work
651
+ // doesn't get dropped. If there's another expired level after that, we'll hit
652
+ // this path again, at which point we can batch all the subsequent levels
653
+ // together.
654
+ if (didTimeout && workInProgress === null) {
655
+ // Mark the current time as expired to synchronously render all expired work
656
+ // in a single batch.
657
const currentTime = requestCurrentTimeForUpdate();
658
markRootExpiredAtTime(root, currentTime);
659
// This will schedule a synchronous callback.
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
+49
@@ -381,6 +381,55 @@ 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
+ expect(() => ReactNoop.expire(10000)).toThrow('Oops');
414
+
415
+ expect(Scheduler).toHaveYielded([
416
+ // The render expired, but we shouldn't throw out the partial work.
417
+ // Finish the current level.
418
+ 'Oops',
419
+ 'C',
420
+ 'D',
421
+
422
+ // Since the error occured during a partially concurrent render, we should
423
+ // retry one more time, synchonrously.
424
+ 'A',
425
+ 'B',
426
+ 'Oops',
427
+ 'C',
428
+ 'D',
429
+ ]);
430
+ expect(ReactNoop.getChildren()).toEqual([]);
431
+ });
432
+
433
it('calls componentDidCatch multiple times for multiple errors', () => {
434
let id = 0;
435
class BadMount extends React.Component {
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).toHaveYielded([
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