Skip to content

Conversation

leebyron
Copy link
Contributor

Fixes #13874

This is just a straw-man 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.

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.
Fixes facebook#13874

This is just a strawman approach for a fix for facebook#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.
@sizebot
Copy link

sizebot commented Oct 17, 2018

React: size: 🔺+0.5%, gzip: 🔺+0.5%

Details of bundled changes.

Comparing: 4f0bd45...d137b03

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.4% +0.5% 94.15 KB 94.49 KB 24.77 KB 24.9 KB UMD_DEV
react.production.min.js 🔺+0.5% 🔺+0.5% 11.72 KB 11.78 KB 4.64 KB 4.66 KB UMD_PROD
react.development.js +0.6% +0.8% 56.86 KB 57.2 KB 15.28 KB 15.41 KB NODE_DEV
react.production.min.js 🔺+1.0% 🔺+0.9% 6.14 KB 6.19 KB 2.62 KB 2.64 KB NODE_PROD
React-dev.js +0.6% +1.0% 53.25 KB 53.59 KB 14.46 KB 14.6 KB FB_WWW_DEV
React-prod.js 🔺+0.9% 🔺+1.1% 14.03 KB 14.16 KB 3.85 KB 3.9 KB FB_WWW_PROD
React-profiling.js +0.9% +1.1% 14.03 KB 14.16 KB 3.85 KB 3.9 KB FB_WWW_PROFILING
react.profiling.min.js +0.4% +0.6% 13.88 KB 13.93 KB 5.15 KB 5.18 KB UMD_PROFILING

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-server.browser.development.js -0.5% -0.7% 106.56 KB 105.98 KB 28.3 KB 28.09 KB UMD_DEV
react-dom-server.browser.production.min.js -0.4% -0.2% 15.53 KB 15.46 KB 5.95 KB 5.94 KB UMD_PROD
react-dom-server.browser.development.js -0.6% -0.8% 102.69 KB 102.11 KB 27.3 KB 27.09 KB NODE_DEV
react-dom-server.browser.production.min.js -0.4% -0.1% 15.44 KB 15.37 KB 5.88 KB 5.87 KB NODE_PROD
react-dom-server.node.development.js -0.6% -0.8% 104.61 KB 104.03 KB 27.85 KB 27.63 KB NODE_DEV
react-dom-server.node.production.min.js -0.4% -0.1% 16.24 KB 16.17 KB 6.18 KB 6.18 KB NODE_PROD
ReactDOMServer-dev.js -0.5% -0.8% 102.16 KB 101.64 KB 26.59 KB 26.39 KB FB_WWW_DEV
ReactDOMServer-prod.js -0.3% 🔺+0.3% 34 KB 33.88 KB 8.17 KB 8.19 KB FB_WWW_PROD

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

leebyron added a commit to leebyron/react that referenced this pull request Oct 17, 2018
…storage")

Fixes facebook#13874

Alternative to facebook#13875

This straw-man approach also avoids mutating global context by giving each ReactPartialRenderer it's own mutable state and using context instances as keys into that mutable state.
@leebyron
Copy link
Contributor Author

Closing in favor of #13877 which is a simpler and probably more efficient fix

@leebyron leebyron closed this Oct 17, 2018
@leebyron leebyron deleted the fix-node-stream-bug branch October 17, 2018 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants