Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
When deeply nested Suspense boundaries inside a fallback of another b…
…oundary 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.
  • Loading branch information
gnoff committed Jun 6, 2025
commit 4a4e90392349fc862cc33aa987b4cac436a5bbf8
107 changes: 107 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ describe('ReactDOMFizzServer', () => {
setTimeout(cb);
container = document.getElementById('container');

CSPnonce = null;
Scheduler = require('scheduler');
React = require('react');
ReactDOM = require('react-dom');
Expand Down Expand Up @@ -10447,4 +10448,110 @@ describe('ReactDOMFizzServer', () => {
</html>,
);
});

it('should not error when discarding deeply nested Suspense boundaries in a parent fallback partially complete before the parent boundary resolves', async () => {
let resolve1;
const promise1 = new Promise(r => (resolve1 = r));
let resolve2;
const promise2 = new Promise(r => (resolve2 = r));
const promise3 = new Promise(r => {});

function Use({children, promise}) {
React.use(promise);
return children;
}
function App() {
return (
<div>
<Suspense
fallback={
<div>
<Suspense fallback="Loading...">
<div>
<Use promise={promise1}>
<div>
<Suspense fallback="Loading more...">
<div>
<Use promise={promise3}>
<div>deep fallback</div>
</Use>
</div>
</Suspense>
</div>
</Use>
</div>
</Suspense>
</div>
}>
<Use promise={promise2}>Success!</Use>
</Suspense>
</div>
);
}

await act(() => {
const {pipe} = renderToPipeableStream(<App />);
pipe(writable);
});

expect(getVisibleChildren(container)).toEqual(
<div>
<div>Loading...</div>
</div>,
);

await act(() => {
resolve1('resolved');
resolve2('resolved');
});

expect(getVisibleChildren(container)).toEqual(<div>Success!</div>);
});

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 () => {
let resolve1;
const promise1 = new Promise(r => (resolve1 = r));
let resolve2;
const promise2 = new Promise(r => (resolve2 = r));
const promise3 = new Promise(r => {});

function Use({children, promise}) {
React.use(promise);
return children;
}
function App() {
return (
<div>
<Suspense
fallback={
<Suspense fallback="Loading...">
<Use promise={promise1}>
<Suspense fallback="Loading more...">
<Use promise={promise3}>
<div>deep fallback</div>
</Use>
</Suspense>
</Use>
</Suspense>
}>
<Use promise={promise2}>Success!</Use>
</Suspense>
</div>
);
}

await act(() => {
const {pipe} = renderToPipeableStream(<App />);
pipe(writable);
});

expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

await act(() => {
resolve1('resolved');
resolve2('resolved');
});

expect(getVisibleChildren(container)).toEqual(<div>Success!</div>);
});
});
3 changes: 2 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMFloat-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ let SuspenseList;
let textCache;
let loadCache;
let writable;
const CSPnonce = null;
let CSPnonce = null;
let container;
let buffer = '';
let hasErrored = false;
Expand Down Expand Up @@ -69,6 +69,7 @@ describe('ReactDOMFloat', () => {
setTimeout(cb);
container = document.getElementById('container');

CSPnonce = null;
React = require('react');
ReactDOM = require('react-dom');
ReactDOMClient = require('react-dom/client');
Expand Down
13 changes: 10 additions & 3 deletions packages/react-server/src/ReactFizzServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -4918,7 +4918,11 @@ function queueCompletedSegment(
const childSegment = segment.children[0];
childSegment.id = segment.id;
childSegment.parentFlushed = true;
if (childSegment.status === COMPLETED) {
if (
childSegment.status === COMPLETED ||
childSegment.status === ABORTED ||
childSegment.status === ERRORED
) {
queueCompletedSegment(boundary, childSegment);
}
} else {
Expand Down Expand Up @@ -4989,7 +4993,7 @@ function finishedTask(
// Our parent segment already flushed, so we need to schedule this segment to be emitted.
// If it is a segment that was aborted, we'll write other content instead so we don't need
// to emit it.
if (segment.status === COMPLETED) {
if (segment.status === COMPLETED || segment.status === ABORTED) {
queueCompletedSegment(boundary, segment);
}
}
Expand Down Expand Up @@ -5058,7 +5062,7 @@ function finishedTask(
// Our parent already flushed, so we need to schedule this segment to be emitted.
// If it is a segment that was aborted, we'll write other content instead so we don't need
// to emit it.
if (segment.status === COMPLETED) {
if (segment.status === COMPLETED || segment.status === ABORTED) {
queueCompletedSegment(boundary, segment);
const completedSegments = boundary.completedSegments;
if (completedSegments.length === 1) {
Expand Down Expand Up @@ -5575,6 +5579,9 @@ function flushSubtree(
}
return r;
}
case ABORTED: {
return true;
}
default: {
throw new Error(
'Aborted, errored or already flushed boundaries should not be flushed again. This is a bug in React.',
Expand Down
Loading