From 8bf2ed6187ae5de23ae9f12d717cc620be056fff Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Wed, 17 Oct 2018 11:24:38 -0700 Subject: [PATCH 1/2] BUG: ReactPartialRenderer / New Context polutes mutable global state The new context API stores the provided values on the shared context instance. When used in a synchronous context, this is not an issue. However when used in an concurrent context this can cause a "push provider" from one react render to have an effect on an unrelated concurrent react render. I've encountered this bug in production when using renderToNodeStream, which asks ReactPartialRenderer for bytes up to a high water mark before yielding. If two Node Streams are created and read from in parallel, the state of one can polute the other. I wrote a failing test to illustrate the conditions under which this happens. I'm also concerned that the experimental concurrent/async React rendering on the client could suffer from the same issue. --- ...eactDOMServerIntegrationNewContext-test.js | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index af9a6095e0a4..334ff0e2cbd2 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js @@ -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 => ( + +
+ {whoAmI => whoAmI} +
+
+ {whoAmI => whoAmI} +
+ + ); + + 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('
Amy
'); + expect(streamBob.read()).toBe('
Bob
'); + }); }); }); From d137b03ba4816b63086ddfbc7724741085a1a06b Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Wed, 17 Oct 2018 12:34:19 -0700 Subject: [PATCH 2/2] [RFC] FIX: Context polutes mutable global state Fixes #13874 This is just a strawman approach for a fix for #13874 that avoids mutating global context by giving each context instance a unique key it can use to store its value in a context object that's scoped to the current render. --- .../src/server/ReactPartialRenderer.js | 65 +++++++------------ packages/react/src/ReactContext.js | 8 +++ packages/shared/ReactTypes.js | 1 + 3 files changed, 31 insertions(+), 43 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index dc79c3fb2803..c0277ff6f263 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -692,9 +692,7 @@ class ReactDOMServerRenderer { previousWasTextNode: boolean; makeStaticMarkup: boolean; - contextIndex: number; - contextStack: Array>; - contextValueStack: Array; + contextValueStack: Array; contextProviderStack: ?Array>; // DEV-only constructor(children: mixed, makeStaticMarkup: boolean) { @@ -720,9 +718,7 @@ class ReactDOMServerRenderer { this.makeStaticMarkup = makeStaticMarkup; // Context (new API) - this.contextIndex = -1; - this.contextStack = []; - this.contextValueStack = []; + this.contextValueStack = [emptyObject]; if (__DEV__) { this.contextProviderStack = []; } @@ -732,53 +728,30 @@ class ReactDOMServerRenderer { * Note: We use just two stacks regardless of how many context providers you have. * Providers are always popped in the reverse order to how they were pushed * so we always know on the way down which provider you'll encounter next on the way up. - * On the way down, we push the current provider, and its context value *before* - * 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 + * On the way down, we push the current provider, and its context value onto + * the stacks, without mutating. */ pushProvider(provider: ReactProvider): void { - const index = ++this.contextIndex; const context: ReactContext = provider.type._context; - const previousValue = context._currentValue; - - // Remember which value to restore this context to on our way up. - this.contextStack[index] = context; - this.contextValueStack[index] = previousValue; + const prevValues = this.contextValueStack[ + this.contextValueStack.length - 1 + ]; + const nextValues = Object.assign({}, prevValues); + nextValues[context._uid] = provider.props.value; + this.contextValueStack.push(nextValues); if (__DEV__) { // Only used for push/pop mismatch warnings. - (this.contextProviderStack: any)[index] = provider; + (this.contextProviderStack: any).push(provider); } - - // Mutate the current value. - context._currentValue = provider.props.value; } popProvider(provider: ReactProvider): void { - const index = this.contextIndex; + this.contextValueStack.pop(); if (__DEV__) { - warningWithoutStack( - index > -1 && provider === (this.contextProviderStack: any)[index], - 'Unexpected pop.', - ); + const poppedProvider = (this.contextProviderStack: any).pop(); + warningWithoutStack(provider === poppedProvider, 'Unexpected pop.'); } - - const context: ReactContext = this.contextStack[index]; - const previousValue = this.contextValueStack[index]; - - // "Hide" these null assignments from Flow by using `any` - // because conceptually they are deletions--as long as we - // promise to never access values beyond `this.contextIndex`. - this.contextStack[index] = (null: any); - this.contextValueStack[index] = (null: any); - if (__DEV__) { - (this.contextProviderStack: any)[index] = (null: any); - } - this.contextIndex--; - - // Restore to the previous value we stored as we were walking down. - context._currentValue = previousValue; } read(bytes: number): string | null { @@ -988,8 +961,14 @@ class ReactDOMServerRenderer { case REACT_CONTEXT_TYPE: { const consumer: ReactConsumer = (nextChild: any); const nextProps: any = consumer.props; - const nextValue = consumer.type._currentValue; - + const values = this.contextValueStack[ + this.contextValueStack.length - 1 + ]; + // Use the value provided, or if not defined, fall back to the + // default value for this context. + const nextValue = values.hasOwnProperty(consumer.type._uid) + ? values[consumer.type._uid] + : consumer.type._currentValue; const nextChildren = toArray(nextProps.children(nextValue)); const frame: Frame = { type: nextChild, diff --git a/packages/react/src/ReactContext.js b/packages/react/src/ReactContext.js index 71d1b3860544..836b95a64691 100644 --- a/packages/react/src/ReactContext.js +++ b/packages/react/src/ReactContext.js @@ -14,6 +14,9 @@ import type {ReactContext} from 'shared/ReactTypes'; import warningWithoutStack from 'shared/warningWithoutStack'; import warning from 'shared/warning'; +const hasSymbol = typeof Symbol === 'function'; +let nextContextID = 1; + export function createContext( defaultValue: T, calculateChangedBits: ?(a: T, b: T) => number, @@ -34,6 +37,10 @@ export function createContext( const context: ReactContext = { $$typeof: REACT_CONTEXT_TYPE, + // A unique key to use when storing and retrieving values for this context + // instance. Uses an opaque Symbol when available, otherwise uses an + // incrementing numeric string. + _uid: hasSymbol ? Symbol() : '' + nextContextID++, _calculateChangedBits: calculateChangedBits, // As a workaround to support multiple concurrent renderers, we categorize // some renderers as primary and others as secondary. We only expect @@ -62,6 +69,7 @@ export function createContext( const Consumer = { $$typeof: REACT_CONTEXT_TYPE, _context: context, + _uid: context._uid, _calculateChangedBits: context._calculateChangedBits, }; // $FlowFixMe: Flow complains about not setting a value, which is intentional here diff --git a/packages/shared/ReactTypes.js b/packages/shared/ReactTypes.js index 080ade12b807..99a7c34f6e59 100644 --- a/packages/shared/ReactTypes.js +++ b/packages/shared/ReactTypes.js @@ -55,6 +55,7 @@ export type ReactContext = { Consumer: ReactContext, Provider: ReactProviderType, + _uid: Symbol | string, _calculateChangedBits: ((a: T, b: T) => number) | null, _currentValue: T,