@samitouri / QOS-React-2 / commits / 0f0aca3ab3

Aborting early should not infinitely suspend (#24751)

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.

Sebastian Markbåge committed Jun 18, 2022 at 11:01 UTC 0f0aca3ab35354040950ac0001fd4c01d70dceb4
5 files changed +181 -30
packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js
+111 -4
@@ -17,7 +17,7 @@ let React;
17 let ReactDOMFizzServer;
18 let Suspense;
19
20 -describe('ReactDOMFizzServer', () => {
20 +describe('ReactDOMFizzServerBrowser', () => {
21 beforeEach(() => {
22 jest.resetModules();
23 React = require('react');
@@ -209,6 +209,113 @@ describe('ReactDOMFizzServer', () => {
209 ]);
210 });
211
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 +
319 it('should not continue rendering after the reader cancels', async () => {
320 let hasLoaded = false;
321 let resolve;
@@ -226,7 +333,7 @@ describe('ReactDOMFizzServer', () => {
333 const stream = await ReactDOMFizzServer.renderToReadableStream(
334 <div>
335 <Suspense fallback={<div>Loading</div>}>
229 - <Wait /> />
336 + <Wait />
337 </Suspense>
338 </div>,
339 {
@@ -296,7 +403,7 @@ describe('ReactDOMFizzServer', () => {
403 expect(result).toMatchInlineSnapshot(`"<div>${str2049}</div>"`);
404 });
405
299 - it('Supports custom abort reasons with a string', async () => {
406 + it('supports custom abort reasons with a string', async () => {
407 const promise = new Promise(r => {});
408 function Wait() {
409 throw promise;
@@ -337,7 +444,7 @@ describe('ReactDOMFizzServer', () => {
444 expect(errors).toEqual(['foobar', 'foobar']);
445 });
446
340 - it('Supports custom abort reasons with an Error', async () => {
447 + it('supports custom abort reasons with an Error', async () => {
448 const promise = new Promise(r => {});
449 function Wait() {
450 throw promise;
packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js
+44 -5
@@ -15,7 +15,7 @@ let React;
15 let ReactDOMFizzServer;
16 let Suspense;
17
18 -describe('ReactDOMFizzServer', () => {
18 +describe('ReactDOMFizzServerNode', () => {
19 beforeEach(() => {
20 jest.resetModules();
21 React = require('react');
@@ -166,7 +166,6 @@ describe('ReactDOMFizzServer', () => {
166 <div>
167 <Throw />
168 </div>,
169 -
169 {
170 onError(x) {
171 reportedErrors.push(x);
@@ -232,7 +231,6 @@ describe('ReactDOMFizzServer', () => {
231 <Throw />
232 </Suspense>
233 </div>,
235 -
234 {
235 onError(x) {
236 reportedErrors.push(x);
@@ -288,7 +286,6 @@ describe('ReactDOMFizzServer', () => {
286 <InfiniteSuspend />
287 </Suspense>
288 </div>,
291 -
289 {
290 onError(x) {
291 errors.push(x.message);
@@ -315,6 +312,49 @@ describe('ReactDOMFizzServer', () => {
312 expect(isCompleteCalls).toBe(1);
313 });
314
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 +
358 it('should be able to complete by abort when the fallback is also suspended', async () => {
359 let isCompleteCalls = 0;
360 const errors = [];
@@ -327,7 +367,6 @@ describe('ReactDOMFizzServer', () => {
367 </Suspense>
368 </Suspense>
369 </div>,
330 -
370 {
371 onError(x) {
372 errors.push(x.message);
packages/react-dom/src/server/ReactDOMFizzServerBrowser.js
+8 -4
@@ -96,11 +96,15 @@ function renderToReadableStream(
96 );
97 if (options && options.signal) {
98 const signal = options.signal;
99 - const listener = () => {
99 + if (signal.aborted) {
100 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 + }
108 }
109 startWork(request);
110 });
packages/react-dom/src/server/ReactDOMLegacyServerImpl.js
+1 -1
@@ -76,7 +76,7 @@ function renderToStringImpl(
76 // That way we write only client-rendered boundaries from the start.
77 abort(request, abortReason);
78 startFlowing(request, destination);
79 - if (didFatal) {
79 + if (didFatal && fatalError !== abortReason) {
80 throw fatalError;
81 }
82
packages/react-server/src/ReactFizzServer.js
+17 -16
@@ -1530,7 +1530,7 @@ function abortTaskSoft(task: Task): void {
1530 finishedTask(request, boundary, segment);
1531 }
1532
1533 -function abortTask(task: Task, request: Request, reason: mixed): void {
1533 +function abortTask(task: Task, request: Request, error: mixed): void {
1534 // This aborts the task and aborts the parent that it blocks, putting it into
1535 // client rendered mode.
1536 const boundary = task.blockedBoundary;
@@ -1541,35 +1541,30 @@ function abortTask(task: Task, request: Request, reason: mixed): void {
1541 request.allPendingTasks--;
1542 // We didn't complete the root so we have nothing to show. We can close
1543 // 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);
1547 }
1548 } else {
1549 boundary.pendingTasks--;
1550
1551 if (!boundary.forceClientRender) {
1552 boundary.forceClientRender = true;
1555 - let error =
1556 - reason === undefined
1557 - ? new Error('The render was aborted by the server without a reason.')
1558 - : reason;
1553 boundary.errorDigest = request.onError(error);
1554 if (__DEV__) {
1555 const errorPrefix =
1556 'The server did not finish this Suspense boundary: ';
1557 + let errorMessage;
1558 if (error && typeof error.message === 'string') {
1564 - error = errorPrefix + error.message;
1559 + errorMessage = errorPrefix + error.message;
1560 } else {
1561 // eslint-disable-next-line react-internal/safe-string-coercion
1567 - error = errorPrefix + String(error);
1562 + errorMessage = errorPrefix + String(error);
1563 }
1564 const previousTaskInDev = currentTaskInDEV;
1565 currentTaskInDEV = task;
1566 try {
1572 - captureBoundaryErrorDetailsDev(boundary, error);
1567 + captureBoundaryErrorDetailsDev(boundary, errorMessage);
1568 } finally {
1569 currentTaskInDEV = previousTaskInDev;
1570 }
@@ -1582,7 +1577,7 @@ function abortTask(task: Task, request: Request, reason: mixed): void {
1577 // If this boundary was still pending then we haven't already cancelled its fallbacks.
1578 // We'll need to abort the fallbacks, which will also error that parent boundary.
1579 boundary.fallbackAbortableTasks.forEach(fallbackTask =>
1585 - abortTask(fallbackTask, request, reason),
1580 + abortTask(fallbackTask, request, error),
1581 );
1582 boundary.fallbackAbortableTasks.clear();
1583
@@ -2178,8 +2173,14 @@ export function startFlowing(request: Request, destination: Destination): void {
2173 export function abort(request: Request, reason: mixed): void {
2174 try {
2175 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 + }
2184 if (request.destination !== null) {
2185 flushCompletedQueues(request, request.destination);
2186 }