Skip to content

Commit 4a4e903

Browse files
gnoffgnoff
committed
When deeply nested Suspense boundaries inside a fallback of another boundary resolve it is possible to encounter situations where you either attempt to flush an aborted Segment or you have a boundary without any root segment. We intended for both of these conditions to be impossible to arrive at legitimately however it turns out in this situation you can. The fix is two-fold
1. allow flushing aborted segments by simply skipping them. This does remove some protection against future misconfiguraiton of React because it is no longer an invariant that you hsould never attempt to flush an aborted segment but there are legitimate cases where this can come up and simply omitting the segment is fine b/c we know that the user will never observe this. A semantically better solution would be to avoid flushing boudaries inside an unneeded fallback but to do this we would need to track all boundaries inside a fallback or create back pointers which add to memory overhead and possibly make GC harder to do efficiently. By flushing extra we're maintaining status quo and only suffer in performance not with broken semantics. 2. when queuing completed segments allow for queueing aborted segments and if we are eliding the enqueued segment allow for child segments that are errored to be enqueued too. This will mean that we can maintain the invariant that a boundary must have a root segment the first time we flush it, it just might be aborted (see point 1 above). This change has two seemingly similar test cases to exercise this fix. The reason we need both is that when you have empty segments you hit different code paths within Fizz and so each one (without this fix) triggers a different error pathway.
1 parent 6ccf328 commit 4a4e903

File tree

3 files changed

+119
-4
lines changed

3 files changed

+119
-4
lines changed

‎packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ describe('ReactDOMFizzServer', () => {
8888
setTimeout(cb);
8989
container = document.getElementById('container');
9090

91+
CSPnonce = null;
9192
Scheduler = require('scheduler');
9293
React = require('react');
9394
ReactDOM = require('react-dom');
@@ -10447,4 +10448,110 @@ describe('ReactDOMFizzServer', () => {
1044710448
</html>,
1044810449
);
1044910450
});
10451+
10452+
it('should not error when discarding deeply nested Suspense boundaries in a parent fallback partially complete before the parent boundary resolves', async () => {
10453+
let resolve1;
10454+
const promise1 = new Promise(r => (resolve1 = r));
10455+
let resolve2;
10456+
const promise2 = new Promise(r => (resolve2 = r));
10457+
const promise3 = new Promise(r => {});
10458+
10459+
function Use({children, promise}) {
10460+
React.use(promise);
10461+
return children;
10462+
}
10463+
function App() {
10464+
return (
10465+
<div>
10466+
<Suspense
10467+
fallback={
10468+
<div>
10469+
<Suspense fallback="Loading...">
10470+
<div>
10471+
<Use promise={promise1}>
10472+
<div>
10473+
<Suspense fallback="Loading more...">
10474+
<div>
10475+
<Use promise={promise3}>
10476+
<div>deep fallback</div>
10477+
</Use>
10478+
</div>
10479+
</Suspense>
10480+
</div>
10481+
</Use>
10482+
</div>
10483+
</Suspense>
10484+
</div>
10485+
}>
10486+
<Use promise={promise2}>Success!</Use>
10487+
</Suspense>
10488+
</div>
10489+
);
10490+
}
10491+
10492+
await act(() => {
10493+
const {pipe} = renderToPipeableStream(<App />);
10494+
pipe(writable);
10495+
});
10496+
10497+
expect(getVisibleChildren(container)).toEqual(
10498+
<div>
10499+
<div>Loading...</div>
10500+
</div>,
10501+
);
10502+
10503+
await act(() => {
10504+
resolve1('resolved');
10505+
resolve2('resolved');
10506+
});
10507+
10508+
expect(getVisibleChildren(container)).toEqual(<div>Success!</div>);
10509+
});
10510+
10511+
it('should not error when discarding deeply nested Suspense boundaries in a parent fallback partially complete before the parent boundary resolves with empty segments', async () => {
10512+
let resolve1;
10513+
const promise1 = new Promise(r => (resolve1 = r));
10514+
let resolve2;
10515+
const promise2 = new Promise(r => (resolve2 = r));
10516+
const promise3 = new Promise(r => {});
10517+
10518+
function Use({children, promise}) {
10519+
React.use(promise);
10520+
return children;
10521+
}
10522+
function App() {
10523+
return (
10524+
<div>
10525+
<Suspense
10526+
fallback={
10527+
<Suspense fallback="Loading...">
10528+
<Use promise={promise1}>
10529+
<Suspense fallback="Loading more...">
10530+
<Use promise={promise3}>
10531+
<div>deep fallback</div>
10532+
</Use>
10533+
</Suspense>
10534+
</Use>
10535+
</Suspense>
10536+
}>
10537+
<Use promise={promise2}>Success!</Use>
10538+
</Suspense>
10539+
</div>
10540+
);
10541+
}
10542+
10543+
await act(() => {
10544+
const {pipe} = renderToPipeableStream(<App />);
10545+
pipe(writable);
10546+
});
10547+
10548+
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
10549+
10550+
await act(() => {
10551+
resolve1('resolved');
10552+
resolve2('resolved');
10553+
});
10554+
10555+
expect(getVisibleChildren(container)).toEqual(<div>Success!</div>);
10556+
});
1045010557
});

‎packages/react-dom/src/__tests__/ReactDOMFloat-test.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ let SuspenseList;
2525
let textCache;
2626
let loadCache;
2727
let writable;
28-
const CSPnonce = null;
28+
let CSPnonce = null;
2929
let container;
3030
let buffer = '';
3131
let hasErrored = false;
@@ -69,6 +69,7 @@ describe('ReactDOMFloat', () => {
6969
setTimeout(cb);
7070
container = document.getElementById('container');
7171

72+
CSPnonce = null;
7273
React = require('react');
7374
ReactDOM = require('react-dom');
7475
ReactDOMClient = require('react-dom/client');

‎packages/react-server/src/ReactFizzServer.js‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4918,7 +4918,11 @@ function queueCompletedSegment(
49184918
const childSegment = segment.children[0];
49194919
childSegment.id = segment.id;
49204920
childSegment.parentFlushed = true;
4921-
if (childSegment.status === COMPLETED) {
4921+
if (
4922+
childSegment.status === COMPLETED ||
4923+
childSegment.status === ABORTED ||
4924+
childSegment.status === ERRORED
4925+
) {
49224926
queueCompletedSegment(boundary, childSegment);
49234927
}
49244928
} else {
@@ -4989,7 +4993,7 @@ function finishedTask(
49894993
// Our parent segment already flushed, so we need to schedule this segment to be emitted.
49904994
// If it is a segment that was aborted, we'll write other content instead so we don't need
49914995
// to emit it.
4992-
if (segment.status === COMPLETED) {
4996+
if (segment.status === COMPLETED || segment.status === ABORTED) {
49934997
queueCompletedSegment(boundary, segment);
49944998
}
49954999
}
@@ -5058,7 +5062,7 @@ function finishedTask(
50585062
// Our parent already flushed, so we need to schedule this segment to be emitted.
50595063
// If it is a segment that was aborted, we'll write other content instead so we don't need
50605064
// to emit it.
5061-
if (segment.status === COMPLETED) {
5065+
if (segment.status === COMPLETED || segment.status === ABORTED) {
50625066
queueCompletedSegment(boundary, segment);
50635067
const completedSegments = boundary.completedSegments;
50645068
if (completedSegments.length === 1) {
@@ -5575,6 +5579,9 @@ function flushSubtree(
55755579
}
55765580
return r;
55775581
}
5582+
case ABORTED: {
5583+
return true;
5584+
}
55785585
default: {
55795586
throw new Error(
55805587
'Aborted, errored or already flushed boundaries should not be flushed again. This is a bug in React.',

0 commit comments

Comments
 (0)