From 3b1f90d92c6e0449cf59458868f4cc23c2bc12ef Mon Sep 17 00:00:00 2001
From: Josh Story
Date: Wed, 18 Oct 2023 09:08:43 -0700
Subject: [PATCH] [Fizz][Float] Do not write after closing the stream
Float methods can hang on to a reference to a Request after the request is closed due to AsyncLocalStorage. If a Float method is called at this point we do not want to attempt to flush anything. This change updates the closing logic to also call `stopFlowing` which will ensure that any checks against the destination properly reflect that we cannot do any writes. In addition it updates the enqueueFlush logic to existence check the destination inside the work function since it can change across the work scheduling gap if it is async.
---
.../src/__tests__/ReactDOMFizzServer-test.js | 40 +++++++++++++++++++
.../ReactDOMFizzServerBrowser-test.js | 35 ++++++++++++++++
packages/react-server/src/ReactFizzServer.js | 15 ++++++-
3 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
index 5d9c8bcaeccc4..2caf48662549d 100644
--- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
@@ -3650,6 +3650,46 @@ describe('ReactDOMFizzServer', () => {
);
});
+ // https://github.com/facebook/react/issues/27540
+ // This test is not actually asserting much because there is possibly a bug in the closeing logic for the
+ // Node implementation of Fizz. The close leads to an abort which sets the destination to null before the Float
+ // method has an opportunity to schedule a write. We should fix this probably and once we do this test will start
+ // to fail if the underyling issue of writing after stream completion isn't fixed
+ it('does not try to write to the stream after it has been closed', async () => {
+ async function preloadLate() {
+ await 1;
+ ReactDOM.preconnect('foo');
+ }
+
+ function Preload() {
+ preloadLate();
+ return null;
+ }
+
+ function App() {
+ return (
+
+
+ hello
+
+
+
+ );
+ }
+ await act(() => {
+ renderToPipeableStream().pipe(writable);
+ });
+
+ expect(getVisibleChildren(document)).toEqual(
+
+
+
+ hello
+
+ ,
+ );
+ });
+
describe('error escaping', () => {
it('escapes error hash, message, and component stack values in directly flushed errors (html escaping)', async () => {
window.__outlet = {};
diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js
index ab5f8509260df..7d391334f5a01 100644
--- a/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js
@@ -15,6 +15,7 @@ global.ReadableStream =
global.TextEncoder = require('util').TextEncoder;
let React;
+let ReactDOM;
let ReactDOMFizzServer;
let Suspense;
@@ -22,6 +23,7 @@ describe('ReactDOMFizzServerBrowser', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
+ ReactDOM = require('react-dom');
ReactDOMFizzServer = require('react-dom/server.browser');
Suspense = React.Suspense;
});
@@ -547,4 +549,37 @@ describe('ReactDOMFizzServerBrowser', () => {
// However, it does error the shell.
expect(caughtError.message).toEqual('testing postpone');
});
+
+ // https://github.com/facebook/react/issues/27540
+ // This test is not actually asserting much because in our test environment the Float method cannot find the request after
+ // an await and thus is a noop. If we fix our test environment to support AsyncLocalStorage we can assert that the
+ // stream does not write after closing.
+ it('does not try to write to the stream after it has been closed', async () => {
+ async function preloadLate() {
+ await 1;
+ ReactDOM.preconnect('foo');
+ }
+
+ function Preload() {
+ preloadLate();
+ return null;
+ }
+
+ function App() {
+ return (
+
+
+ hello
+
+
+
+ );
+ }
+ const stream = await ReactDOMFizzServer.renderToReadableStream();
+ const result = await readResult(stream);
+
+ expect(result).toMatchInlineSnapshot(
+ `"hello"`,
+ );
+ });
});
diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js
index 66c76ffaebfe7..9e550baa45461 100644
--- a/packages/react-server/src/ReactFizzServer.js
+++ b/packages/react-server/src/ReactFizzServer.js
@@ -3986,6 +3986,9 @@ function flushCompletedQueues(
}
// We're done.
close(destination);
+ // We need to stop flowing now because we do not want any async contexts which might call
+ // float methods to initiate any flushes after this point
+ stopFlowing(request);
} else {
completeWriting(destination);
flushBuffered(destination);
@@ -4011,9 +4014,17 @@ function enqueueFlush(request: Request): void {
// happen when we start flowing again
request.destination !== null
) {
- const destination = request.destination;
request.flushScheduled = true;
- scheduleWork(() => flushCompletedQueues(request, destination));
+ scheduleWork(() => {
+ // We need to existence check destination again here because it might go away
+ // in between the enqueueFlush call and the work execution
+ const destination = request.destination;
+ if (destination) {
+ flushCompletedQueues(request, destination);
+ } else {
+ request.flushScheduled = false;
+ }
+ });
}
}