From 29433b40f2858be6ab9a5dcf532078a66b7b5d3f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 30 Jan 2020 12:53:45 -0800 Subject: [PATCH] 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 --- .../src/ReactFiberWorkLoop.js | 13 ++- ...tIncrementalErrorHandling-test.internal.js | 50 ++++++++ .../ReactIncrementalUpdates-test.internal.js | 110 ++++++++++++++++++ 3 files changed, 170 insertions(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 8850e8532f901..15a49f37e93b7 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -643,9 +643,16 @@ function performConcurrentWorkOnRoot(root, didTimeout) { // event time. The next update will compute a new event time. currentEventTime = NoWork; - if (didTimeout) { - // The render task took too long to complete. Mark the current time as - // expired to synchronously render all expired work in a single batch. + // Check if the render expired. If so, restart at the current time so that we + // can finish all the expired work in a single batch. However, we should only + // do this if we're starting a new tree. If we're in the middle of an existing + // tree, we'll continue working on that (without yielding) so that the work + // doesn't get dropped. If there's another expired level after that, we'll hit + // this path again, at which point we can batch all the subsequent levels + // together. + if (didTimeout && workInProgress === null) { + // Mark the current time as expired to synchronously render all expired work + // in a single batch. const currentTime = requestCurrentTimeForUpdate(); markRootExpiredAtTime(root, currentTime); // This will schedule a synchronous callback. diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 6bea104b78db5..6930fe9f08a55 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -381,6 +381,56 @@ describe('ReactIncrementalErrorHandling', () => { expect(ReactNoop.getChildren()).toEqual([]); }); + it('retries one more time if an error occurs during a render that expires midway through the tree', () => { + function Oops() { + Scheduler.unstable_yieldValue('Oops'); + throw new Error('Oops'); + } + + function Text({text}) { + Scheduler.unstable_yieldValue(text); + return text; + } + + function App() { + return ( + <> + + + + + + + ); + } + + ReactNoop.render(); + + // Render part of the tree + expect(Scheduler).toFlushAndYieldThrough(['A', 'B']); + + // Expire the render midway through + Scheduler.unstable_advanceTime(10000); + expect(() => Scheduler.unstable_flushExpired()).toThrow('Oops'); + + expect(Scheduler).toHaveYielded([ + // The render expired, but we shouldn't throw out the partial work. + // Finish the current level. + 'Oops', + 'C', + 'D', + + // Since the error occured during a partially concurrent render, we should + // retry one more time, synchonrously. + 'A', + 'B', + 'Oops', + 'C', + 'D', + ]); + expect(ReactNoop.getChildren()).toEqual([]); + }); + it('calls componentDidCatch multiple times for multiple errors', () => { let id = 0; class BadMount extends React.Component { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js index 231dcde677eb1..be544c18528d9 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js @@ -655,6 +655,116 @@ describe('ReactIncrementalUpdates', () => { }); }); + it('does not throw out partially completed tree if it expires midway through', () => { + function Text({text}) { + Scheduler.unstable_yieldValue(text); + return text; + } + + function App({step}) { + return ( + <> + + + + + ); + } + + function interrupt() { + ReactNoop.flushSync(() => { + ReactNoop.renderToRootWithID(null, 'other-root'); + }); + } + + // First, as a sanity check, assert what happens when four low pri + // updates in separate batches are all flushed in the same callback + ReactNoop.act(() => { + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + // Each update flushes in a separate commit. + // Note: This isn't necessarily the ideal behavior. It might be better to + // batch all of these updates together. The fact that they don't is an + // implementation detail. The important part of this unit test is what + // happens when they expire, in which case they really should be batched to + // avoid blocking the main thread for a long time. + expect(Scheduler).toFlushAndYield([ + // A1 already completed. Finish rendering the first level. + 'B1', + 'C1', + // The remaining two levels complete sequentially. + 'A2', + 'B2', + 'C2', + 'A3', + 'B3', + 'C3', + 'A4', + 'B4', + 'C4', + ]); + }); + + ReactNoop.act(() => { + // Now do the same thing over again, but this time, expire all the updates + // instead of flushing them normally. + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + // Expire all the updates + ReactNoop.expire(10000); + + expect(Scheduler).toFlushExpired([ + // A1 already completed. Finish rendering the first level. + 'B1', + 'C1', + // Then render the remaining two levels in a single batch + 'A4', + 'B4', + 'C4', + ]); + }); + }); + it('when rebasing, does not exclude updates that were already committed, regardless of priority', async () => { const {useState, useLayoutEffect} = React;