From 0f0aca3ab35354040950ac0001fd4c01d70dceb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Sat, 18 Jun 2022 11:01:58 -0400 Subject: [PATCH] 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. --- .../ReactDOMFizzServerBrowser-test.js | 115 +++++++++++++++++- .../__tests__/ReactDOMFizzServerNode-test.js | 49 +++++++- .../src/server/ReactDOMFizzServerBrowser.js | 12 +- .../src/server/ReactDOMLegacyServerImpl.js | 2 +- packages/react-server/src/ReactFizzServer.js | 33 ++--- 5 files changed, 181 insertions(+), 30 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js index af5f3d9b3565a..3df7eb79de6cc 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js @@ -17,7 +17,7 @@ let React; let ReactDOMFizzServer; let Suspense; -describe('ReactDOMFizzServer', () => { +describe('ReactDOMFizzServerBrowser', () => { beforeEach(() => { jest.resetModules(); React = require('react'); @@ -209,6 +209,113 @@ describe('ReactDOMFizzServer', () => { ]); }); + it('should reject if aborting before the shell is complete', async () => { + const errors = []; + const controller = new AbortController(); + const promise = ReactDOMFizzServer.renderToReadableStream( +
+ +
, + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ); + + await jest.runAllTimers(); + + const theReason = new Error('aborted for reasons'); + // @TODO this is a hack to work around lack of support for abortSignal.reason in node + // The abort call itself should set this property but since we are testing in node we + // set it here manually + controller.signal.reason = theReason; + controller.abort(theReason); + + let caughtError = null; + try { + await promise; + } catch (error) { + caughtError = error; + } + expect(caughtError).toBe(theReason); + expect(errors).toEqual(['aborted for reasons']); + }); + + it('should be able to abort before something suspends', async () => { + const errors = []; + const controller = new AbortController(); + function App() { + controller.abort(); + return ( + Loading}> + + + ); + } + const streamPromise = ReactDOMFizzServer.renderToReadableStream( +
+ +
, + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ); + + let caughtError = null; + try { + await streamPromise; + } catch (error) { + caughtError = error; + } + expect(caughtError.message).toBe( + 'The render was aborted by the server without a reason.', + ); + expect(errors).toEqual([ + 'The render was aborted by the server without a reason.', + ]); + }); + + it('should reject if passing an already aborted signal', async () => { + const errors = []; + const controller = new AbortController(); + const theReason = new Error('aborted for reasons'); + // @TODO this is a hack to work around lack of support for abortSignal.reason in node + // The abort call itself should set this property but since we are testing in node we + // set it here manually + controller.signal.reason = theReason; + controller.abort(theReason); + + const promise = ReactDOMFizzServer.renderToReadableStream( +
+ Loading
}> + + + , + { + signal: controller.signal, + onError(x) { + errors.push(x.message); + }, + }, + ); + + // Technically we could still continue rendering the shell but currently the + // semantics mean that we also abort any pending CPU work. + let caughtError = null; + try { + await promise; + } catch (error) { + caughtError = error; + } + expect(caughtError).toBe(theReason); + expect(errors).toEqual(['aborted for reasons']); + }); + it('should not continue rendering after the reader cancels', async () => { let hasLoaded = false; let resolve; @@ -226,7 +333,7 @@ describe('ReactDOMFizzServer', () => { const stream = await ReactDOMFizzServer.renderToReadableStream(
Loading
}> - /> + , { @@ -296,7 +403,7 @@ describe('ReactDOMFizzServer', () => { expect(result).toMatchInlineSnapshot(`"
${str2049}
"`); }); - it('Supports custom abort reasons with a string', async () => { + it('supports custom abort reasons with a string', async () => { const promise = new Promise(r => {}); function Wait() { throw promise; @@ -337,7 +444,7 @@ describe('ReactDOMFizzServer', () => { expect(errors).toEqual(['foobar', 'foobar']); }); - it('Supports custom abort reasons with an Error', async () => { + it('supports custom abort reasons with an Error', async () => { const promise = new Promise(r => {}); function Wait() { throw promise; diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js index f5f85c953148a..f3a6aa50c3fdf 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js @@ -15,7 +15,7 @@ let React; let ReactDOMFizzServer; let Suspense; -describe('ReactDOMFizzServer', () => { +describe('ReactDOMFizzServerNode', () => { beforeEach(() => { jest.resetModules(); React = require('react'); @@ -166,7 +166,6 @@ describe('ReactDOMFizzServer', () => {
, - { onError(x) { reportedErrors.push(x); @@ -232,7 +231,6 @@ describe('ReactDOMFizzServer', () => { , - { onError(x) { reportedErrors.push(x); @@ -288,7 +286,6 @@ describe('ReactDOMFizzServer', () => { , - { onError(x) { errors.push(x.message); @@ -315,6 +312,49 @@ describe('ReactDOMFizzServer', () => { expect(isCompleteCalls).toBe(1); }); + it('should fail the shell if you abort before work has begun', async () => { + let isCompleteCalls = 0; + const errors = []; + const shellErrors = []; + const {writable, output, completed} = getTestWritable(); + const {pipe, abort} = ReactDOMFizzServer.renderToPipeableStream( +
+ Loading
}> + + + , + { + onError(x) { + errors.push(x.message); + }, + onShellError(x) { + shellErrors.push(x.message); + }, + onAllReady() { + isCompleteCalls++; + }, + }, + ); + pipe(writable); + + // Currently we delay work so if we abort, we abort the remaining CPU + // work as well. + + // Abort before running the timers that perform the work + const theReason = new Error('uh oh'); + abort(theReason); + + jest.runAllTimers(); + + await completed; + + expect(errors).toEqual(['uh oh']); + expect(shellErrors).toEqual(['uh oh']); + expect(output.error).toBe(theReason); + expect(output.result).toBe(''); + expect(isCompleteCalls).toBe(0); + }); + it('should be able to complete by abort when the fallback is also suspended', async () => { let isCompleteCalls = 0; const errors = []; @@ -327,7 +367,6 @@ describe('ReactDOMFizzServer', () => { , - { onError(x) { errors.push(x.message); diff --git a/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js b/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js index fc35fdb286798..acfe13e4ebbcc 100644 --- a/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js +++ b/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js @@ -96,11 +96,15 @@ function renderToReadableStream( ); if (options && options.signal) { const signal = options.signal; - const listener = () => { + if (signal.aborted) { abort(request, (signal: any).reason); - signal.removeEventListener('abort', listener); - }; - signal.addEventListener('abort', listener); + } else { + const listener = () => { + abort(request, (signal: any).reason); + signal.removeEventListener('abort', listener); + }; + signal.addEventListener('abort', listener); + } } startWork(request); }); diff --git a/packages/react-dom/src/server/ReactDOMLegacyServerImpl.js b/packages/react-dom/src/server/ReactDOMLegacyServerImpl.js index 27e41b42e43ae..504201f3c24ff 100644 --- a/packages/react-dom/src/server/ReactDOMLegacyServerImpl.js +++ b/packages/react-dom/src/server/ReactDOMLegacyServerImpl.js @@ -76,7 +76,7 @@ function renderToStringImpl( // That way we write only client-rendered boundaries from the start. abort(request, abortReason); startFlowing(request, destination); - if (didFatal) { + if (didFatal && fatalError !== abortReason) { throw fatalError; } diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 9e6551d938c0f..a8a3fe6932072 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -1530,7 +1530,7 @@ function abortTaskSoft(task: Task): void { finishedTask(request, boundary, segment); } -function abortTask(task: Task, request: Request, reason: mixed): void { +function abortTask(task: Task, request: Request, error: mixed): void { // This aborts the task and aborts the parent that it blocks, putting it into // client rendered mode. const boundary = task.blockedBoundary; @@ -1541,35 +1541,30 @@ function abortTask(task: Task, request: Request, reason: mixed): void { request.allPendingTasks--; // We didn't complete the root so we have nothing to show. We can close // the request; - if (request.status !== CLOSED) { - request.status = CLOSED; - if (request.destination !== null) { - close(request.destination); - } + if (request.status !== CLOSING && request.status !== CLOSED) { + logRecoverableError(request, error); + fatalError(request, error); } } else { boundary.pendingTasks--; if (!boundary.forceClientRender) { boundary.forceClientRender = true; - let error = - reason === undefined - ? new Error('The render was aborted by the server without a reason.') - : reason; boundary.errorDigest = request.onError(error); if (__DEV__) { const errorPrefix = 'The server did not finish this Suspense boundary: '; + let errorMessage; if (error && typeof error.message === 'string') { - error = errorPrefix + error.message; + errorMessage = errorPrefix + error.message; } else { // eslint-disable-next-line react-internal/safe-string-coercion - error = errorPrefix + String(error); + errorMessage = errorPrefix + String(error); } const previousTaskInDev = currentTaskInDEV; currentTaskInDEV = task; try { - captureBoundaryErrorDetailsDev(boundary, error); + captureBoundaryErrorDetailsDev(boundary, errorMessage); } finally { currentTaskInDEV = previousTaskInDev; } @@ -1582,7 +1577,7 @@ function abortTask(task: Task, request: Request, reason: mixed): void { // If this boundary was still pending then we haven't already cancelled its fallbacks. // We'll need to abort the fallbacks, which will also error that parent boundary. boundary.fallbackAbortableTasks.forEach(fallbackTask => - abortTask(fallbackTask, request, reason), + abortTask(fallbackTask, request, error), ); boundary.fallbackAbortableTasks.clear(); @@ -2178,8 +2173,14 @@ export function startFlowing(request: Request, destination: Destination): void { export function abort(request: Request, reason: mixed): void { try { const abortableTasks = request.abortableTasks; - abortableTasks.forEach(task => abortTask(task, request, reason)); - abortableTasks.clear(); + if (abortableTasks.size > 0) { + const error = + reason === undefined + ? new Error('The render was aborted by the server without a reason.') + : reason; + abortableTasks.forEach(task => abortTask(task, request, error)); + abortableTasks.clear(); + } if (request.destination !== null) { flushCompletedQueues(request, request.destination); }