Skip to content

Commit cc71f9a

Browse files
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 dddcae7 commit cc71f9a

File tree

2 files changed

+116
-3
lines changed

2 files changed

+116
-3
lines changed

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

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

packages/react-server/src/ReactFizzServer.js

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4882,7 +4882,11 @@ function queueCompletedSegment(
48824882
const childSegment = segment.children[0];
48834883
childSegment.id = segment.id;
48844884
childSegment.parentFlushed = true;
4885-
if (childSegment.status === COMPLETED) {
4885+
if (
4886+
childSegment.status === COMPLETED ||
4887+
childSegment.status === ABORTED ||
4888+
childSegment.status === ERRORED
4889+
) {
48864890
queueCompletedSegment(boundary, childSegment);
48874891
}
48884892
} else {
@@ -4953,7 +4957,7 @@ function finishedTask(
49534957
// Our parent segment already flushed, so we need to schedule this segment to be emitted.
49544958
// If it is a segment that was aborted, we'll write other content instead so we don't need
49554959
// to emit it.
4956-
if (segment.status === COMPLETED) {
4960+
if (segment.status === COMPLETED || segment.status === ABORTED) {
49574961
queueCompletedSegment(boundary, segment);
49584962
}
49594963
}
@@ -5022,7 +5026,7 @@ function finishedTask(
50225026
// Our parent already flushed, so we need to schedule this segment to be emitted.
50235027
// If it is a segment that was aborted, we'll write other content instead so we don't need
50245028
// to emit it.
5025-
if (segment.status === COMPLETED) {
5029+
if (segment.status === COMPLETED || segment.status === ABORTED) {
50265030
queueCompletedSegment(boundary, segment);
50275031
const completedSegments = boundary.completedSegments;
50285032
if (completedSegments.length === 1) {
@@ -5533,6 +5537,9 @@ function flushSubtree(
55335537
}
55345538
return r;
55355539
}
5540+
case ABORTED: {
5541+
return true;
5542+
}
55365543
default: {
55375544
throw new Error(
55385545
'Aborted, errored or already flushed boundaries should not be flushed again. This is a bug in React.',

0 commit comments

Comments
 (0)