[Flight] do not emit error after abort

When synchronously aborting in a non-async Function Component if you throw after aborting the task would error rather than abort because React never observed the AbortSignal.

This change adds an additional check to see if the request is currently aborting. I left the sigil check in place too in case a task pings after the request is closed (though I'm pretty sure that can't actually happen)

I also renamed AbortSigil to AbortSymbol and changed it to a Symbol to make some of the duck typing more efficient.

Author: gnoff

packages/react-server-dom-webpack/src/__tests__/ReactFlightDOM-test.js

@@ -2554,4 +2554,100 @@ describe('ReactFlightDOM', () => {
       </div>,
     );
   });
+
+  it('can error synchronously after aborting in a synchronous Component', async () => {
+    const rejectError = new Error('bam!');
+    const rejectedPromise = Promise.reject(rejectError);
+    rejectedPromise.catch(() => {});
+    rejectedPromise.status = 'rejected';
+    rejectedPromise.reason = rejectError;
+
+    const resolvedValue = <p>hello world</p>;
+    const resolvedPromise = Promise.resolve(resolvedValue);
+    resolvedPromise.status = 'fulfilled';
+    resolvedPromise.value = resolvedValue;
+
+    function App() {
+      return (
+        <div>
+          <Suspense fallback={<p>loading...</p>}>
+            <ComponentThatAborts />
+          </Suspense>
+          <Suspense fallback={<p>loading too...</p>}>
+            {rejectedPromise}
+          </Suspense>
+          <Suspense fallback={<p>loading three...</p>}>
+            {resolvedPromise}
+          </Suspense>
+        </div>
+      );
+    }
+
+    const abortRef = {current: null};
+
+    // This test is specifically asserting that this works with Sync Server Component
+    function ComponentThatAborts() {
+      abortRef.current();
+      throw new Error('boom');
+    }
+
+    const {writable: flightWritable, readable: flightReadable} =
+      getTestStream();
+
+    await serverAct(() => {
+      const {pipe, abort} = ReactServerDOMServer.renderToPipeableStream(
+        <App />,
+        webpackMap,
+        {
+          onError(e) {
+            console.error(e);
+          },
+        },
+      );
+      abortRef.current = abort;
+      pipe(flightWritable);
+    });
+
+    assertConsoleErrorDev([
+      'The render was aborted by the server without a reason.',
+      'bam!',
+    ]);
+
+    const response =
+      ReactServerDOMClient.createFromReadableStream(flightReadable);
+
+    const {writable: fizzWritable, readable: fizzReadable} = getTestStream();
+
+    function ClientApp() {
+      return use(response);
+    }
+
+    const shellErrors = [];
+    await serverAct(async () => {
+      ReactDOMFizzServer.renderToPipeableStream(
+        React.createElement(ClientApp),
+        {
+          onShellError(error) {
+            shellErrors.push(error.message);
+          },
+        },
+      ).pipe(fizzWritable);
+    });
+    assertConsoleErrorDev([
+      'The render was aborted by the server without a reason.',
+      'bam!',
+    ]);
+
+    expect(shellErrors).toEqual([]);
+
+    const container = document.createElement('div');
+    await readInto(container, fizzReadable);
+    expect(getMeaningfulChildren(container)).toEqual(
+      <div>
+        <p>loading...</p>
+        <p>loading too...</p>
+        <p>hello world</p>
+      </div>,
+    );
+  });
 });

packages/react-server/src/ReactFlightServer.js

@@ -382,8 +382,6 @@ export type Request = {
   didWarnForKey: null | WeakSet<ReactComponentInfo>,
 };
 
-const AbortSigil = {};
-
 const {
   TaintRegistryObjects,
   TaintRegistryValues,
@@ -587,13 +585,16 @@ function serializeThenable(
         x !== null &&
         (x: any).$$typeof === REACT_POSTPONE_TYPE
       ) {
+        newTask.status = COMPLETED;
         const postponeInstance: Postpone = (x: any);
         logPostpone(request, postponeInstance.message, newTask);
         emitPostponeChunk(request, newTask.id, postponeInstance);
       } else {
+        newTask.status = ERRORED;
         const digest = logRecoverableError(request, x, null);
         emitErrorChunk(request, newTask.id, digest, x);
       }
+      request.abortableTasks.delete(newTask);
       return newTask.id;
     }
     default: {
@@ -1114,7 +1115,8 @@ function renderFunctionComponent<Props>(
     // If we aborted during rendering we should interrupt the render but
     // we don't need to provide an error because the renderer will encode
     // the abort error as the reason.
-    throw AbortSigil;
+    // eslint-disable-next-line no-throw-literal
+    throw null;
   }
 
   if (
@@ -1616,7 +1618,8 @@ function renderElement(
           // lazy initializers are user code and could abort during render
           // we don't wan to return any value resolved from the lazy initializer
           // if it aborts so we interrupt rendering here
-          throw AbortSigil;
+          // eslint-disable-next-line no-throw-literal
+          throw null;
         }
         return renderElement(
           request,
@@ -2183,7 +2186,7 @@ function renderModel(
       }
     }
 
-    if (thrownValue === AbortSigil) {
+    if (request.status === ABORTING) {
       task.status = ABORTED;
       const errorId: number = (request.fatalError: any);
       if (wasReactNode) {
@@ -2357,7 +2360,8 @@ function renderModelDestructive(
           // lazy initializers are user code and could abort during render
           // we don't wan to return any value resolved from the lazy initializer
           // if it aborts so we interrupt rendering here
-          throw AbortSigil;
+          // eslint-disable-next-line no-throw-literal
+          throw null;
         }
         if (__DEV__) {
           const debugInfo: ?ReactDebugInfo = lazy._debugInfo;
@@ -3690,7 +3694,7 @@ function retryTask(request: Request, task: Task): void {
       }
     }
 
-    if (x === AbortSigil) {
+    if (request.status === ABORTING) {
       request.abortableTasks.delete(task);
       task.status = ABORTED;
       const errorId: number = (request.fatalError: any);
@@ -3909,7 +3913,9 @@ export function stopFlowing(request: Request): void {
 // This is called to early terminate a request. It creates an error at all pending tasks.
 export function abort(request: Request, reason: mixed): void {
   try {
-    request.status = ABORTING;
+    if (request.status === OPEN) {
+      request.status = ABORTING;
+    }
     const abortableTasks = request.abortableTasks;
     // We have tasks to abort. We'll emit one error row and then emit a reference
     // to that row from every row that's still remaining.