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

Use unique thread ID for each partial render to access Context #14182

Merged
merged 4 commits into from
Nov 9, 2018
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 @@ -348,5 +348,96 @@ describe('ReactDOMServerIntegration', () => {
await render(<App />, 1);
},
);

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>');
});

it('does not pollute parallel node streams when many are used', () => {
const CurrentIndex = React.createContext();

const NthRender = index => (
<CurrentIndex.Provider value={index}>
<header>
<CurrentIndex.Consumer>{idx => idx}</CurrentIndex.Consumer>
</header>
<footer>
<CurrentIndex.Consumer>{idx => idx}</CurrentIndex.Consumer>
</footer>
</CurrentIndex.Provider>
);

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(
'<header>' + i + '</header><footer>' + i + '</footer>',
);
}
});
});
});
6 changes: 5 additions & 1 deletion packages/react-dom/src/server/ReactDOMNodeStreamRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this breaking?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? Or is it a fix?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is it fixing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I submitted a PR to fix this: #14314

this.destroy(err);
}
}
}
Expand Down
16 changes: 12 additions & 4 deletions packages/react-dom/src/server/ReactDOMStringRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why these aren't just return renderer.read(Infinity)?

} finally {
renderer.destroy();
}
}

/**
Expand All @@ -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();
}
}
102 changes: 35 additions & 67 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* @flow
*/

import type {ThreadID} from './ReactThreadIDAllocator';
import type {ReactElement} from 'shared/ReactElementType';
import type {ReactProvider, ReactContext} from 'shared/ReactTypes';

Expand All @@ -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 {
Expand All @@ -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,
Expand All @@ -50,6 +56,8 @@ import {
finishHooks,
Dispatcher,
DispatcherWithoutHooks,
currentThreadID,
setCurrentThreadID,
} from './ReactPartialRendererHooks';
import {
Namespaces,
Expand Down Expand Up @@ -176,7 +184,6 @@ const didWarnAboutBadClass = {};
const didWarnAboutDeprecatedWillMount = {};
const didWarnAboutUndefinedDerivedState = {};
const didWarnAboutUninitializedState = {};
const didWarnAboutInvalidateContextType = {};
const valuePropNames = ['value', 'defaultValue'];
const newlineEatingTags = {
listing: true,
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -453,6 +401,7 @@ function validateRenderResult(child, type) {
function resolve(
child: mixed,
context: Object,
threadID: ThreadID,
): {|
child: mixed,
context: Object,
Expand All @@ -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;
Expand Down Expand Up @@ -718,6 +667,7 @@ type FrameDev = Frame & {
};

class ReactDOMServerRenderer {
threadID: ThreadID;
stack: Array<Frame>;
exhausted: boolean;
// TODO: type this more strictly:
Expand Down Expand Up @@ -747,6 +697,7 @@ class ReactDOMServerRenderer {
if (__DEV__) {
((topFrame: any): FrameDev).debugElementStack = [];
}
this.threadID = allocThreadID();
this.stack = [topFrame];
this.exhausted = false;
this.currentSelectValue = null;
Expand All @@ -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
Expand All @@ -776,7 +734,9 @@ class ReactDOMServerRenderer {
pushProvider<T>(provider: ReactProvider<T>): void {
const index = ++this.contextIndex;
const context: ReactContext<any> = 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;
Expand All @@ -787,7 +747,7 @@ class ReactDOMServerRenderer {
}

// Mutate the current value.
context._currentValue = provider.props.value;
context[threadID] = provider.props.value;
}

popProvider<T>(provider: ReactProvider<T>): void {
Expand All @@ -813,14 +773,18 @@ 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 {
if (this.exhausted) {
return null;
}

const prevThreadID = currentThreadID;
setCurrentThreadID(this.threadID);
const prevDispatcher = ReactCurrentOwner.currentDispatcher;
if (enableHooks) {
ReactCurrentOwner.currentDispatcher = Dispatcher;
Expand All @@ -835,6 +799,7 @@ class ReactDOMServerRenderer {
while (out[0].length < bytes) {
if (this.stack.length === 0) {
this.exhausted = true;
freeThreadID(this.threadID);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also replace these two lines with this.destroy()

break;
}
const frame: Frame = this.stack[this.stack.length - 1];
Expand Down Expand Up @@ -906,6 +871,7 @@ class ReactDOMServerRenderer {
return out[0];
} finally {
ReactCurrentOwner.currentDispatcher = prevDispatcher;
setCurrentThreadID(prevThreadID);
}
}

Expand All @@ -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)) {
Expand Down Expand Up @@ -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 = {
Expand Down
Loading