Aborting early should not infinitely suspend (#24751)

sebmarkbage · web-flow · commit 0f0aca3ab353 · 2022-06-18T11:01:58.000-04:00
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.
diff --git 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(
+      <div>
+        <InfiniteSuspend />
+      </div>,
+      {
+        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 (
+        <Suspense fallback={<div>Loading</div>}>
+          <InfiniteSuspend />
+        </Suspense>
+      );
+    }
+    const streamPromise = ReactDOMFizzServer.renderToReadableStream(
+      <div>
+        <App />
+      </div>,
+      {
+        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(
+      <div>
+        <Suspense fallback={<div>Loading</div>}>
+          <InfiniteSuspend />
+        </Suspense>
+      </div>,
+      {
+        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(
       <div>
         <Suspense fallback={<div>Loading</div>}>
-          <Wait /> />
+          <Wait />
         </Suspense>
       </div>,
       {
@@ -296,7 +403,7 @@ describe('ReactDOMFizzServer', () => {
     expect(result).toMatchInlineSnapshot(`"<div>${str2049}</div>"`);
   });
 
-  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
@@ -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', () => {
       <div>
         <Throw />
       </div>,
-
       {
         onError(x) {
           reportedErrors.push(x);
@@ -232,7 +231,6 @@ describe('ReactDOMFizzServer', () => {
           <Throw />
         </Suspense>
       </div>,
-
       {
         onError(x) {
           reportedErrors.push(x);
@@ -288,7 +286,6 @@ describe('ReactDOMFizzServer', () => {
           <InfiniteSuspend />
         </Suspense>
       </div>,
-
       {
         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(
+      <div>
+        <Suspense fallback={<div>Loading</div>}>
+          <InfiniteSuspend />
+        </Suspense>
+      </div>,
+      {
+        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', () => {
           </Suspense>
         </Suspense>
       </div>,
-
       {
         onError(x) {
           errors.push(x.message);
diff --git 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
@@ -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
@@ -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);
     }
