[Fizz] Aborting early should not infinitely suspend

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;

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);

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);
   });

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;
   }
 

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);
     }