Skip to content

Commit a597c2f

Browse files
authored
[Fizz] Fix reentrancy bug (#21270)
* Fix reentrancy bug * Fix another reentrancy bug There's also an issue if we try to schedule something to be client rendered if its fallback hasn't rendered yet. So we don't do it in that case.
1 parent 6809778 commit a597c2f

File tree

3 files changed

+77
-24
lines changed

3 files changed

+77
-24
lines changed

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

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -353,13 +353,16 @@ describe('ReactDOMFizzServer', () => {
353353

354354
await act(async () => {
355355
const {startWriting} = ReactDOMFizzServer.pipeToNodeWritable(
356-
<Suspense fallback={<Text text="Loading A..." />}>
357-
<>
358-
<Text text="This will show A: " />
359-
<div>
360-
<AsyncText text="A" />
361-
</div>
362-
</>
356+
// We use two nested boundaries to flush out coverage of an old reentrancy bug.
357+
<Suspense fallback="Loading...">
358+
<Suspense fallback={<Text text="Loading A..." />}>
359+
<>
360+
<Text text="This will show A: " />
361+
<div>
362+
<AsyncText text="A" />
363+
</div>
364+
</>
365+
</Suspense>
363366
</Suspense>,
364367
writableA,
365368
{

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

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ describe('ReactDOMFizzServer', () => {
9999
}
100100
return 'Done';
101101
}
102-
let isComplete = false;
102+
let isCompleteCalls = 0;
103103
const {writable, output} = getTestWritable();
104104
const {startWriting} = ReactDOMFizzServer.pipeToNodeWritable(
105105
<div>
@@ -110,21 +110,21 @@ describe('ReactDOMFizzServer', () => {
110110
writable,
111111
{
112112
onCompleteAll() {
113-
isComplete = true;
113+
isCompleteCalls++;
114114
},
115115
},
116116
);
117117
await jest.runAllTimers();
118118
expect(output.result).toBe('');
119-
expect(isComplete).toBe(false);
119+
expect(isCompleteCalls).toBe(0);
120120
// Resolve the loading.
121121
hasLoaded = true;
122122
await resolve();
123123

124124
await jest.runAllTimers();
125125

126126
expect(output.result).toBe('');
127-
expect(isComplete).toBe(true);
127+
expect(isCompleteCalls).toBe(1);
128128

129129
// First we write our header.
130130
output.result +=
@@ -244,6 +244,7 @@ describe('ReactDOMFizzServer', () => {
244244

245245
// @gate experimental
246246
it('should be able to complete by aborting even if the promise never resolves', async () => {
247+
let isCompleteCalls = 0;
247248
const {writable, output, completed} = getTestWritable();
248249
const {startWriting, abort} = ReactDOMFizzServer.pipeToNodeWritable(
249250
<div>
@@ -252,18 +253,60 @@ describe('ReactDOMFizzServer', () => {
252253
</Suspense>
253254
</div>,
254255
writable,
256+
{
257+
onCompleteAll() {
258+
isCompleteCalls++;
259+
},
260+
},
261+
);
262+
startWriting();
263+
264+
jest.runAllTimers();
265+
266+
expect(output.result).toContain('Loading');
267+
expect(isCompleteCalls).toBe(0);
268+
269+
abort();
270+
271+
await completed;
272+
273+
expect(output.error).toBe(undefined);
274+
expect(output.result).toContain('Loading');
275+
expect(isCompleteCalls).toBe(1);
276+
});
277+
278+
// @gate experimental
279+
it('should be able to complete by abort when the fallback is also suspended', async () => {
280+
let isCompleteCalls = 0;
281+
const {writable, output, completed} = getTestWritable();
282+
const {startWriting, abort} = ReactDOMFizzServer.pipeToNodeWritable(
283+
<div>
284+
<Suspense fallback="Loading">
285+
<Suspense fallback={<InfiniteSuspend />}>
286+
<InfiniteSuspend />
287+
</Suspense>
288+
</Suspense>
289+
</div>,
290+
writable,
291+
{
292+
onCompleteAll() {
293+
isCompleteCalls++;
294+
},
295+
},
255296
);
256297
startWriting();
257298

258299
jest.runAllTimers();
259300

260301
expect(output.result).toContain('Loading');
302+
expect(isCompleteCalls).toBe(0);
261303

262304
abort();
263305

264306
await completed;
265307

266308
expect(output.error).toBe(undefined);
267309
expect(output.result).toContain('Loading');
310+
expect(isCompleteCalls).toBe(1);
268311
});
269312
});

packages/react-server/src/ReactFizzServer.js

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1172,8 +1172,8 @@ function abortTask(task: Task): void {
11721172
const segment = task.blockedSegment;
11731173
segment.status = ABORTED;
11741174

1175-
request.allPendingTasks--;
11761175
if (boundary === null) {
1176+
request.allPendingTasks--;
11771177
// We didn't complete the root so we have nothing to show. We can close
11781178
// the request;
11791179
if (request.status !== CLOSED) {
@@ -1183,18 +1183,23 @@ function abortTask(task: Task): void {
11831183
} else {
11841184
boundary.pendingTasks--;
11851185

1186-
// If this boundary was still pending then we haven't already cancelled its fallbacks.
1187-
// We'll need to abort the fallbacks, which will also error that parent boundary.
1188-
boundary.fallbackAbortableTasks.forEach(abortTask, request);
1189-
boundary.fallbackAbortableTasks.clear();
1190-
1191-
if (!boundary.forceClientRender) {
1192-
boundary.forceClientRender = true;
1193-
if (boundary.parentFlushed) {
1194-
request.clientRenderedBoundaries.push(boundary);
1186+
if (boundary.fallbackAbortableTasks.size > 0) {
1187+
// If this boundary was still pending then we haven't already cancelled its fallbacks.
1188+
// We'll need to abort the fallbacks, which will also error that parent boundary.
1189+
// This means that we don't have to client render this boundary because its parent
1190+
// will be client rendered anyway.
1191+
boundary.fallbackAbortableTasks.forEach(abortTask, request);
1192+
boundary.fallbackAbortableTasks.clear();
1193+
} else {
1194+
if (!boundary.forceClientRender) {
1195+
boundary.forceClientRender = true;
1196+
if (boundary.parentFlushed) {
1197+
request.clientRenderedBoundaries.push(boundary);
1198+
}
11951199
}
11961200
}
11971201

1202+
request.allPendingTasks--;
11981203
if (request.allPendingTasks === 0) {
11991204
const onCompleteAll = request.onCompleteAll;
12001205
onCompleteAll();
@@ -1226,9 +1231,6 @@ function finishedTask(
12261231
// This already errored.
12271232
} else if (boundary.pendingTasks === 0) {
12281233
// This must have been the last segment we were waiting on. This boundary is now complete.
1229-
// We can now cancel any pending task on the fallback since we won't need to show it anymore.
1230-
boundary.fallbackAbortableTasks.forEach(abortTaskSoft, request);
1231-
boundary.fallbackAbortableTasks.clear();
12321234
if (segment.parentFlushed) {
12331235
// Our parent segment already flushed, so we need to schedule this segment to be emitted.
12341236
boundary.completedSegments.push(segment);
@@ -1238,6 +1240,11 @@ function finishedTask(
12381240
// parent flushed, we need to schedule the boundary to be emitted.
12391241
request.completedBoundaries.push(boundary);
12401242
}
1243+
// We can now cancel any pending task on the fallback since we won't need to show it anymore.
1244+
// This needs to happen after we read the parentFlushed flags because aborting can finish
1245+
// work which can trigger user code, which can start flushing, which can change those flags.
1246+
boundary.fallbackAbortableTasks.forEach(abortTaskSoft, request);
1247+
boundary.fallbackAbortableTasks.clear();
12411248
} else {
12421249
if (segment.parentFlushed) {
12431250
// Our parent already flushed, so we need to schedule this segment to be emitted.

0 commit comments

Comments
 (0)