[Fizz] Clean up row that was blocked by an aborted boundary (#33318)

Fixes a bug that we caused us to hang after an abort because we didn't
manage the ref count correctly.
This commit is contained in:
Sebastian Markbåge 2025-05-20 20:31:16 -04:00 committed by GitHub
parent 50389e1792
commit 9c7b10e22e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 160 additions and 13 deletions

View File

@ -27,9 +27,10 @@ let writable;
let container; let container;
let buffer = ''; let buffer = '';
let hasErrored = false; let hasErrored = false;
let hasCompleted = false;
let fatalError = undefined; let fatalError = undefined;
describe('ReactDOMFizSuspenseList', () => { describe('ReactDOMFizzSuspenseList', () => {
beforeEach(() => { beforeEach(() => {
jest.resetModules(); jest.resetModules();
JSDOM = require('jsdom').JSDOM; JSDOM = require('jsdom').JSDOM;
@ -59,6 +60,7 @@ describe('ReactDOMFizSuspenseList', () => {
buffer = ''; buffer = '';
hasErrored = false; hasErrored = false;
hasCompleted = false;
writable = new Stream.PassThrough(); writable = new Stream.PassThrough();
writable.setEncoding('utf8'); writable.setEncoding('utf8');
@ -69,6 +71,9 @@ describe('ReactDOMFizSuspenseList', () => {
hasErrored = true; hasErrored = true;
fatalError = error; fatalError = error;
}); });
writable.on('finish', () => {
hasCompleted = true;
});
}); });
afterEach(() => { afterEach(() => {
@ -103,7 +108,12 @@ describe('ReactDOMFizSuspenseList', () => {
function createAsyncText(text) { function createAsyncText(text) {
let resolved = false; let resolved = false;
let error = undefined;
const Component = function () { const Component = function () {
if (error !== undefined) {
Scheduler.log('Error! [' + error.message + ']');
throw error;
}
if (!resolved) { if (!resolved) {
Scheduler.log('Suspend! [' + text + ']'); Scheduler.log('Suspend! [' + text + ']');
throw promise; throw promise;
@ -115,6 +125,10 @@ describe('ReactDOMFizSuspenseList', () => {
resolved = true; resolved = true;
return resolve(); return resolve();
}; };
Component.reject = function (e) {
error = e;
return resolve();
};
}); });
return Component; return Component;
} }
@ -714,4 +728,120 @@ describe('ReactDOMFizSuspenseList', () => {
</div>, </div>,
); );
}); });
// @gate enableSuspenseList
it('can abort a pending SuspenseList', async () => {
const A = createAsyncText('A');
function Foo() {
return (
<div>
<SuspenseList revealOrder="forwards">
<Suspense fallback={<Text text="Loading A" />}>
<A />
</Suspense>
<Suspense fallback={<Text text="Loading B" />}>
<Text text="B" />
</Suspense>
</SuspenseList>
</div>
);
}
const errors = [];
let abortStream;
await serverAct(async () => {
const {pipe, abort} = ReactDOMFizzServer.renderToPipeableStream(<Foo />, {
onError(error) {
errors.push(error.message);
},
});
pipe(writable);
abortStream = abort;
});
assertLog([
'Suspend! [A]',
'B', // TODO: Defer rendering the content after fallback if previous suspended,
'Loading A',
'Loading B',
]);
expect(getVisibleChildren(container)).toEqual(
<div>
<span>Loading A</span>
<span>Loading B</span>
</div>,
);
await serverAct(() => {
abortStream();
});
expect(hasCompleted).toBe(true);
expect(errors).toEqual([
'The render was aborted by the server without a reason.',
]);
});
// @gate enableSuspenseList
it('can error a pending SuspenseList', async () => {
const A = createAsyncText('A');
function Foo() {
return (
<div>
<SuspenseList revealOrder="forwards">
<Suspense fallback={<Text text="Loading A" />}>
<A />
</Suspense>
<Suspense fallback={<Text text="Loading B" />}>
<Text text="B" />
</Suspense>
</SuspenseList>
</div>
);
}
const errors = [];
await serverAct(async () => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<Foo />, {
onError(error) {
errors.push(error.message);
},
});
pipe(writable);
});
assertLog([
'Suspend! [A]',
'B', // TODO: Defer rendering the content after fallback if previous suspended,
'Loading A',
'Loading B',
]);
expect(getVisibleChildren(container)).toEqual(
<div>
<span>Loading A</span>
<span>Loading B</span>
</div>,
);
await serverAct(async () => {
A.reject(new Error('hi'));
});
assertLog(['Error! [hi]']);
expect(getVisibleChildren(container)).toEqual(
<div>
<span>Loading A</span>
<span>B</span>
</div>,
);
expect(errors).toEqual(['hi']);
expect(hasErrored).toBe(false);
expect(hasCompleted).toBe(true);
});
}); });

View File

@ -4392,6 +4392,14 @@ function erroredTask(
encodeErrorForBoundary(boundary, errorDigest, error, errorInfo, false); encodeErrorForBoundary(boundary, errorDigest, error, errorInfo, false);
untrackBoundary(request, boundary); untrackBoundary(request, boundary);
const boundaryRow = boundary.row;
if (boundaryRow !== null) {
// Unblock the SuspenseListRow that was blocked by this boundary.
if (--boundaryRow.pendingTasks === 0) {
finishSuspenseListRow(request, boundaryRow);
}
}
// Regardless of what happens next, this boundary won't be displayed, // Regardless of what happens next, this boundary won't be displayed,
// so we can flush it, if the parent already flushed. // so we can flush it, if the parent already flushed.
if (boundary.parentFlushed) { if (boundary.parentFlushed) {
@ -4544,13 +4552,6 @@ function abortTask(task: Task, request: Request, error: mixed): void {
segment.status = ABORTED; segment.status = ABORTED;
} }
const row = task.row;
if (row !== null) {
if (--row.pendingTasks === 0) {
finishSuspenseListRow(request, row);
}
}
const errorInfo = getThrownInfo(task.componentStack); const errorInfo = getThrownInfo(task.componentStack);
if (boundary === null) { if (boundary === null) {
@ -4573,7 +4574,7 @@ function abortTask(task: Task, request: Request, error: mixed): void {
// we just need to mark it as postponed. // we just need to mark it as postponed.
logPostpone(request, postponeInstance.message, errorInfo, null); logPostpone(request, postponeInstance.message, errorInfo, null);
trackPostpone(request, trackedPostpones, task, segment); trackPostpone(request, trackedPostpones, task, segment);
finishedTask(request, null, row, segment); finishedTask(request, null, task.row, segment);
} else { } else {
const fatal = new Error( const fatal = new Error(
'The render was aborted with postpone when the shell is incomplete. Reason: ' + 'The render was aborted with postpone when the shell is incomplete. Reason: ' +
@ -4592,7 +4593,7 @@ function abortTask(task: Task, request: Request, error: mixed): void {
// We log the error but we still resolve the prerender // We log the error but we still resolve the prerender
logRecoverableError(request, error, errorInfo, null); logRecoverableError(request, error, errorInfo, null);
trackPostpone(request, trackedPostpones, task, segment); trackPostpone(request, trackedPostpones, task, segment);
finishedTask(request, null, row, segment); finishedTask(request, null, task.row, segment);
} else { } else {
logRecoverableError(request, error, errorInfo, null); logRecoverableError(request, error, errorInfo, null);
fatalError(request, error, errorInfo, null); fatalError(request, error, errorInfo, null);
@ -4636,7 +4637,6 @@ function abortTask(task: Task, request: Request, error: mixed): void {
} }
} }
} else { } else {
boundary.pendingTasks--;
// We construct an errorInfo from the boundary's componentStack so the error in dev will indicate which // We construct an errorInfo from the boundary's componentStack so the error in dev will indicate which
// boundary the message is referring to // boundary the message is referring to
const trackedPostpones = request.trackedPostpones; const trackedPostpones = request.trackedPostpones;
@ -4664,7 +4664,7 @@ function abortTask(task: Task, request: Request, error: mixed): void {
abortTask(fallbackTask, request, error), abortTask(fallbackTask, request, error),
); );
boundary.fallbackAbortableTasks.clear(); boundary.fallbackAbortableTasks.clear();
return finishedTask(request, boundary, row, segment); return finishedTask(request, boundary, task.row, segment);
} }
} }
boundary.status = CLIENT_RENDERED; boundary.status = CLIENT_RENDERED;
@ -4681,7 +4681,7 @@ function abortTask(task: Task, request: Request, error: mixed): void {
logPostpone(request, postponeInstance.message, errorInfo, null); logPostpone(request, postponeInstance.message, errorInfo, null);
if (request.trackedPostpones !== null && segment !== null) { if (request.trackedPostpones !== null && segment !== null) {
trackPostpone(request, request.trackedPostpones, task, segment); trackPostpone(request, request.trackedPostpones, task, segment);
finishedTask(request, task.blockedBoundary, row, segment); finishedTask(request, task.blockedBoundary, task.row, segment);
// If this boundary was still pending then we haven't already cancelled its fallbacks. // If this boundary was still pending then we haven't already cancelled its fallbacks.
// We'll need to abort the fallbacks, which will also error that parent boundary. // We'll need to abort the fallbacks, which will also error that parent boundary.
@ -4706,6 +4706,16 @@ function abortTask(task: Task, request: Request, error: mixed): void {
} }
} }
boundary.pendingTasks--;
const boundaryRow = boundary.row;
if (boundaryRow !== null) {
// Unblock the SuspenseListRow that was blocked by this boundary.
if (--boundaryRow.pendingTasks === 0) {
finishSuspenseListRow(request, boundaryRow);
}
}
// If this boundary was still pending then we haven't already cancelled its fallbacks. // If this boundary was still pending then we haven't already cancelled its fallbacks.
// We'll need to abort the fallbacks, which will also error that parent boundary. // We'll need to abort the fallbacks, which will also error that parent boundary.
boundary.fallbackAbortableTasks.forEach(fallbackTask => boundary.fallbackAbortableTasks.forEach(fallbackTask =>
@ -4714,6 +4724,13 @@ function abortTask(task: Task, request: Request, error: mixed): void {
boundary.fallbackAbortableTasks.clear(); boundary.fallbackAbortableTasks.clear();
} }
const row = task.row;
if (row !== null) {
if (--row.pendingTasks === 0) {
finishSuspenseListRow(request, row);
}
}
request.allPendingTasks--; request.allPendingTasks--;
if (request.allPendingTasks === 0) { if (request.allPendingTasks === 0) {
completeAll(request); completeAll(request);