Skip to content

Commit dbc6d81

Browse files
sebmarkbagejetoneza
authored andcommitted
Use unique thread ID for each partial render to access Context (facebook#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.
1 parent d2115e6 commit dbc6d81

12 files changed

+336
-74
lines changed

packages/react-dom/src/__tests__/ReactDOMServerIntegrationNewContext-test.js

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,5 +348,96 @@ describe('ReactDOMServerIntegration', () => {
348348
await render(<App />, 1);
349349
},
350350
);
351+
352+
it('does not pollute parallel node streams', () => {
353+
const LoggedInUser = React.createContext();
354+
355+
const AppWithUser = user => (
356+
<LoggedInUser.Provider value={user}>
357+
<header>
358+
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
359+
</header>
360+
<footer>
361+
<LoggedInUser.Consumer>{whoAmI => whoAmI}</LoggedInUser.Consumer>
362+
</footer>
363+
</LoggedInUser.Provider>
364+
);
365+
366+
const streamAmy = ReactDOMServer.renderToNodeStream(
367+
AppWithUser('Amy'),
368+
).setEncoding('utf8');
369+
const streamBob = ReactDOMServer.renderToNodeStream(
370+
AppWithUser('Bob'),
371+
).setEncoding('utf8');
372+
373+
// Testing by filling the buffer using internal _read() with a small
374+
// number of bytes to avoid a test case which needs to align to a
375+
// highWaterMark boundary of 2^14 chars.
376+
streamAmy._read(20);
377+
streamBob._read(20);
378+
streamAmy._read(20);
379+
streamBob._read(20);
380+
381+
expect(streamAmy.read()).toBe('<header>Amy</header><footer>Amy</footer>');
382+
expect(streamBob.read()).toBe('<header>Bob</header><footer>Bob</footer>');
383+
});
384+
385+
it('does not pollute parallel node streams when many are used', () => {
386+
const CurrentIndex = React.createContext();
387+
388+
const NthRender = index => (
389+
<CurrentIndex.Provider value={index}>
390+
<header>
391+
<CurrentIndex.Consumer>{idx => idx}</CurrentIndex.Consumer>
392+
</header>
393+
<footer>
394+
<CurrentIndex.Consumer>{idx => idx}</CurrentIndex.Consumer>
395+
</footer>
396+
</CurrentIndex.Provider>
397+
);
398+
399+
let streams = [];
400+
401+
// Test with more than 32 streams to test that growing the thread count
402+
// works properly.
403+
let streamCount = 34;
404+
405+
for (let i = 0; i < streamCount; i++) {
406+
streams[i] = ReactDOMServer.renderToNodeStream(
407+
NthRender(i % 2 === 0 ? 'Expected to be recreated' : i),
408+
).setEncoding('utf8');
409+
}
410+
411+
// Testing by filling the buffer using internal _read() with a small
412+
// number of bytes to avoid a test case which needs to align to a
413+
// highWaterMark boundary of 2^14 chars.
414+
for (let i = 0; i < streamCount; i++) {
415+
streams[i]._read(20);
416+
}
417+
418+
// Early destroy every other stream
419+
for (let i = 0; i < streamCount; i += 2) {
420+
streams[i].destroy();
421+
}
422+
423+
// Recreate those same streams.
424+
for (let i = 0; i < streamCount; i += 2) {
425+
streams[i] = ReactDOMServer.renderToNodeStream(
426+
NthRender(i),
427+
).setEncoding('utf8');
428+
}
429+
430+
// Read a bit from all streams again.
431+
for (let i = 0; i < streamCount; i++) {
432+
streams[i]._read(20);
433+
}
434+
435+
// Assert that all stream rendered the expected output.
436+
for (let i = 0; i < streamCount; i++) {
437+
expect(streams[i].read()).toBe(
438+
'<header>' + i + '</header><footer>' + i + '</footer>',
439+
);
440+
}
441+
});
351442
});
352443
});

packages/react-dom/src/server/ReactDOMNodeStreamRenderer.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,15 @@ class ReactMarkupReadableStream extends Readable {
1818
this.partialRenderer = new ReactPartialRenderer(element, makeStaticMarkup);
1919
}
2020

21+
_destroy() {
22+
this.partialRenderer.destroy();
23+
}
24+
2125
_read(size) {
2226
try {
2327
this.push(this.partialRenderer.read(size));
2428
} catch (err) {
25-
this.emit('error', err);
29+
this.destroy(err);
2630
}
2731
}
2832
}

packages/react-dom/src/server/ReactDOMStringRenderer.js

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,12 @@ import ReactPartialRenderer from './ReactPartialRenderer';
1414
*/
1515
export function renderToString(element) {
1616
const renderer = new ReactPartialRenderer(element, false);
17-
const markup = renderer.read(Infinity);
18-
return markup;
17+
try {
18+
const markup = renderer.read(Infinity);
19+
return markup;
20+
} finally {
21+
renderer.destroy();
22+
}
1923
}
2024

2125
/**
@@ -25,6 +29,10 @@ export function renderToString(element) {
2529
*/
2630
export function renderToStaticMarkup(element) {
2731
const renderer = new ReactPartialRenderer(element, true);
28-
const markup = renderer.read(Infinity);
29-
return markup;
32+
try {
33+
const markup = renderer.read(Infinity);
34+
return markup;
35+
} finally {
36+
renderer.destroy();
37+
}
3038
}

packages/react-dom/src/server/ReactPartialRenderer.js

Lines changed: 35 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
* @flow
88
*/
99

10+
import type {ThreadID} from './ReactThreadIDAllocator';
1011
import type {ReactElement} from 'shared/ReactElementType';
1112
import type {ReactProvider, ReactContext} from 'shared/ReactTypes';
1213

@@ -16,7 +17,6 @@ import getComponentName from 'shared/getComponentName';
1617
import lowPriorityWarning from 'shared/lowPriorityWarning';
1718
import warning from 'shared/warning';
1819
import warningWithoutStack from 'shared/warningWithoutStack';
19-
import checkPropTypes from 'prop-types/checkPropTypes';
2020
import describeComponentFrame from 'shared/describeComponentFrame';
2121
import ReactSharedInternals from 'shared/ReactSharedInternals';
2222
import {
@@ -39,6 +39,12 @@ import {
3939
REACT_MEMO_TYPE,
4040
} from 'shared/ReactSymbols';
4141

42+
import {
43+
emptyObject,
44+
processContext,
45+
validateContextBounds,
46+
} from './ReactPartialRendererContext';
47+
import {allocThreadID, freeThreadID} from './ReactThreadIDAllocator';
4248
import {
4349
createMarkupForCustomAttribute,
4450
createMarkupForProperty,
@@ -50,6 +56,8 @@ import {
5056
finishHooks,
5157
Dispatcher,
5258
DispatcherWithoutHooks,
59+
currentThreadID,
60+
setCurrentThreadID,
5361
} from './ReactPartialRendererHooks';
5462
import {
5563
Namespaces,
@@ -176,7 +184,6 @@ const didWarnAboutBadClass = {};
176184
const didWarnAboutDeprecatedWillMount = {};
177185
const didWarnAboutUndefinedDerivedState = {};
178186
const didWarnAboutUninitializedState = {};
179-
const didWarnAboutInvalidateContextType = {};
180187
const valuePropNames = ['value', 'defaultValue'];
181188
const newlineEatingTags = {
182189
listing: true,
@@ -324,65 +331,6 @@ function flattenOptionChildren(children: mixed): ?string {
324331
return content;
325332
}
326333

327-
const emptyObject = {};
328-
if (__DEV__) {
329-
Object.freeze(emptyObject);
330-
}
331-
332-
function maskContext(type, context) {
333-
const contextTypes = type.contextTypes;
334-
if (!contextTypes) {
335-
return emptyObject;
336-
}
337-
const maskedContext = {};
338-
for (const contextName in contextTypes) {
339-
maskedContext[contextName] = context[contextName];
340-
}
341-
return maskedContext;
342-
}
343-
344-
function checkContextTypes(typeSpecs, values, location: string) {
345-
if (__DEV__) {
346-
checkPropTypes(
347-
typeSpecs,
348-
values,
349-
location,
350-
'Component',
351-
getCurrentServerStackImpl,
352-
);
353-
}
354-
}
355-
356-
function processContext(type, context) {
357-
const contextType = type.contextType;
358-
if (typeof contextType === 'object' && contextType !== null) {
359-
if (__DEV__) {
360-
if (contextType.$$typeof !== REACT_CONTEXT_TYPE) {
361-
let name = getComponentName(type) || 'Component';
362-
if (!didWarnAboutInvalidateContextType[name]) {
363-
didWarnAboutInvalidateContextType[type] = true;
364-
warningWithoutStack(
365-
false,
366-
'%s defines an invalid contextType. ' +
367-
'contextType should point to the Context object returned by React.createContext(). ' +
368-
'Did you accidentally pass the Context.Provider instead?',
369-
name,
370-
);
371-
}
372-
}
373-
}
374-
return contextType._currentValue;
375-
} else {
376-
const maskedContext = maskContext(type, context);
377-
if (__DEV__) {
378-
if (type.contextTypes) {
379-
checkContextTypes(type.contextTypes, maskedContext, 'context');
380-
}
381-
}
382-
return maskedContext;
383-
}
384-
}
385-
386334
const hasOwnProperty = Object.prototype.hasOwnProperty;
387335
const STYLE = 'style';
388336
const RESERVED_PROPS = {
@@ -453,6 +401,7 @@ function validateRenderResult(child, type) {
453401
function resolve(
454402
child: mixed,
455403
context: Object,
404+
threadID: ThreadID,
456405
): {|
457406
child: mixed,
458407
context: Object,
@@ -472,7 +421,7 @@ function resolve(
472421

473422
// Extra closure so queue and replace can be captured properly
474423
function processChild(element, Component) {
475-
let publicContext = processContext(Component, context);
424+
let publicContext = processContext(Component, context, threadID);
476425

477426
let queue = [];
478427
let replace = false;
@@ -718,6 +667,7 @@ type FrameDev = Frame & {
718667
};
719668

720669
class ReactDOMServerRenderer {
670+
threadID: ThreadID;
721671
stack: Array<Frame>;
722672
exhausted: boolean;
723673
// TODO: type this more strictly:
@@ -747,6 +697,7 @@ class ReactDOMServerRenderer {
747697
if (__DEV__) {
748698
((topFrame: any): FrameDev).debugElementStack = [];
749699
}
700+
this.threadID = allocThreadID();
750701
this.stack = [topFrame];
751702
this.exhausted = false;
752703
this.currentSelectValue = null;
@@ -763,6 +714,13 @@ class ReactDOMServerRenderer {
763714
}
764715
}
765716

717+
destroy() {
718+
if (!this.exhausted) {
719+
this.exhausted = true;
720+
freeThreadID(this.threadID);
721+
}
722+
}
723+
766724
/**
767725
* Note: We use just two stacks regardless of how many context providers you have.
768726
* Providers are always popped in the reverse order to how they were pushed
@@ -776,7 +734,9 @@ class ReactDOMServerRenderer {
776734
pushProvider<T>(provider: ReactProvider<T>): void {
777735
const index = ++this.contextIndex;
778736
const context: ReactContext<any> = provider.type._context;
779-
const previousValue = context._currentValue;
737+
const threadID = this.threadID;
738+
validateContextBounds(context, threadID);
739+
const previousValue = context[threadID];
780740

781741
// Remember which value to restore this context to on our way up.
782742
this.contextStack[index] = context;
@@ -787,7 +747,7 @@ class ReactDOMServerRenderer {
787747
}
788748

789749
// Mutate the current value.
790-
context._currentValue = provider.props.value;
750+
context[threadID] = provider.props.value;
791751
}
792752

793753
popProvider<T>(provider: ReactProvider<T>): void {
@@ -813,14 +773,18 @@ class ReactDOMServerRenderer {
813773
this.contextIndex--;
814774

815775
// Restore to the previous value we stored as we were walking down.
816-
context._currentValue = previousValue;
776+
// We've already verified that this context has been expanded to accommodate
777+
// this thread id, so we don't need to do it again.
778+
context[this.threadID] = previousValue;
817779
}
818780

819781
read(bytes: number): string | null {
820782
if (this.exhausted) {
821783
return null;
822784
}
823785

786+
const prevThreadID = currentThreadID;
787+
setCurrentThreadID(this.threadID);
824788
const prevDispatcher = ReactCurrentOwner.currentDispatcher;
825789
if (enableHooks) {
826790
ReactCurrentOwner.currentDispatcher = Dispatcher;
@@ -835,6 +799,7 @@ class ReactDOMServerRenderer {
835799
while (out[0].length < bytes) {
836800
if (this.stack.length === 0) {
837801
this.exhausted = true;
802+
freeThreadID(this.threadID);
838803
break;
839804
}
840805
const frame: Frame = this.stack[this.stack.length - 1];
@@ -906,6 +871,7 @@ class ReactDOMServerRenderer {
906871
return out[0];
907872
} finally {
908873
ReactCurrentOwner.currentDispatcher = prevDispatcher;
874+
setCurrentThreadID(prevThreadID);
909875
}
910876
}
911877

@@ -929,7 +895,7 @@ class ReactDOMServerRenderer {
929895
return escapeTextForBrowser(text);
930896
} else {
931897
let nextChild;
932-
({child: nextChild, context} = resolve(child, context));
898+
({child: nextChild, context} = resolve(child, context, this.threadID));
933899
if (nextChild === null || nextChild === false) {
934900
return '';
935901
} else if (!React.isValidElement(nextChild)) {
@@ -1136,7 +1102,9 @@ class ReactDOMServerRenderer {
11361102
}
11371103
}
11381104
const nextProps: any = (nextChild: any).props;
1139-
const nextValue = reactContext._currentValue;
1105+
const threadID = this.threadID;
1106+
validateContextBounds(reactContext, threadID);
1107+
const nextValue = reactContext[threadID];
11401108

11411109
const nextChildren = toArray(nextProps.children(nextValue));
11421110
const frame: Frame = {

0 commit comments

Comments
 (0)