Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fizz][Float] Do not write after closing the stream #27541

Merged
merged 1 commit into from
Oct 18, 2023
Merged

[Fizz][Float] Do not write after closing the stream #27541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<html>
<body>
<main>hello</main>
<Preload />
</body>
</html>
);
}
await act(() => {
renderToPipeableStream(<App />).pipe(writable);
});

expect(getVisibleChildren(document)).toEqual(
<html>
<head />
<body>
<main>hello</main>
</body>
</html>,
);
});

describe('error escaping', () => {
it('escapes error hash, message, and component stack values in directly flushed errors (html escaping)', async () => {
window.__outlet = {};
Expand Down
35 changes: 35 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServerBrowser-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@ global.ReadableStream =
global.TextEncoder = require('util').TextEncoder;

let React;
let ReactDOM;
let ReactDOMFizzServer;
let Suspense;

describe('ReactDOMFizzServerBrowser', () => {
beforeEach(() => {
jest.resetModules();
React = require('react');
ReactDOM = require('react-dom');
ReactDOMFizzServer = require('react-dom/server.browser');
Suspense = React.Suspense;
});
Expand Down Expand Up @@ -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 (
<html>
<body>
<main>hello</main>
<Preload />
</body>
</html>
);
}
const stream = await ReactDOMFizzServer.renderToReadableStream(<App />);
const result = await readResult(stream);

expect(result).toMatchInlineSnapshot(
`"<!DOCTYPE html><html><head></head><body><main>hello</main></body></html>"`,
);
});
});
15 changes: 13 additions & 2 deletions packages/react-server/src/ReactFizzServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}
});
}
}

Expand Down