Rename Node SSR Callbacks to onShellReady/onAllReady and Other Fixes (#24030)

* I forgot to call onFatalError

I can't figure out how to write a test for this because it only happens
when there is a bug in React itself which would then be fixed if we found
it.

We're also covered by the protection of ReadableStream which doesn't leak
other errors to us.

* Abort requests if the reader cancels

No need to continue computing at this point.

* Abort requests if node streams get destroyed

This is if the downstream cancels is for example.

* Rename Node APIs for Parity with allReady

The "Complete" terminology is a little misleading because not everything
has been written yet. It's just "Ready" to be written now.

onShellReady
onShellError
onAllReady

* 'close' should be enough

fixtures/ssr/server/render.js

@@ -22,13 +22,13 @@ export default function render(url, res) {
   let didError = false;
   const {pipe, abort} = renderToPipeableStream(<App assets={assets} />, {
     bootstrapScripts: [assets['main.js']],
-    onCompleteShell() {
+    onShellReady() {
       // If something errored before we started streaming, we set the error code appropriately.
       res.statusCode = didError ? 500 : 200;
       res.setHeader('Content-type', 'text/html');
       pipe(res);
     },
-    onErrorShell(x) {
+    onShellError(x) {
       // Something errored before we could complete the shell so we emit an alternative shell.
       res.statusCode = 500;
       res.send('<!doctype><p>Error</p>');

fixtures/ssr2/server/render.js

@@ -49,7 +49,7 @@ module.exports = function render(url, res) {
         res.setHeader('Content-type', 'text/html');
         pipe(res);
       },
-      onErrorShell(x) {
+      onShellError(x) {
         // Something errored before we could complete the shell so we emit an alternative shell.
         res.statusCode = 500;
         res.send('<!doctype><p>Error</p>');

packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js

@@ -914,7 +914,7 @@ describe('ReactDOMFizzServer', () => {
         </Suspense>,
         {
           identifierPrefix: 'A_',
-          onCompleteShell() {
+          onShellReady() {
             writableA.write('<div id="container-A">');
             pipe(writableA);
             writableA.write('</div>');
@@ -933,7 +933,7 @@ describe('ReactDOMFizzServer', () => {
         </Suspense>,
         {
           identifierPrefix: 'B_',
-          onCompleteShell() {
+          onShellReady() {
             writableB.write('<div id="container-B">');
             pipe(writableB);
             writableB.write('</div>');
@@ -1168,7 +1168,7 @@ describe('ReactDOMFizzServer', () => {
 
         {
           namespaceURI: 'http://www.w3.org/2000/svg',
-          onCompleteShell() {
+          onShellReady() {
             writable.write('<svg>');
             pipe(writable);
             writable.write('</svg>');

packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js

@@ -209,4 +209,43 @@ describe('ReactDOMFizzServer', () => {
     const result = await readResult(stream);
     expect(result).toContain('Loading');
   });
+
+  // @gate experimental
+  it('should not continue rendering after the reader cancels', async () => {
+    let hasLoaded = false;
+    let resolve;
+    let isComplete = false;
+    let rendered = false;
+    const promise = new Promise(r => (resolve = r));
+    function Wait() {
+      if (!hasLoaded) {
+        throw promise;
+      }
+      rendered = true;
+      return 'Done';
+    }
+    const stream = await ReactDOMFizzServer.renderToReadableStream(
+      <div>
+        <Suspense fallback={<div>Loading</div>}>
+          <Wait /> />
+        </Suspense>
+      </div>,
+    );
+
+    stream.allReady.then(() => (isComplete = true));
+
+    expect(rendered).toBe(false);
+    expect(isComplete).toBe(false);
+
+    const reader = stream.getReader();
+    reader.cancel();
+
+    hasLoaded = true;
+    resolve();
+
+    await jest.runAllTimers();
+
+    expect(rendered).toBe(false);
+    expect(isComplete).toBe(true);
+  });
 });

packages/react-dom/src/__tests__/ReactDOMFizzServerNode-test.js

@@ -138,7 +138,7 @@ describe('ReactDOMFizzServer', () => {
       </div>,
 
       {
-        onCompleteAll() {
+        onAllReady() {
           isCompleteCalls++;
         },
       },
@@ -179,7 +179,7 @@ describe('ReactDOMFizzServer', () => {
         onError(x) {
           reportedErrors.push(x);
         },
-        onErrorShell(x) {
+        onShellError(x) {
           reportedShellErrors.push(x);
         },
       },
@@ -213,7 +213,7 @@ describe('ReactDOMFizzServer', () => {
         onError(x) {
           reportedErrors.push(x);
         },
-        onErrorShell(x) {
+        onShellError(x) {
           reportedShellErrors.push(x);
         },
       },
@@ -244,7 +244,7 @@ describe('ReactDOMFizzServer', () => {
         onError(x) {
           reportedErrors.push(x);
         },
-        onErrorShell(x) {
+        onShellError(x) {
           reportedShellErrors.push(x);
         },
       },
@@ -298,7 +298,7 @@ describe('ReactDOMFizzServer', () => {
       </div>,
 
       {
-        onCompleteAll() {
+        onAllReady() {
           isCompleteCalls++;
         },
       },
@@ -333,7 +333,7 @@ describe('ReactDOMFizzServer', () => {
       </div>,
 
       {
-        onCompleteAll() {
+        onAllReady() {
           isCompleteCalls++;
         },
       },
@@ -537,4 +537,49 @@ describe('ReactDOMFizzServer', () => {
     expect(output.result).not.toContain('context never found');
     expect(output.result).toContain('OK');
   });
+
+  // @gate experimental
+  it('should not continue rendering after the writable ends unexpectedly', async () => {
+    let hasLoaded = false;
+    let resolve;
+    let isComplete = false;
+    let rendered = false;
+    const promise = new Promise(r => (resolve = r));
+    function Wait() {
+      if (!hasLoaded) {
+        throw promise;
+      }
+      rendered = true;
+      return 'Done';
+    }
+    const {writable, completed} = getTestWritable();
+    const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
+      <div>
+        <Suspense fallback={<div>Loading</div>}>
+          <Wait />
+        </Suspense>
+      </div>,
+      {
+        onAllReady() {
+          isComplete = true;
+        },
+      },
+    );
+    pipe(writable);
+
+    expect(rendered).toBe(false);
+    expect(isComplete).toBe(false);
+
+    writable.end();
+
+    await jest.runAllTimers();
+
+    hasLoaded = true;
+    resolve();
+
+    await completed;
+
+    expect(rendered).toBe(false);
+    expect(isComplete).toBe(true);
+  });
 });

packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js

@@ -155,7 +155,7 @@ module.exports = function(initModules) {
         new Promise((resolve, reject) => {
           const writable = new DrainWritable();
           const s = ReactDOMServer.renderToPipeableStream(reactElement, {
-            onErrorShell(e) {
+            onShellError(e) {
               reject(e);
             },
           });

packages/react-dom/src/server/ReactDOMFizzServerBrowser.js

@@ -46,25 +46,27 @@ function renderToReadableStream(
 ): Promise<ReactDOMServerReadableStream> {
   return new Promise((resolve, reject) => {
     let onFatalError;
-    let onCompleteAll;
+    let onAllReady;
     const allReady = new Promise((res, rej) => {
-      onCompleteAll = res;
+      onAllReady = res;
       onFatalError = rej;
     });
 
-    function onCompleteShell() {
+    function onShellReady() {
       const stream: ReactDOMServerReadableStream = (new ReadableStream({
         type: 'bytes',
         pull(controller) {
           startFlowing(request, controller);
         },
-        cancel(reason) {},
+        cancel(reason) {
+          abort(request);
+        },
       }): any);
       // TODO: Move to sub-classing ReadableStream.
       stream.allReady = allReady;
       resolve(stream);
     }
-    function onErrorShell(error: mixed) {
+    function onShellError(error: mixed) {
       reject(error);
     }
     const request = createRequest(
@@ -79,9 +81,9 @@ function renderToReadableStream(
       createRootFormatContext(options ? options.namespaceURI : undefined),
       options ? options.progressiveChunkSize : undefined,
       options ? options.onError : undefined,
-      onCompleteAll,
+      onAllReady,
-      onCompleteShell,
+      onShellReady,
-      onErrorShell,
+      onShellError,
       onFatalError,
     );
     if (options && options.signal) {

packages/react-dom/src/server/ReactDOMFizzServerNode.js

@@ -28,6 +28,10 @@ function createDrainHandler(destination, request) {
   return () => startFlowing(request, destination);
 }
 
+function createAbortHandler(request) {
+  return () => abort(request);
+}
+
 type Options = {|
   identifierPrefix?: string,
   namespaceURI?: string,
@@ -36,9 +40,9 @@ type Options = {|
   bootstrapScripts?: Array<string>,
   bootstrapModules?: Array<string>,
   progressiveChunkSize?: number,
-  onCompleteShell?: () => void,
+  onShellReady?: () => void,
-  onErrorShell?: () => void,
+  onShellError?: () => void,
-  onCompleteAll?: () => void,
+  onAllReady?: () => void,
   onError?: (error: mixed) => void,
 |};
 
@@ -62,9 +66,9 @@ function createRequestImpl(children: ReactNodeList, options: void | Options) {
     createRootFormatContext(options ? options.namespaceURI : undefined),
     options ? options.progressiveChunkSize : undefined,
     options ? options.onError : undefined,
-    options ? options.onCompleteAll : undefined,
+    options ? options.onAllReady : undefined,
-    options ? options.onCompleteShell : undefined,
+    options ? options.onShellReady : undefined,
-    options ? options.onErrorShell : undefined,
+    options ? options.onShellError : undefined,
     undefined,
   );
 }
@@ -86,6 +90,7 @@ function renderToPipeableStream(
       hasStartedFlowing = true;
       startFlowing(request, destination);
       destination.on('drain', createDrainHandler(destination, request));
+      destination.on('close', createAbortHandler(request));
       return destination;
     },
     abort() {

packages/react-dom/src/server/ReactDOMLegacyServerBrowser.js

@@ -53,7 +53,7 @@ function renderToStringImpl(
   };
 
   let readyToStream = false;
-  function onCompleteShell() {
+  function onShellReady() {
     readyToStream = true;
   }
   const request = createRequest(
@@ -66,7 +66,7 @@ function renderToStringImpl(
     Infinity,
     onError,
     undefined,
-    onCompleteShell,
+    onShellReady,
     undefined,
     undefined,
   );

packages/react-dom/src/server/ReactDOMLegacyServerNode.js

@@ -68,7 +68,7 @@ function renderToNodeStreamImpl(
   options: void | ServerOptions,
   generateStaticMarkup: boolean,
 ): Readable {
-  function onCompleteAll() {
+  function onAllReady() {
     // We wait until everything has loaded before starting to write.
     // That way we only end up with fully resolved HTML even if we suspend.
     destination.startedFlowing = true;
@@ -81,7 +81,7 @@ function renderToNodeStreamImpl(
     createRootFormatContext(),
     Infinity,
     onError,
-    onCompleteAll,
+    onAllReady,
     undefined,
     undefined,
   );

packages/react-noop-renderer/src/ReactNoopServer.js

@@ -251,8 +251,8 @@ const ReactNoopServer = ReactFizzServer({
 
 type Options = {
   progressiveChunkSize?: number,
-  onCompleteShell?: () => void,
+  onShellReady?: () => void,
-  onCompleteAll?: () => void,
+  onAllReady?: () => void,
   onError?: (error: mixed) => void,
 };
 
@@ -272,8 +272,8 @@ function render(children: React$Element<any>, options?: Options): Destination {
     null,
     options ? options.progressiveChunkSize : undefined,
     options ? options.onError : undefined,
-    options ? options.onCompleteAll : undefined,
+    options ? options.onAllReady : undefined,
-    options ? options.onCompleteShell : undefined,
+    options ? options.onShellReady : undefined,
   );
   ReactNoopServer.startWork(request);
   ReactNoopServer.startFlowing(request, destination);

packages/react-server/src/ReactFizzServer.js

@@ -194,16 +194,16 @@ export opaque type Request = {
   partialBoundaries: Array<SuspenseBoundary>, // Partially completed boundaries that can flush its segments early.
   // onError is called when an error happens anywhere in the tree. It might recover.
   onError: (error: mixed) => void,
-  // onCompleteAll is called when all pending task is done but it may not have flushed yet.
+  // onAllReady is called when all pending task is done but it may not have flushed yet.
   // This is a good time to start writing if you want only HTML and no intermediate steps.
-  onCompleteAll: () => void,
+  onAllReady: () => void,
-  // onCompleteShell is called when there is at least a root fallback ready to show.
+  // onShellReady is called when there is at least a root fallback ready to show.
   // Typically you don't need this callback because it's best practice to always have a
   // root fallback ready so there's no need to wait.
-  onCompleteShell: () => void,
+  onShellReady: () => void,
-  // onErrorShell is called when the shell didn't complete. That means you probably want to
+  // onShellError is called when the shell didn't complete. That means you probably want to
   // emit a different response to the stream instead.
-  onErrorShell: (error: mixed) => void,
+  onShellError: (error: mixed) => void,
   onFatalError: (error: mixed) => void,
 };
 
@@ -236,9 +236,9 @@ export function createRequest(
   rootFormatContext: FormatContext,
   progressiveChunkSize: void | number,
   onError: void | ((error: mixed) => void),
-  onCompleteAll: void | (() => void),
+  onAllReady: void | (() => void),
-  onCompleteShell: void | (() => void),
+  onShellReady: void | (() => void),
-  onErrorShell: void | ((error: mixed) => void),
+  onShellError: void | ((error: mixed) => void),
   onFatalError: void | ((error: mixed) => void),
 ): Request {
   const pingedTasks = [];
@@ -262,9 +262,9 @@ export function createRequest(
     completedBoundaries: [],
     partialBoundaries: [],
     onError: onError === undefined ? defaultErrorHandler : onError,
-    onCompleteAll: onCompleteAll === undefined ? noop : onCompleteAll,
+    onAllReady: onAllReady === undefined ? noop : onAllReady,
-    onCompleteShell: onCompleteShell === undefined ? noop : onCompleteShell,
+    onShellReady: onShellReady === undefined ? noop : onShellReady,
-    onErrorShell: onErrorShell === undefined ? noop : onErrorShell,
+    onShellError: onShellError === undefined ? noop : onShellError,
     onFatalError: onFatalError === undefined ? noop : onFatalError,
   };
   // This segment represents the root fallback.
@@ -422,8 +422,10 @@ function fatalError(request: Request, error: mixed): void {
   // This is called outside error handling code such as if the root errors outside
   // a suspense boundary or if the root suspense boundary's fallback errors.
   // It's also called if React itself or its host configs errors.
-  const onErrorShell = request.onErrorShell;
+  const onShellError = request.onShellError;
-  onErrorShell(error);
+  onShellError(error);
+  const onFatalError = request.onFatalError;
+  onFatalError(error);
   if (request.destination !== null) {
     request.status = CLOSED;
     closeWithError(request.destination, error);
@@ -1371,8 +1373,8 @@ function erroredTask(
 
   request.allPendingTasks--;
   if (request.allPendingTasks === 0) {
-    const onCompleteAll = request.onCompleteAll;
+    const onAllReady = request.onAllReady;
-    onCompleteAll();
+    onAllReady();
   }
 }
 
@@ -1422,8 +1424,8 @@ function abortTask(task: Task): void {
 
     request.allPendingTasks--;
     if (request.allPendingTasks === 0) {
-      const onCompleteAll = request.onCompleteAll;
+      const onAllReady = request.onAllReady;
-      onCompleteAll();
+      onAllReady();
     }
   }
 }
@@ -1446,9 +1448,9 @@ function finishedTask(
     request.pendingRootTasks--;
     if (request.pendingRootTasks === 0) {
       // We have completed the shell so the shell can't error anymore.
-      request.onErrorShell = noop;
+      request.onShellError = noop;
-      const onCompleteShell = request.onCompleteShell;
+      const onShellReady = request.onShellReady;
-      onCompleteShell();
+      onShellReady();
     }
   } else {
     boundary.pendingTasks--;
@@ -1499,8 +1501,8 @@ function finishedTask(
   if (request.allPendingTasks === 0) {
     // This needs to be called at the very end so that we can synchronously write the result
     // in the callback if needed.
-    const onCompleteAll = request.onCompleteAll;
+    const onAllReady = request.onAllReady;
-    onCompleteAll();
+    onAllReady();
   }
 }