Skip to content

Commit f1af67e

Browse files
authored
[Fizz] Do not allow abort reentrancy (react#36574)
Aborting is a gate you can only pass through once. A request that is already aborting, already completed, or already fataled cannot be aborted a second time. Previously this was generally functionally true but you could contrive sequences where an onError would fire after a render fataled. This change makes it more explicit that this is not semantically correct by bailing out of abort if the request is in a status that cannot be aborted.
1 parent f0dfee3 commit f1af67e

3 files changed

Lines changed: 77 additions & 10 deletions

File tree

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

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4003,10 +4003,46 @@ describe('ReactDOMFizzServer', () => {
40034003
await act(() => pipe(testWritable));
40044004
expect(didRender).toBe(false);
40054005
expect(didFatal).toBe(didFatal);
4006-
expect(errors).toEqual([
4007-
'boom',
4008-
'The destination stream errored while writing data.',
4009-
]);
4006+
expect(errors).toEqual(['boom']);
4007+
});
4008+
4009+
it('does not report aborts after fatally erroring', async () => {
4010+
const promise = new Promise(() => {});
4011+
function AsyncComp() {
4012+
React.use(promise);
4013+
return 'Async';
4014+
}
4015+
4016+
function ErrorComp() {
4017+
throw new Error('boom');
4018+
}
4019+
4020+
const errors = [];
4021+
let abort;
4022+
await act(() => {
4023+
abort = renderToPipeableStream(
4024+
<div>
4025+
<Suspense fallback="loading...">
4026+
<AsyncComp />
4027+
</Suspense>
4028+
<ErrorComp />
4029+
</div>,
4030+
{
4031+
onError(error) {
4032+
errors.push(error.message);
4033+
},
4034+
onShellError() {},
4035+
},
4036+
).abort;
4037+
});
4038+
4039+
expect(errors).toEqual(['boom']);
4040+
4041+
await act(() => {
4042+
abort(new Error('too late'));
4043+
});
4044+
4045+
expect(errors).toEqual(['boom']);
40104046
});
40114047

40124048
describe('error escaping', () => {

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

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,37 @@ describe('ReactDOMFizzServerNode', () => {
237237
expect(reportedShellErrors).toEqual([theError]);
238238
});
239239

240+
it('should not report aborts after the shell has fatally errored', async () => {
241+
const reportedErrors = [];
242+
const reportedShellErrors = [];
243+
const {abort} = ReactDOMFizzServer.renderToPipeableStream(
244+
<div>
245+
<Suspense fallback="Loading">
246+
<InfiniteSuspend />
247+
</Suspense>
248+
<Throw />
249+
</div>,
250+
{
251+
onError(x) {
252+
reportedErrors.push(x);
253+
},
254+
onShellError(x) {
255+
reportedShellErrors.push(x);
256+
},
257+
},
258+
);
259+
260+
await jest.runAllTimers();
261+
262+
expect(reportedErrors).toEqual([theError]);
263+
expect(reportedShellErrors).toEqual([theError]);
264+
265+
abort(new Error('too late'));
266+
267+
expect(reportedErrors).toEqual([theError]);
268+
expect(reportedShellErrors).toEqual([theError]);
269+
});
270+
240271
it('should error the stream when an error is thrown inside a fallback', async () => {
241272
const reportedErrors = [];
242273
const reportedShellErrors = [];
@@ -263,10 +294,7 @@ describe('ReactDOMFizzServerNode', () => {
263294

264295
expect(output.error).toBe(theError);
265296
expect(output.result).toBe('');
266-
expect(reportedErrors).toEqual([
267-
theError.message,
268-
'The destination stream errored while writing data.',
269-
]);
297+
expect(reportedErrors).toEqual([theError.message]);
270298
expect(reportedShellErrors).toEqual([theError]);
271299
});
272300

packages/react-server/src/ReactFizzServer.js

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6143,9 +6143,12 @@ export function stopFlowing(request: Request): void {
61436143

61446144
// This is called to early terminate a request. It puts all pending boundaries in client rendered state.
61456145
export function abort(request: Request, reason: mixed): void {
6146-
if (request.status === OPEN || request.status === OPENING) {
6147-
request.status = ABORTING;
6146+
if (request.status !== OPEN && request.status !== OPENING) {
6147+
// Only requests that are not already complete or in the process of aborting
6148+
// can be aborted. in practice this makes abort callable at most once per render.
6149+
return;
61486150
}
6151+
request.status = ABORTING;
61496152

61506153
try {
61516154
const abortableTasks = request.abortableTasks;

0 commit comments

Comments
 (0)