Revert "Bugfix: Expiring a partially completed tree (#17926)" (#17941)
This reverts commit 01974a867c543fca6ebe9f4e612dcc3bb8f80528. * 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 30, 2020 at 11:14 UTC
9dbe1c54dff3b6a7f89bae9b19b4698e0bfbcb9a
3 files changed
+3
-169
packages/react-reconciler/src/ReactFiberWorkLoop.js
+3
-10
@@ -644,16 +644,9 @@ 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. 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.
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.
650
const currentTime = requestCurrentTimeForUpdate();
651
markRootExpiredAtTime(root, currentTime);
652
// This will schedule a synchronous callback.
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
-49
@@ -381,55 +381,6 @@ 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
-
384
it('calls componentDidCatch multiple times for multiple errors', () => {
385
let id = 0;
386
class BadMount extends React.Component {
packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js
-110
@@ -655,116 +655,6 @@ 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
-
658
it('when rebasing, does not exclude updates that were already committed, regardless of priority', async () => {
659
const {useState, useLayoutEffect} = React;
660