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.

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

Fix react-dom/server context leaks when render stream destroyed early #14706

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -482,5 +482,98 @@ describe('ReactDOMServerIntegration', () => {
);
}
});

// Regression test for https://github.com/facebook/react/issues/14705
it('does not pollute later renders when stream destroyed', () => {
const LoggedInUser = React.createContext('default');

const AppWithUser = user => (
<LoggedInUser.Provider value={user}>
<header>
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
</header>
</LoggedInUser.Provider>
);

const stream = ReactDOMServer.renderToNodeStream(
AppWithUser('Amy'),
).setEncoding('utf8');

// This is an implementation detail because we test a memory leak
const {threadID} = stream.partialRenderer;

// Read enough to render Provider but not enough for it to be exited
stream._read(10);
expect(LoggedInUser[threadID]).toBe('Amy');

stream.destroy();

const AppWithUserNoProvider = () => (
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
);

const stream2 = ReactDOMServer.renderToNodeStream(
AppWithUserNoProvider(),
).setEncoding('utf8');

// Sanity check to ensure 2nd render has same threadID as 1st render,
// otherwise this test is not testing what it's meant to
expect(stream2.partialRenderer.threadID).toBe(threadID);

const markup = stream2.read(Infinity);

expect(markup).toBe('default');
});

// Regression test for https://github.com/facebook/react/issues/14705
it('frees context value reference when stream destroyed', () => {
const LoggedInUser = React.createContext('default');

const AppWithUser = user => (
<LoggedInUser.Provider value={user}>
<header>
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
</header>
</LoggedInUser.Provider>
);

const stream = ReactDOMServer.renderToNodeStream(
AppWithUser('Amy'),
).setEncoding('utf8');

// This is an implementation detail because we test a memory leak
const {threadID} = stream.partialRenderer;

// Read enough to render Provider but not enough for it to be exited
stream._read(10);
expect(LoggedInUser[threadID]).toBe('Amy');

stream.destroy();
expect(LoggedInUser[threadID]).toBe('default');
});

it('does not pollute sync renders after an error', () => {
const LoggedInUser = React.createContext('default');
const Crash = () => {
throw new Error('Boo!');
};
const AppWithUser = user => (
<LoggedInUser.Provider value={user}>
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
<Crash />
</LoggedInUser.Provider>
);

expect(() => {
ReactDOMServer.renderToString(AppWithUser('Casper'));
}).toThrow('Boo');

// Should not report a value from failed render
expect(
ReactDOMServer.renderToString(
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>,
),
).toBe('default');
});
});
});
10 changes: 10 additions & 0 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,7 @@ class ReactDOMServerRenderer {
destroy() {
if (!this.exhausted) {
this.exhausted = true;
this.clearProviders();
freeThreadID(this.threadID);
}
}
Expand Down Expand Up @@ -776,6 +777,15 @@ class ReactDOMServerRenderer {
context[this.threadID] = previousValue;
}

clearProviders(): void {
// Restore any remaining providers on the stack to previous values
for (let index = this.contextIndex; index >= 0; index--) {
const context: ReactContext<any> = this.contextStack[index];
const previousValue = this.contextValueStack[index];
context[this.threadID] = previousValue;
}
}

read(bytes: number): string | null {
if (this.exhausted) {
return null;
Expand Down