From b55654ab937e1f22188acd99173d5eeeb3708094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 9 Nov 2018 15:38:20 -0800 Subject: [PATCH] Use unique thread ID for each partial render to access Context (#14182) * 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. * Use unique thread ID for each partial render to access Context This first adds an allocator that keeps track of a unique ThreadID index for each currently executing partial renderer. IDs are not just growing but are reused as streams are destroyed. This ensures that IDs are kept nice and compact. This lets us use an "array" for each Context object to store the current values. The look up for these are fast because they're just looking up an offset in a tightly packed "array". I don't use an actual Array object to store the values. Instead, I rely on that VMs (notably V8) treat storage of numeric index property access as a separate "elements" allocation. This lets us avoid an extra indirection. However, we must ensure that these arrays are not holey to preserve this feature. To do that I store the _threadCount on each context (effectively it takes the place of the .length property on an array). This lets us first validate that the context has enough slots before we access the slot. If not, we fill in the slots with the default value. --- ...eactDOMServerIntegrationNewContext-test.js | 91 +++++++++++++++ .../src/server/ReactDOMNodeStreamRenderer.js | 6 +- .../src/server/ReactDOMStringRenderer.js | 16 ++- .../src/server/ReactPartialRenderer.js | 102 ++++++----------- .../src/server/ReactPartialRendererContext.js | 104 ++++++++++++++++++ .../src/server/ReactPartialRendererHooks.js | 18 ++- .../src/server/ReactThreadIDAllocator.js | 58 ++++++++++ packages/react/src/ReactContext.js | 11 ++ packages/shared/ReactTypes.js | 1 + scripts/rollup/validate/eslintrc.cjs.js | 1 + scripts/rollup/validate/eslintrc.fb.js | 1 + scripts/rollup/validate/eslintrc.umd.js | 1 + 12 files changed, 336 insertions(+), 74 deletions(-) create mode 100644 packages/react-dom/src/server/ReactPartialRendererContext.js create mode 100644 packages/react-dom/src/server/ReactThreadIDAllocator.js diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js index ee2afb4fae371..5f941da8f9a9f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js @@ -348,5 +348,96 @@ describe('ReactDOMServerIntegration', () => { await render(, 1); }, ); + + 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
Amy
'); + expect(streamBob.read()).toBe('
Bob
Bob
'); + }); + + it('does not pollute parallel node streams when many are used', () => { + const CurrentIndex = React.createContext(); + + const NthRender = index => ( + +
+ {idx => idx} +
+
+ {idx => idx} +
+ + ); + + let streams = []; + + // Test with more than 32 streams to test that growing the thread count + // works properly. + let streamCount = 34; + + for (let i = 0; i < streamCount; i++) { + streams[i] = ReactDOMServer.renderToNodeStream( + NthRender(i % 2 === 0 ? 'Expected to be recreated' : i), + ).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. + for (let i = 0; i < streamCount; i++) { + streams[i]._read(20); + } + + // Early destroy every other stream + for (let i = 0; i < streamCount; i += 2) { + streams[i].destroy(); + } + + // Recreate those same streams. + for (let i = 0; i < streamCount; i += 2) { + streams[i] = ReactDOMServer.renderToNodeStream( + NthRender(i), + ).setEncoding('utf8'); + } + + // Read a bit from all streams again. + for (let i = 0; i < streamCount; i++) { + streams[i]._read(20); + } + + // Assert that all stream rendered the expected output. + for (let i = 0; i < streamCount; i++) { + expect(streams[i].read()).toBe( + '
' + i + '
' + i + '
', + ); + } + }); }); }); diff --git a/packages/react-dom/src/server/ReactDOMNodeStreamRenderer.js b/packages/react-dom/src/server/ReactDOMNodeStreamRenderer.js index 162b8e9d76acb..7478d86cc3f18 100644 --- a/packages/react-dom/src/server/ReactDOMNodeStreamRenderer.js +++ b/packages/react-dom/src/server/ReactDOMNodeStreamRenderer.js @@ -18,11 +18,15 @@ class ReactMarkupReadableStream extends Readable { this.partialRenderer = new ReactPartialRenderer(element, makeStaticMarkup); } + _destroy() { + this.partialRenderer.destroy(); + } + _read(size) { try { this.push(this.partialRenderer.read(size)); } catch (err) { - this.emit('error', err); + this.destroy(err); } } } diff --git a/packages/react-dom/src/server/ReactDOMStringRenderer.js b/packages/react-dom/src/server/ReactDOMStringRenderer.js index 50b20cb039359..1afc65acd6d5c 100644 --- a/packages/react-dom/src/server/ReactDOMStringRenderer.js +++ b/packages/react-dom/src/server/ReactDOMStringRenderer.js @@ -14,8 +14,12 @@ import ReactPartialRenderer from './ReactPartialRenderer'; */ export function renderToString(element) { const renderer = new ReactPartialRenderer(element, false); - const markup = renderer.read(Infinity); - return markup; + try { + const markup = renderer.read(Infinity); + return markup; + } finally { + renderer.destroy(); + } } /** @@ -25,6 +29,10 @@ export function renderToString(element) { */ export function renderToStaticMarkup(element) { const renderer = new ReactPartialRenderer(element, true); - const markup = renderer.read(Infinity); - return markup; + try { + const markup = renderer.read(Infinity); + return markup; + } finally { + renderer.destroy(); + } } diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index c5f124e7c4a9a..dfc1aba8c88ce 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -7,6 +7,7 @@ * @flow */ +import type {ThreadID} from './ReactThreadIDAllocator'; import type {ReactElement} from 'shared/ReactElementType'; import type {ReactProvider, ReactContext} from 'shared/ReactTypes'; @@ -16,7 +17,6 @@ import getComponentName from 'shared/getComponentName'; import lowPriorityWarning from 'shared/lowPriorityWarning'; import warning from 'shared/warning'; import warningWithoutStack from 'shared/warningWithoutStack'; -import checkPropTypes from 'prop-types/checkPropTypes'; import describeComponentFrame from 'shared/describeComponentFrame'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import { @@ -39,6 +39,12 @@ import { REACT_MEMO_TYPE, } from 'shared/ReactSymbols'; +import { + emptyObject, + processContext, + validateContextBounds, +} from './ReactPartialRendererContext'; +import {allocThreadID, freeThreadID} from './ReactThreadIDAllocator'; import { createMarkupForCustomAttribute, createMarkupForProperty, @@ -50,6 +56,8 @@ import { finishHooks, Dispatcher, DispatcherWithoutHooks, + currentThreadID, + setCurrentThreadID, } from './ReactPartialRendererHooks'; import { Namespaces, @@ -176,7 +184,6 @@ const didWarnAboutBadClass = {}; const didWarnAboutDeprecatedWillMount = {}; const didWarnAboutUndefinedDerivedState = {}; const didWarnAboutUninitializedState = {}; -const didWarnAboutInvalidateContextType = {}; const valuePropNames = ['value', 'defaultValue']; const newlineEatingTags = { listing: true, @@ -324,65 +331,6 @@ function flattenOptionChildren(children: mixed): ?string { return content; } -const emptyObject = {}; -if (__DEV__) { - Object.freeze(emptyObject); -} - -function maskContext(type, context) { - const contextTypes = type.contextTypes; - if (!contextTypes) { - return emptyObject; - } - const maskedContext = {}; - for (const contextName in contextTypes) { - maskedContext[contextName] = context[contextName]; - } - return maskedContext; -} - -function checkContextTypes(typeSpecs, values, location: string) { - if (__DEV__) { - checkPropTypes( - typeSpecs, - values, - location, - 'Component', - getCurrentServerStackImpl, - ); - } -} - -function processContext(type, context) { - const contextType = type.contextType; - if (typeof contextType === 'object' && contextType !== null) { - if (__DEV__) { - if (contextType.$$typeof !== REACT_CONTEXT_TYPE) { - let name = getComponentName(type) || 'Component'; - if (!didWarnAboutInvalidateContextType[name]) { - didWarnAboutInvalidateContextType[type] = true; - warningWithoutStack( - false, - '%s defines an invalid contextType. ' + - 'contextType should point to the Context object returned by React.createContext(). ' + - 'Did you accidentally pass the Context.Provider instead?', - name, - ); - } - } - } - return contextType._currentValue; - } else { - const maskedContext = maskContext(type, context); - if (__DEV__) { - if (type.contextTypes) { - checkContextTypes(type.contextTypes, maskedContext, 'context'); - } - } - return maskedContext; - } -} - const hasOwnProperty = Object.prototype.hasOwnProperty; const STYLE = 'style'; const RESERVED_PROPS = { @@ -453,6 +401,7 @@ function validateRenderResult(child, type) { function resolve( child: mixed, context: Object, + threadID: ThreadID, ): {| child: mixed, context: Object, @@ -472,7 +421,7 @@ function resolve( // Extra closure so queue and replace can be captured properly function processChild(element, Component) { - let publicContext = processContext(Component, context); + let publicContext = processContext(Component, context, threadID); let queue = []; let replace = false; @@ -718,6 +667,7 @@ type FrameDev = Frame & { }; class ReactDOMServerRenderer { + threadID: ThreadID; stack: Array; exhausted: boolean; // TODO: type this more strictly: @@ -747,6 +697,7 @@ class ReactDOMServerRenderer { if (__DEV__) { ((topFrame: any): FrameDev).debugElementStack = []; } + this.threadID = allocThreadID(); this.stack = [topFrame]; this.exhausted = false; this.currentSelectValue = null; @@ -763,6 +714,13 @@ class ReactDOMServerRenderer { } } + destroy() { + if (!this.exhausted) { + this.exhausted = true; + freeThreadID(this.threadID); + } + } + /** * 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 @@ -776,7 +734,9 @@ class ReactDOMServerRenderer { pushProvider(provider: ReactProvider): void { const index = ++this.contextIndex; const context: ReactContext = provider.type._context; - const previousValue = context._currentValue; + const threadID = this.threadID; + validateContextBounds(context, threadID); + const previousValue = context[threadID]; // Remember which value to restore this context to on our way up. this.contextStack[index] = context; @@ -787,7 +747,7 @@ class ReactDOMServerRenderer { } // Mutate the current value. - context._currentValue = provider.props.value; + context[threadID] = provider.props.value; } popProvider(provider: ReactProvider): void { @@ -813,7 +773,9 @@ class ReactDOMServerRenderer { this.contextIndex--; // Restore to the previous value we stored as we were walking down. - context._currentValue = previousValue; + // We've already verified that this context has been expanded to accommodate + // this thread id, so we don't need to do it again. + context[this.threadID] = previousValue; } read(bytes: number): string | null { @@ -821,6 +783,8 @@ class ReactDOMServerRenderer { return null; } + const prevThreadID = currentThreadID; + setCurrentThreadID(this.threadID); const prevDispatcher = ReactCurrentOwner.currentDispatcher; if (enableHooks) { ReactCurrentOwner.currentDispatcher = Dispatcher; @@ -835,6 +799,7 @@ class ReactDOMServerRenderer { while (out[0].length < bytes) { if (this.stack.length === 0) { this.exhausted = true; + freeThreadID(this.threadID); break; } const frame: Frame = this.stack[this.stack.length - 1]; @@ -906,6 +871,7 @@ class ReactDOMServerRenderer { return out[0]; } finally { ReactCurrentOwner.currentDispatcher = prevDispatcher; + setCurrentThreadID(prevThreadID); } } @@ -929,7 +895,7 @@ class ReactDOMServerRenderer { return escapeTextForBrowser(text); } else { let nextChild; - ({child: nextChild, context} = resolve(child, context)); + ({child: nextChild, context} = resolve(child, context, this.threadID)); if (nextChild === null || nextChild === false) { return ''; } else if (!React.isValidElement(nextChild)) { @@ -1136,7 +1102,9 @@ class ReactDOMServerRenderer { } } const nextProps: any = (nextChild: any).props; - const nextValue = reactContext._currentValue; + const threadID = this.threadID; + validateContextBounds(reactContext, threadID); + const nextValue = reactContext[threadID]; const nextChildren = toArray(nextProps.children(nextValue)); const frame: Frame = { diff --git a/packages/react-dom/src/server/ReactPartialRendererContext.js b/packages/react-dom/src/server/ReactPartialRendererContext.js new file mode 100644 index 0000000000000..6a0e1fad16f38 --- /dev/null +++ b/packages/react-dom/src/server/ReactPartialRendererContext.js @@ -0,0 +1,104 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import type {ThreadID} from './ReactThreadIDAllocator'; +import type {ReactContext} from 'shared/ReactTypes'; + +import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols'; +import ReactSharedInternals from 'shared/ReactSharedInternals'; +import getComponentName from 'shared/getComponentName'; +import warningWithoutStack from 'shared/warningWithoutStack'; +import checkPropTypes from 'prop-types/checkPropTypes'; + +let ReactDebugCurrentFrame; +if (__DEV__) { + ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame; +} + +const didWarnAboutInvalidateContextType = {}; + +export const emptyObject = {}; +if (__DEV__) { + Object.freeze(emptyObject); +} + +function maskContext(type, context) { + const contextTypes = type.contextTypes; + if (!contextTypes) { + return emptyObject; + } + const maskedContext = {}; + for (const contextName in contextTypes) { + maskedContext[contextName] = context[contextName]; + } + return maskedContext; +} + +function checkContextTypes(typeSpecs, values, location: string) { + if (__DEV__) { + checkPropTypes( + typeSpecs, + values, + location, + 'Component', + ReactDebugCurrentFrame.getCurrentStack, + ); + } +} + +export function validateContextBounds( + context: ReactContext, + threadID: ThreadID, +) { + // If we don't have enough slots in this context to store this threadID, + // fill it in without leaving any holes to ensure that the VM optimizes + // this as non-holey index properties. + for (let i = context._threadCount; i <= threadID; i++) { + // We assume that this is the same as the defaultValue which might not be + // true if we're rendering inside a secondary renderer but they are + // secondary because these use cases are very rare. + context[i] = context._currentValue2; + context._threadCount = i + 1; + } +} + +export function processContext( + type: Function, + context: Object, + threadID: ThreadID, +) { + const contextType = type.contextType; + if (typeof contextType === 'object' && contextType !== null) { + if (__DEV__) { + if (contextType.$$typeof !== REACT_CONTEXT_TYPE) { + let name = getComponentName(type) || 'Component'; + if (!didWarnAboutInvalidateContextType[name]) { + didWarnAboutInvalidateContextType[name] = true; + warningWithoutStack( + false, + '%s defines an invalid contextType. ' + + 'contextType should point to the Context object returned by React.createContext(). ' + + 'Did you accidentally pass the Context.Provider instead?', + name, + ); + } + } + } + validateContextBounds(contextType, threadID); + return contextType[threadID]; + } else { + const maskedContext = maskContext(type, context); + if (__DEV__) { + if (type.contextTypes) { + checkContextTypes(type.contextTypes, maskedContext, 'context'); + } + } + return maskedContext; + } +} diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index d3ddf0d6b6157..2860ee9bfbeb9 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -6,9 +6,13 @@ * * @flow */ + +import type {ThreadID} from './ReactThreadIDAllocator'; import type {ReactContext} from 'shared/ReactTypes'; import areHookInputsEqual from 'shared/areHookInputsEqual'; +import {validateContextBounds} from './ReactPartialRendererContext'; + import invariant from 'shared/invariant'; import warning from 'shared/warning'; @@ -139,7 +143,9 @@ function readContext( context: ReactContext, observedBits: void | number | boolean, ): T { - return context._currentValue; + let threadID = currentThreadID; + validateContextBounds(context, threadID); + return context[threadID]; } function useContext( @@ -147,7 +153,9 @@ function useContext( observedBits: void | number | boolean, ): T { resolveCurrentlyRenderingComponent(); - return context._currentValue; + let threadID = currentThreadID; + validateContextBounds(context, threadID); + return context[threadID]; } function basicStateReducer(state: S, action: BasicStateAction): S { @@ -334,6 +342,12 @@ function dispatchAction( function noop(): void {} +export let currentThreadID: ThreadID = 0; + +export function setCurrentThreadID(threadID: ThreadID) { + currentThreadID = threadID; +} + export const Dispatcher = { readContext, useContext, diff --git a/packages/react-dom/src/server/ReactThreadIDAllocator.js b/packages/react-dom/src/server/ReactThreadIDAllocator.js new file mode 100644 index 0000000000000..1bba2dd50fcac --- /dev/null +++ b/packages/react-dom/src/server/ReactThreadIDAllocator.js @@ -0,0 +1,58 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +// Allocates a new index for each request. Tries to stay as compact as possible so that these +// indices can be used to reference a tightly packaged array. As opposed to being used in a Map. +// The first allocated index is 1. + +import invariant from 'shared/invariant'; + +export type ThreadID = number; + +let nextAvailableThreadIDs = new Uint16Array(16); +for (let i = 0; i < 15; i++) { + nextAvailableThreadIDs[i] = i + 1; +} +nextAvailableThreadIDs[15] = 0; + +function growThreadCountAndReturnNextAvailable() { + let oldArray = nextAvailableThreadIDs; + let oldSize = oldArray.length; + let newSize = oldSize * 2; + invariant( + newSize <= 0x10000, + 'Maximum number of concurrent React renderers exceeded. ' + + 'This can happen if you are not properly destroying the Readable provided by React. ' + + 'Ensure that you call .destroy() on it if you no longer want to read from it.' + + ', and did not read to the end. If you use .pipe() this should be automatic.', + ); + let newArray = new Uint16Array(newSize); + newArray.set(oldArray); + nextAvailableThreadIDs = newArray; + nextAvailableThreadIDs[0] = oldSize + 1; + for (let i = oldSize; i < newSize - 1; i++) { + nextAvailableThreadIDs[i] = i + 1; + } + nextAvailableThreadIDs[newSize - 1] = 0; + return oldSize; +} + +export function allocThreadID(): ThreadID { + let nextID = nextAvailableThreadIDs[0]; + if (nextID === 0) { + return growThreadCountAndReturnNextAvailable(); + } + nextAvailableThreadIDs[0] = nextAvailableThreadIDs[nextID]; + return nextID; +} + +export function freeThreadID(id: ThreadID) { + nextAvailableThreadIDs[id] = nextAvailableThreadIDs[0]; + nextAvailableThreadIDs[0] = id; +} diff --git a/packages/react/src/ReactContext.js b/packages/react/src/ReactContext.js index 71d1b38605446..643a13019e074 100644 --- a/packages/react/src/ReactContext.js +++ b/packages/react/src/ReactContext.js @@ -42,6 +42,9 @@ export function createContext( // Secondary renderers store their context values on separate fields. _currentValue: defaultValue, _currentValue2: defaultValue, + // Used to track how many concurrent renderers this context currently + // supports within in a single renderer. Such as parallel server rendering. + _threadCount: 0, // These are circular Provider: (null: any), Consumer: (null: any), @@ -98,6 +101,14 @@ export function createContext( context._currentValue2 = _currentValue2; }, }, + _threadCount: { + get() { + return context._threadCount; + }, + set(_threadCount) { + context._threadCount = _threadCount; + }, + }, Consumer: { get() { if (!hasWarnedAboutUsingNestedContextConsumers) { diff --git a/packages/shared/ReactTypes.js b/packages/shared/ReactTypes.js index 080ade12b807b..f09207928ceaf 100644 --- a/packages/shared/ReactTypes.js +++ b/packages/shared/ReactTypes.js @@ -59,6 +59,7 @@ export type ReactContext = { _currentValue: T, _currentValue2: T, + _threadCount: number, // DEV only _currentRenderer?: Object | null, diff --git a/scripts/rollup/validate/eslintrc.cjs.js b/scripts/rollup/validate/eslintrc.cjs.js index 82c236f7c2228..a1b4a7fdfac44 100644 --- a/scripts/rollup/validate/eslintrc.cjs.js +++ b/scripts/rollup/validate/eslintrc.cjs.js @@ -12,6 +12,7 @@ module.exports = { Proxy: true, Symbol: true, WeakMap: true, + Uint16Array: true, // Vendor specific MSApp: true, __REACT_DEVTOOLS_GLOBAL_HOOK__: true, diff --git a/scripts/rollup/validate/eslintrc.fb.js b/scripts/rollup/validate/eslintrc.fb.js index 527fd0a473c98..2e2949444f5da 100644 --- a/scripts/rollup/validate/eslintrc.fb.js +++ b/scripts/rollup/validate/eslintrc.fb.js @@ -12,6 +12,7 @@ module.exports = { Symbol: true, Proxy: true, WeakMap: true, + Uint16Array: true, // Vendor specific MSApp: true, __REACT_DEVTOOLS_GLOBAL_HOOK__: true, diff --git a/scripts/rollup/validate/eslintrc.umd.js b/scripts/rollup/validate/eslintrc.umd.js index 7ee1e112f2b09..a3d5fadf2c55e 100644 --- a/scripts/rollup/validate/eslintrc.umd.js +++ b/scripts/rollup/validate/eslintrc.umd.js @@ -11,6 +11,7 @@ module.exports = { Symbol: true, Proxy: true, WeakMap: true, + Uint16Array: true, // Vendor specific MSApp: true, __REACT_DEVTOOLS_GLOBAL_HOOK__: true,