Skip to content

FIX: Context pollutes mutable global state (with "thread local storage") #13877

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

Closed
wants to merge 2 commits into from
Closed

FIX: Context pollutes mutable global state (with "thread local storage") #13877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8bf2ed6
BUG: ReactPartialRenderer / New Context polutes mutable global state
leebyron Oct 17, 2018
1e5100e
[RFC] FIX: Context pollutes mutable global state (with "thread local …
leebyron Oct 17, 2018
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
33 changes: 33 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,5 +249,38 @@ describe('ReactDOMServerIntegration', () => {
expect(e.querySelector('#language2').textContent).toBe('sanskrit');
expect(e.querySelector('#language3').textContent).toBe('french');
});

it('does not pollute parallel node streams', () => {
const LoggedInUser = React.createContext();

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

const streamAmy = ReactDOMServer.renderToNodeStream(
AppWithUser('Amy'),
).setEncoding('utf8');
const streamBob = ReactDOMServer.renderToNodeStream(
AppWithUser('Bob'),
).setEncoding('utf8');

// Testing by filling the buffer using internal _read() with a small
// number of bytes to avoid a test case which needs to align to a
// highWaterMark boundary of 2^14 chars.
streamAmy._read(20);
streamBob._read(20);
streamAmy._read(20);
streamBob._read(20);

expect(streamAmy.read()).toBe('<header>Amy</header><footer>Amy</footer>');
expect(streamBob.read()).toBe('<header>Bob</header><footer>Bob</footer>');
});
});
});
23 changes: 18 additions & 5 deletions packages/react-dom/src/server/ReactPartialRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,7 @@ class ReactDOMServerRenderer {
contextIndex: number;
contextStack: Array<ReactContext<any>>;
contextValueStack: Array<any>;
contextValueMap: Map<ReactContext<any>, any>;
contextProviderStack: ?Array<ReactProvider<any>>; // DEV-only

constructor(children: mixed, makeStaticMarkup: boolean) {
Expand Down Expand Up @@ -723,6 +724,7 @@ class ReactDOMServerRenderer {
this.contextIndex = -1;
this.contextStack = [];
this.contextValueStack = [];
this.contextValueMap = new Map();
if (__DEV__) {
this.contextProviderStack = [];
}
Expand All @@ -736,12 +738,21 @@ class ReactDOMServerRenderer {
* we mutated it, onto the stacks. Therefore, on the way up, we always know which
* provider needs to be "restored" to which value.
* https://github.com/facebook/react/pull/12985#issuecomment-396301248
*
* A context's _currentValue is not directly mutated to avoid issues with
* concurrent renderers. Instead a local contextValueMap stores the current
* value for each context instance.
*/

pushProvider<T>(provider: ReactProvider<T>): void {
const index = ++this.contextIndex;
const context: ReactContext<any> = provider.type._context;
const previousValue = context._currentValue;
// NOTE: in __DEV__ the _context and Consumer references are different,
// while in production they are the same. For consistent Map keys, always
// use context.Consumer.
const context: ReactContext<any> = provider.type._context.Consumer;
const previousValue = this.contextValueMap.has(context)
? this.contextValueMap.get(context)
: context._currentValue;

// Remember which value to restore this context to on our way up.
this.contextStack[index] = context;
Expand All @@ -752,7 +763,7 @@ class ReactDOMServerRenderer {
}

// Mutate the current value.
context._currentValue = provider.props.value;
this.contextValueMap.set(context, provider.props.value);
}

popProvider<T>(provider: ReactProvider<T>): void {
Expand All @@ -778,7 +789,7 @@ class ReactDOMServerRenderer {
this.contextIndex--;

// Restore to the previous value we stored as we were walking down.
context._currentValue = previousValue;
this.contextValueMap.set(context, previousValue);
}

read(bytes: number): string | null {
Expand Down Expand Up @@ -988,7 +999,9 @@ class ReactDOMServerRenderer {
case REACT_CONTEXT_TYPE: {
const consumer: ReactConsumer<any> = (nextChild: any);
const nextProps: any = consumer.props;
const nextValue = consumer.type._currentValue;
const nextValue = this.contextValueMap.has(consumer.type)
? this.contextValueMap.get(consumer.type)
: consumer.type._currentValue;

const nextChildren = toArray(nextProps.children(nextValue));
const frame: Frame = {
Expand Down