[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.
Sebastian Markbåge committed
Apr 14, 2021 at 16:49 UTC
a597c2f5dc5596bea331b455aca0548fb933038e
3 files changed
+77
-24
packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
+10
-7
@@ -353,13 +353,16 @@ describe('ReactDOMFizzServer', () => {
353
354
await act(async () => {
355
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>
366
</Suspense>,
367
writableA,
368
{
packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js
+47
-4
@@ -99,7 +99,7 @@ describe('ReactDOMFizzServer', () => {
99
}
100
return 'Done';
101
}
102
- let isComplete = false;
102
+ let isCompleteCalls = 0;
103
const {writable, output} = getTestWritable();
104
const {startWriting} = ReactDOMFizzServer.pipeToNodeWritable(
105
<div>
@@ -110,13 +110,13 @@ describe('ReactDOMFizzServer', () => {
110
writable,
111
{
112
onCompleteAll() {
113
- isComplete = true;
113
+ isCompleteCalls++;
114
},
115
},
116
);
117
await jest.runAllTimers();
118
expect(output.result).toBe('');
119
- expect(isComplete).toBe(false);
119
+ expect(isCompleteCalls).toBe(0);
120
// Resolve the loading.
121
hasLoaded = true;
122
await resolve();
@@ -124,7 +124,7 @@ describe('ReactDOMFizzServer', () => {
124
await jest.runAllTimers();
125
126
expect(output.result).toBe('');
127
- expect(isComplete).toBe(true);
127
+ expect(isCompleteCalls).toBe(1);
128
129
// First we write our header.
130
output.result +=
@@ -244,6 +244,7 @@ describe('ReactDOMFizzServer', () => {
244
245
// @gate experimental
246
it('should be able to complete by aborting even if the promise never resolves', async () => {
247
+ let isCompleteCalls = 0;
248
const {writable, output, completed} = getTestWritable();
249
const {startWriting, abort} = ReactDOMFizzServer.pipeToNodeWritable(
250
<div>
@@ -252,12 +253,53 @@ describe('ReactDOMFizzServer', () => {
253
</Suspense>
254
</div>,
255
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
+ },
296
);
297
startWriting();
298
299
jest.runAllTimers();
300
301
expect(output.result).toContain('Loading');
302
+ expect(isCompleteCalls).toBe(0);
303
304
abort();
305
@@ -265,5 +307,6 @@ describe('ReactDOMFizzServer', () => {
307
308
expect(output.error).toBe(undefined);
309
expect(output.result).toContain('Loading');
310
+ expect(isCompleteCalls).toBe(1);
311
});
312
});
packages/react-server/src/ReactFizzServer.js
+20
-13
@@ -1172,8 +1172,8 @@ function abortTask(task: Task): void {
1172
const segment = task.blockedSegment;
1173
segment.status = ABORTED;
1174
1175
- request.allPendingTasks--;
1175
if (boundary === null) {
1176
+ request.allPendingTasks--;
1177
// We didn't complete the root so we have nothing to show. We can close
1178
// the request;
1179
if (request.status !== CLOSED) {
@@ -1183,18 +1183,23 @@ function abortTask(task: Task): void {
1183
} else {
1184
boundary.pendingTasks--;
1185
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
+ }
1199
}
1200
}
1201
1202
+ request.allPendingTasks--;
1203
if (request.allPendingTasks === 0) {
1204
const onCompleteAll = request.onCompleteAll;
1205
onCompleteAll();
@@ -1226,9 +1231,6 @@ function finishedTask(
1231
// This already errored.
1232
} else if (boundary.pendingTasks === 0) {
1233
// 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();
1234
if (segment.parentFlushed) {
1235
// Our parent segment already flushed, so we need to schedule this segment to be emitted.
1236
boundary.completedSegments.push(segment);
@@ -1238,6 +1240,11 @@ function finishedTask(
1240
// parent flushed, we need to schedule the boundary to be emitted.
1241
request.completedBoundaries.push(boundary);
1242
}
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();
1248
} else {
1249
if (segment.parentFlushed) {
1250
// Our parent already flushed, so we need to schedule this segment to be emitted.