Skip to content

Commit 728970d

Browse files
sebmarkbagesebmarkbage
committed
Aborting early should not infinitely suspend
Before this change we weren't calling onError nor onFatalError if you abort before completing the shell. This means that the render never completes and hangs. Aborting early can happen before even creating the stream for AbortSignal, before rendering starts in Node since there's an setImmediate atm, or during rendering.
1 parent 72ebc70 commit 728970d

4 files changed

Lines changed: 180 additions & 29 deletions

File tree

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

Lines changed: 111 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ let React;
1717
let ReactDOMFizzServer;
1818
let Suspense;
1919

20-
describe('ReactDOMFizzServer', () => {
20+
describe('ReactDOMFizzServerBrowser', () => {
2121
beforeEach(() => {
2222
jest.resetModules();
2323
React = require('react');
@@ -209,6 +209,113 @@ describe('ReactDOMFizzServer', () => {
209209
]);
210210
});
211211

212+
it('should reject if aborting before the shell is complete', async () => {
213+
const errors = [];
214+
const controller = new AbortController();
215+
const promise = ReactDOMFizzServer.renderToReadableStream(
216+
<div>
217+
<InfiniteSuspend />
218+
</div>,
219+
{
220+
signal: controller.signal,
221+
onError(x) {
222+
errors.push(x.message);
223+
},
224+
},
225+
);
226+
227+
await jest.runAllTimers();
228+
229+
const theReason = new Error('aborted for reasons');
230+
// @TODO this is a hack to work around lack of support for abortSignal.reason in node
231+
// The abort call itself should set this property but since we are testing in node we
232+
// set it here manually
233+
controller.signal.reason = theReason;
234+
controller.abort(theReason);
235+
236+
let caughtError = null;
237+
try {
238+
await promise;
239+
} catch (error) {
240+
caughtError = error;
241+
}
242+
expect(caughtError).toBe(theReason);
243+
expect(errors).toEqual(['aborted for reasons']);
244+
});
245+
246+
it('should be able to abort before something suspends', async () => {
247+
const errors = [];
248+
const controller = new AbortController();
249+
function App() {
250+
controller.abort();
251+
return (
252+
<Suspense fallback={<div>Loading</div>}>
253+
<InfiniteSuspend />
254+
</Suspense>
255+
);
256+
}
257+
const streamPromise = ReactDOMFizzServer.renderToReadableStream(
258+
<div>
259+
<App />
260+
</div>,
261+
{
262+
signal: controller.signal,
263+
onError(x) {
264+
errors.push(x.message);
265+
},
266+
},
267+
);
268+
269+
let caughtError = null;
270+
try {
271+
await streamPromise;
272+
} catch (error) {
273+
caughtError = error;
274+
}
275+
expect(caughtError.message).toBe(
276+
'The render was aborted by the server without a reason.',
277+
);
278+
expect(errors).toEqual([
279+
'The render was aborted by the server without a reason.',
280+
]);
281+
});
282+
283+
it('should reject if passing an already aborted signal', async () => {
284+
const errors = [];
285+
const controller = new AbortController();
286+
const theReason = new Error('aborted for reasons');
287+
// @TODO this is a hack to work around lack of support for abortSignal.reason in node
288+
// The abort call itself should set this property but since we are testing in node we
289+
// set it here manually
290+
controller.signal.reason = theReason;
291+
controller.abort(theReason);
292+
293+
const promise = ReactDOMFizzServer.renderToReadableStream(
294+
<div>
295+
<Suspense fallback={<div>Loading</div>}>
296+
<InfiniteSuspend />
297+
</Suspense>
298+
</div>,
299+
{
300+
signal: controller.signal,
301+
onError(x) {
302+
errors.push(x.message);
303+
},
304+
},
305+
);
306+
307+
// Technically we could still continue rendering the shell but currently the
308+
// semantics mean that we also abort any pending CPU work.
309+
let caughtError = null;
310+
try {
311+
await promise;
312+
} catch (error) {
313+
caughtError = error;
314+
}
315+
expect(caughtError).toBe(theReason);
316+
expect(errors).toEqual(['aborted for reasons']);
317+
});
318+
212319
it('should not continue rendering after the reader cancels', async () => {
213320
let hasLoaded = false;
214321
let resolve;
@@ -226,7 +333,7 @@ describe('ReactDOMFizzServer', () => {
226333
const stream = await ReactDOMFizzServer.renderToReadableStream(
227334
<div>
228335
<Suspense fallback={<div>Loading</div>}>
229-
<Wait /> />
336+
<Wait />
230337
</Suspense>
231338
</div>,
232339
{
@@ -296,7 +403,7 @@ describe('ReactDOMFizzServer', () => {
296403
expect(result).toMatchInlineSnapshot(`"<div>${str2049}</div>"`);
297404
});
298405

299-
it('Supports custom abort reasons with a string', async () => {
406+
it('supports custom abort reasons with a string', async () => {
300407
const promise = new Promise(r => {});
301408
function Wait() {
302409
throw promise;
@@ -337,7 +444,7 @@ describe('ReactDOMFizzServer', () => {
337444
expect(errors).toEqual(['foobar', 'foobar']);
338445
});
339446

340-
it('Supports custom abort reasons with an Error', async () => {
447+
it('supports custom abort reasons with an Error', async () => {
341448
const promise = new Promise(r => {});
342449
function Wait() {
343450
throw promise;

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

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ let React;
1515
let ReactDOMFizzServer;
1616
let Suspense;
1717

18-
describe('ReactDOMFizzServer', () => {
18+
describe('ReactDOMFizzServerNode', () => {
1919
beforeEach(() => {
2020
jest.resetModules();
2121
React = require('react');
@@ -166,7 +166,6 @@ describe('ReactDOMFizzServer', () => {
166166
<div>
167167
<Throw />
168168
</div>,
169-
170169
{
171170
onError(x) {
172171
reportedErrors.push(x);
@@ -232,7 +231,6 @@ describe('ReactDOMFizzServer', () => {
232231
<Throw />
233232
</Suspense>
234233
</div>,
235-
236234
{
237235
onError(x) {
238236
reportedErrors.push(x);
@@ -288,7 +286,6 @@ describe('ReactDOMFizzServer', () => {
288286
<InfiniteSuspend />
289287
</Suspense>
290288
</div>,
291-
292289
{
293290
onError(x) {
294291
errors.push(x.message);
@@ -315,6 +312,49 @@ describe('ReactDOMFizzServer', () => {
315312
expect(isCompleteCalls).toBe(1);
316313
});
317314

315+
it('should fail the shell if you abort before work has begun', async () => {
316+
let isCompleteCalls = 0;
317+
const errors = [];
318+
const shellErrors = [];
319+
const {writable, output, completed} = getTestWritable();
320+
const {pipe, abort} = ReactDOMFizzServer.renderToPipeableStream(
321+
<div>
322+
<Suspense fallback={<div>Loading</div>}>
323+
<InfiniteSuspend />
324+
</Suspense>
325+
</div>,
326+
{
327+
onError(x) {
328+
errors.push(x.message);
329+
},
330+
onShellError(x) {
331+
shellErrors.push(x.message);
332+
},
333+
onAllReady() {
334+
isCompleteCalls++;
335+
},
336+
},
337+
);
338+
pipe(writable);
339+
340+
// Currently we delay work so if we abort, we abort the remaining CPU
341+
// work as well.
342+
343+
// Abort before running the timers that perform the work
344+
const theReason = new Error('uh oh');
345+
abort(theReason);
346+
347+
jest.runAllTimers();
348+
349+
await completed;
350+
351+
expect(errors).toEqual(['uh oh']);
352+
expect(shellErrors).toEqual(['uh oh']);
353+
expect(output.error).toBe(theReason);
354+
expect(output.result).toBe('');
355+
expect(isCompleteCalls).toBe(0);
356+
});
357+
318358
it('should be able to complete by abort when the fallback is also suspended', async () => {
319359
let isCompleteCalls = 0;
320360
const errors = [];
@@ -327,7 +367,6 @@ describe('ReactDOMFizzServer', () => {
327367
</Suspense>
328368
</Suspense>
329369
</div>,
330-
331370
{
332371
onError(x) {
333372
errors.push(x.message);

‎packages/react-dom/src/server/ReactDOMFizzServerBrowser.js‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,15 @@ function renderToReadableStream(
9696
);
9797
if (options && options.signal) {
9898
const signal = options.signal;
99-
const listener = () => {
99+
if (signal.aborted) {
100100
abort(request, (signal: any).reason);
101-
signal.removeEventListener('abort', listener);
102-
};
103-
signal.addEventListener('abort', listener);
101+
} else {
102+
const listener = () => {
103+
abort(request, (signal: any).reason);
104+
signal.removeEventListener('abort', listener);
105+
};
106+
signal.addEventListener('abort', listener);
107+
}
104108
}
105109
startWork(request);
106110
});

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

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1530,7 +1530,7 @@ function abortTaskSoft(task: Task): void {
15301530
finishedTask(request, boundary, segment);
15311531
}
15321532

1533-
function abortTask(task: Task, request: Request, reason: mixed): void {
1533+
function abortTask(task: Task, request: Request, error: mixed): void {
15341534
// This aborts the task and aborts the parent that it blocks, putting it into
15351535
// client rendered mode.
15361536
const boundary = task.blockedBoundary;
@@ -1541,35 +1541,30 @@ function abortTask(task: Task, request: Request, reason: mixed): void {
15411541
request.allPendingTasks--;
15421542
// We didn't complete the root so we have nothing to show. We can close
15431543
// the request;
1544-
if (request.status !== CLOSED) {
1545-
request.status = CLOSED;
1546-
if (request.destination !== null) {
1547-
close(request.destination);
1548-
}
1544+
if (request.status !== CLOSING && request.status !== CLOSED) {
1545+
logRecoverableError(request, error);
1546+
fatalError(request, error);
15491547
}
15501548
} else {
15511549
boundary.pendingTasks--;
15521550

15531551
if (!boundary.forceClientRender) {
15541552
boundary.forceClientRender = true;
1555-
let error =
1556-
reason === undefined
1557-
? new Error('The render was aborted by the server without a reason.')
1558-
: reason;
15591553
boundary.errorDigest = request.onError(error);
15601554
if (__DEV__) {
15611555
const errorPrefix =
15621556
'The server did not finish this Suspense boundary: ';
1557+
let errorMessage;
15631558
if (error && typeof error.message === 'string') {
1564-
error = errorPrefix + error.message;
1559+
errorMessage = errorPrefix + error.message;
15651560
} else {
15661561
// eslint-disable-next-line react-internal/safe-string-coercion
1567-
error = errorPrefix + String(error);
1562+
errorMessage = errorPrefix + String(error);
15681563
}
15691564
const previousTaskInDev = currentTaskInDEV;
15701565
currentTaskInDEV = task;
15711566
try {
1572-
captureBoundaryErrorDetailsDev(boundary, error);
1567+
captureBoundaryErrorDetailsDev(boundary, errorMessage);
15731568
} finally {
15741569
currentTaskInDEV = previousTaskInDev;
15751570
}
@@ -1582,7 +1577,7 @@ function abortTask(task: Task, request: Request, reason: mixed): void {
15821577
// If this boundary was still pending then we haven't already cancelled its fallbacks.
15831578
// We'll need to abort the fallbacks, which will also error that parent boundary.
15841579
boundary.fallbackAbortableTasks.forEach(fallbackTask =>
1585-
abortTask(fallbackTask, request, reason),
1580+
abortTask(fallbackTask, request, error),
15861581
);
15871582
boundary.fallbackAbortableTasks.clear();
15881583

@@ -2178,8 +2173,14 @@ export function startFlowing(request: Request, destination: Destination): void {
21782173
export function abort(request: Request, reason: mixed): void {
21792174
try {
21802175
const abortableTasks = request.abortableTasks;
2181-
abortableTasks.forEach(task => abortTask(task, request, reason));
2182-
abortableTasks.clear();
2176+
if (abortableTasks.size > 0) {
2177+
const error =
2178+
reason === undefined
2179+
? new Error('The render was aborted by the server without a reason.')
2180+
: reason;
2181+
abortableTasks.forEach(task => abortTask(task, request, error));
2182+
abortableTasks.clear();
2183+
}
21832184
if (request.destination !== null) {
21842185
flushCompletedQueues(request, request.destination);
21852186
}

0 commit comments

Comments
 (0)