Skip to content

Commit 9c7b10e

Browse files
sebmarkbagesebmarkbage
authored
[Fizz] Clean up row that was blocked by an aborted boundary (facebook#33318)
Fixes a bug that we caused us to hang after an abort because we didn't manage the ref count correctly.
1 parent 50389e1 commit 9c7b10e

File tree

2 files changed

+160
-13
lines changed

2 files changed

+160
-13
lines changed

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

Lines changed: 131 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,10 @@ let writable;
2727
let container;
2828
let buffer = '';
2929
let hasErrored = false;
30+
let hasCompleted = false;
3031
let fatalError = undefined;
3132

32-
describe('ReactDOMFizSuspenseList', () => {
33+
describe('ReactDOMFizzSuspenseList', () => {
3334
beforeEach(() => {
3435
jest.resetModules();
3536
JSDOM = require('jsdom').JSDOM;
@@ -59,6 +60,7 @@ describe('ReactDOMFizSuspenseList', () => {
5960

6061
buffer = '';
6162
hasErrored = false;
63+
hasCompleted = false;
6264

6365
writable = new Stream.PassThrough();
6466
writable.setEncoding('utf8');
@@ -69,6 +71,9 @@ describe('ReactDOMFizSuspenseList', () => {
6971
hasErrored = true;
7072
fatalError = error;
7173
});
74+
writable.on('finish', () => {
75+
hasCompleted = true;
76+
});
7277
});
7378

7479
afterEach(() => {
@@ -103,7 +108,12 @@ describe('ReactDOMFizSuspenseList', () => {
103108

104109
function createAsyncText(text) {
105110
let resolved = false;
111+
let error = undefined;
106112
const Component = function () {
113+
if (error !== undefined) {
114+
Scheduler.log('Error! [' + error.message + ']');
115+
throw error;
116+
}
107117
if (!resolved) {
108118
Scheduler.log('Suspend! [' + text + ']');
109119
throw promise;
@@ -115,6 +125,10 @@ describe('ReactDOMFizSuspenseList', () => {
115125
resolved = true;
116126
return resolve();
117127
};
128+
Component.reject = function (e) {
129+
error = e;
130+
return resolve();
131+
};
118132
});
119133
return Component;
120134
}
@@ -714,4 +728,120 @@ describe('ReactDOMFizSuspenseList', () => {
714728
</div>,
715729
);
716730
});
731+
732+
// @gate enableSuspenseList
733+
it('can abort a pending SuspenseList', async () => {
734+
const A = createAsyncText('A');
735+
736+
function Foo() {
737+
return (
738+
<div>
739+
<SuspenseList revealOrder="forwards">
740+
<Suspense fallback={<Text text="Loading A" />}>
741+
<A />
742+
</Suspense>
743+
<Suspense fallback={<Text text="Loading B" />}>
744+
<Text text="B" />
745+
</Suspense>
746+
</SuspenseList>
747+
</div>
748+
);
749+
}
750+
751+
const errors = [];
752+
let abortStream;
753+
await serverAct(async () => {
754+
const {pipe, abort} = ReactDOMFizzServer.renderToPipeableStream(<Foo />, {
755+
onError(error) {
756+
errors.push(error.message);
757+
},
758+
});
759+
pipe(writable);
760+
abortStream = abort;
761+
});
762+
763+
assertLog([
764+
'Suspend! [A]',
765+
'B', // TODO: Defer rendering the content after fallback if previous suspended,
766+
'Loading A',
767+
'Loading B',
768+
]);
769+
770+
expect(getVisibleChildren(container)).toEqual(
771+
<div>
772+
<span>Loading A</span>
773+
<span>Loading B</span>
774+
</div>,
775+
);
776+
777+
await serverAct(() => {
778+
abortStream();
779+
});
780+
781+
expect(hasCompleted).toBe(true);
782+
expect(errors).toEqual([
783+
'The render was aborted by the server without a reason.',
784+
]);
785+
});
786+
787+
// @gate enableSuspenseList
788+
it('can error a pending SuspenseList', async () => {
789+
const A = createAsyncText('A');
790+
791+
function Foo() {
792+
return (
793+
<div>
794+
<SuspenseList revealOrder="forwards">
795+
<Suspense fallback={<Text text="Loading A" />}>
796+
<A />
797+
</Suspense>
798+
<Suspense fallback={<Text text="Loading B" />}>
799+
<Text text="B" />
800+
</Suspense>
801+
</SuspenseList>
802+
</div>
803+
);
804+
}
805+
806+
const errors = [];
807+
await serverAct(async () => {
808+
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<Foo />, {
809+
onError(error) {
810+
errors.push(error.message);
811+
},
812+
});
813+
pipe(writable);
814+
});
815+
816+
assertLog([
817+
'Suspend! [A]',
818+
'B', // TODO: Defer rendering the content after fallback if previous suspended,
819+
'Loading A',
820+
'Loading B',
821+
]);
822+
823+
expect(getVisibleChildren(container)).toEqual(
824+
<div>
825+
<span>Loading A</span>
826+
<span>Loading B</span>
827+
</div>,
828+
);
829+
830+
await serverAct(async () => {
831+
A.reject(new Error('hi'));
832+
});
833+
834+
assertLog(['Error! [hi]']);
835+
836+
expect(getVisibleChildren(container)).toEqual(
837+
<div>
838+
<span>Loading A</span>
839+
<span>B</span>
840+
</div>,
841+
);
842+
843+
expect(errors).toEqual(['hi']);
844+
expect(hasErrored).toBe(false);
845+
expect(hasCompleted).toBe(true);
846+
});
717847
});

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

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4392,6 +4392,14 @@ function erroredTask(
43924392
encodeErrorForBoundary(boundary, errorDigest, error, errorInfo, false);
43934393
untrackBoundary(request, boundary);
43944394

4395+
const boundaryRow = boundary.row;
4396+
if (boundaryRow !== null) {
4397+
// Unblock the SuspenseListRow that was blocked by this boundary.
4398+
if (--boundaryRow.pendingTasks === 0) {
4399+
finishSuspenseListRow(request, boundaryRow);
4400+
}
4401+
}
4402+
43954403
// Regardless of what happens next, this boundary won't be displayed,
43964404
// so we can flush it, if the parent already flushed.
43974405
if (boundary.parentFlushed) {
@@ -4544,13 +4552,6 @@ function abortTask(task: Task, request: Request, error: mixed): void {
45444552
segment.status = ABORTED;
45454553
}
45464554

4547-
const row = task.row;
4548-
if (row !== null) {
4549-
if (--row.pendingTasks === 0) {
4550-
finishSuspenseListRow(request, row);
4551-
}
4552-
}
4553-
45544555
const errorInfo = getThrownInfo(task.componentStack);
45554556

45564557
if (boundary === null) {
@@ -4573,7 +4574,7 @@ function abortTask(task: Task, request: Request, error: mixed): void {
45734574
// we just need to mark it as postponed.
45744575
logPostpone(request, postponeInstance.message, errorInfo, null);
45754576
trackPostpone(request, trackedPostpones, task, segment);
4576-
finishedTask(request, null, row, segment);
4577+
finishedTask(request, null, task.row, segment);
45774578
} else {
45784579
const fatal = new Error(
45794580
'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 {
45924593
// We log the error but we still resolve the prerender
45934594
logRecoverableError(request, error, errorInfo, null);
45944595
trackPostpone(request, trackedPostpones, task, segment);
4595-
finishedTask(request, null, row, segment);
4596+
finishedTask(request, null, task.row, segment);
45964597
} else {
45974598
logRecoverableError(request, error, errorInfo, null);
45984599
fatalError(request, error, errorInfo, null);
@@ -4636,7 +4637,6 @@ function abortTask(task: Task, request: Request, error: mixed): void {
46364637
}
46374638
}
46384639
} else {
4639-
boundary.pendingTasks--;
46404640
// We construct an errorInfo from the boundary's componentStack so the error in dev will indicate which
46414641
// boundary the message is referring to
46424642
const trackedPostpones = request.trackedPostpones;
@@ -4664,7 +4664,7 @@ function abortTask(task: Task, request: Request, error: mixed): void {
46644664
abortTask(fallbackTask, request, error),
46654665
);
46664666
boundary.fallbackAbortableTasks.clear();
4667-
return finishedTask(request, boundary, row, segment);
4667+
return finishedTask(request, boundary, task.row, segment);
46684668
}
46694669
}
46704670
boundary.status = CLIENT_RENDERED;
@@ -4681,7 +4681,7 @@ function abortTask(task: Task, request: Request, error: mixed): void {
46814681
logPostpone(request, postponeInstance.message, errorInfo, null);
46824682
if (request.trackedPostpones !== null && segment !== null) {
46834683
trackPostpone(request, request.trackedPostpones, task, segment);
4684-
finishedTask(request, task.blockedBoundary, row, segment);
4684+
finishedTask(request, task.blockedBoundary, task.row, segment);
46854685

46864686
// If this boundary was still pending then we haven't already cancelled its fallbacks.
46874687
// 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 {
47064706
}
47074707
}
47084708

4709+
boundary.pendingTasks--;
4710+
4711+
const boundaryRow = boundary.row;
4712+
if (boundaryRow !== null) {
4713+
// Unblock the SuspenseListRow that was blocked by this boundary.
4714+
if (--boundaryRow.pendingTasks === 0) {
4715+
finishSuspenseListRow(request, boundaryRow);
4716+
}
4717+
}
4718+
47094719
// If this boundary was still pending then we haven't already cancelled its fallbacks.
47104720
// We'll need to abort the fallbacks, which will also error that parent boundary.
47114721
boundary.fallbackAbortableTasks.forEach(fallbackTask =>
@@ -4714,6 +4724,13 @@ function abortTask(task: Task, request: Request, error: mixed): void {
47144724
boundary.fallbackAbortableTasks.clear();
47154725
}
47164726

4727+
const row = task.row;
4728+
if (row !== null) {
4729+
if (--row.pendingTasks === 0) {
4730+
finishSuspenseListRow(request, row);
4731+
}
4732+
}
4733+
47174734
request.allPendingTasks--;
47184735
if (request.allPendingTasks === 0) {
47194736
completeAll(request);

0 commit comments

Comments
 (0)