[Flight] do not emit error after abort (#30683)

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.

Using a sigil to throw after aborting during render isn't effective b/c
the user code itself could throw so insteead we just read the request
status. This is ok b/c we don't expect any tasks to still be pending
after the currently running task finishes.

However I found one instance where that wasn't true related to
serializing thenables which I've fixed so we may find other cases. If we
do, though it's almost certainly a bug in our task bookkeeping so we
should just fix it if it comes up.

I also updated `abort` to not set the status to ABORTING unless the
status was OPEN. we don't want to ever leave CLOSED or CLOSING status

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,
@@ -594,6 +592,8 @@ function serializeThenable(
         const digest = logRecoverableError(request, x, null);
         emitErrorChunk(request, newTask.id, digest, x);
       }
+      newTask.status = ERRORED;
+      request.abortableTasks.delete(newTask);
       return newTask.id;
     }
     default: {
@@ -650,10 +650,10 @@ function serializeThenable(
         logPostpone(request, postponeInstance.message, newTask);
         emitPostponeChunk(request, newTask.id, postponeInstance);
       } else {
-        newTask.status = ERRORED;
         const digest = logRecoverableError(request, reason, newTask);
         emitErrorChunk(request, newTask.id, digest, reason);
       }
+      newTask.status = ERRORED;
       request.abortableTasks.delete(newTask);
       enqueueFlush(request);
     },
@@ -1114,7 +1114,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 +1617,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 +2185,7 @@ function renderModel(
       }
     }
 
-    if (thrownValue === AbortSigil) {
+    if (request.status === ABORTING) {
       task.status = ABORTED;
       const errorId: number = (request.fatalError: any);
       if (wasReactNode) {
@@ -2357,7 +2359,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 +3693,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 +3912,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.