Skip to content

Commit 49595e9

Browse files
authored
[New Scheduler] Fix: Suspending an expired update (#15326)
When an async update expires, React renders at the expiration time that corresponds to the current time, not at the original update's expiration time. That way, all the expired work in the tree is flushed in a single batch. This is implemented inside `renderRoot` by comparing the next render expiration time to the current time. If the current time is later, `renderRoot` will restart at the later time. Because of poor factoring, the check is currently performed right before entering the work loop. But the work loop is usually entered multiple times in a single callback: each time a component throws or suspends. This led to an infinite loop where React would detect that an update expired, restart at the current time, make a bit of progress, suspend, check for expired work again, and start the loop again. I fixed this by moving the expired work check to the beginning of `renderRoot`, so that it is not performed every time something suspends. This isn't ideal, because you could technically still fall into a loop if more than 10ms lapse in between exiting `renderRoot` and entering it again. The proper fix is to lift the check outside of `renderRoot` entirely so that the function can restart without checking for expired work again. Since this is exceedingly unlikely (and this whole thing is still behind a flag), I'll do the better fix in an already-planned follow up to fork `renderRoot` into separate functions for sync and async work.
1 parent b93a8a9 commit 49595e9

File tree

2 files changed

+71
-15
lines changed

2 files changed

+71
-15
lines changed

packages/react-reconciler/src/ReactFiberScheduler.new.js

+27-15
Original file line numberDiff line numberDiff line change
@@ -742,26 +742,38 @@ function renderRoot(
742742
}
743743

744744
startWorkLoopTimer(workInProgress);
745+
746+
// TODO: Fork renderRoot into renderRootSync and renderRootAsync
747+
if (isSync) {
748+
if (expirationTime !== Sync) {
749+
// An async update expired. There may be other expired updates on
750+
// this root. We should render all the expired work in a
751+
// single batch.
752+
const currentTime = requestCurrentTime();
753+
if (currentTime < expirationTime) {
754+
// Restart at the current time.
755+
workPhase = prevWorkPhase;
756+
resetContextDependencies();
757+
ReactCurrentDispatcher.current = prevDispatcher;
758+
if (enableSchedulerTracing) {
759+
__interactionsRef.current = ((prevInteractions: any): Set<
760+
Interaction,
761+
>);
762+
}
763+
return renderRoot.bind(null, root, currentTime);
764+
}
765+
}
766+
} else {
767+
// Since we know we're in a React event, we can clear the current
768+
// event time. The next update will compute a new event time.
769+
currentEventTime = NoWork;
770+
}
771+
745772
do {
746773
try {
747774
if (isSync) {
748-
if (expirationTime !== Sync) {
749-
// An async update expired. There may be other expired updates on
750-
// this root. We should render all the expired work in a
751-
// single batch.
752-
const currentTime = requestCurrentTime();
753-
if (currentTime < expirationTime) {
754-
// Restart at the current time.
755-
workPhase = prevWorkPhase;
756-
ReactCurrentDispatcher.current = prevDispatcher;
757-
return renderRoot.bind(null, root, currentTime);
758-
}
759-
}
760775
workLoopSync();
761776
} else {
762-
// Since we know we're in a React event, we can clear the current
763-
// event time. The next update will compute a new event time.
764-
currentEventTime = NoWork;
765777
workLoop();
766778
}
767779
break;

packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js

+44
Original file line numberDiff line numberDiff line change
@@ -827,6 +827,50 @@ describe('ReactSuspenseWithNoopRenderer', () => {
827827
expect(ReactNoop.getChildren()).toEqual([span('goodbye')]);
828828
});
829829

830+
it('a suspended update that expires', async () => {
831+
// Regression test. This test used to fall into an infinite loop.
832+
function ExpensiveText({text}) {
833+
// This causes the update to expire.
834+
Scheduler.advanceTime(10000);
835+
// Then something suspends.
836+
return <AsyncText text={text} ms={200000} />;
837+
}
838+
839+
function App() {
840+
return (
841+
<Suspense fallback="Loading...">
842+
<ExpensiveText text="A" />
843+
<ExpensiveText text="B" />
844+
<ExpensiveText text="C" />
845+
</Suspense>
846+
);
847+
}
848+
849+
ReactNoop.render(<App />);
850+
expect(Scheduler).toFlushAndYield([
851+
'Suspend! [A]',
852+
'Suspend! [B]',
853+
'Suspend! [C]',
854+
]);
855+
expect(ReactNoop).toMatchRenderedOutput('Loading...');
856+
857+
await advanceTimers(200000);
858+
expect(Scheduler).toHaveYielded([
859+
'Promise resolved [A]',
860+
'Promise resolved [B]',
861+
'Promise resolved [C]',
862+
]);
863+
864+
expect(Scheduler).toFlushAndYield(['A', 'B', 'C']);
865+
expect(ReactNoop).toMatchRenderedOutput(
866+
<React.Fragment>
867+
<span prop="A" />
868+
<span prop="B" />
869+
<span prop="C" />
870+
</React.Fragment>,
871+
);
872+
});
873+
830874
describe('sync mode', () => {
831875
it('times out immediately', async () => {
832876
function App() {

0 commit comments

Comments
 (0)