-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Fix react-dom/server context leaks when render stream destroyed early #14706
Fix react-dom/server context leaks when render stream destroyed early #14706
Conversation
2nd test added for #14705 (comment). |
@@ -776,6 +777,16 @@ class ReactDOMServerRenderer { | |||
context[this.threadID] = previousValue; | |||
} | |||
|
|||
clearProviders(): void { | |||
while (this.contextIndex > -1) { | |||
if (__DEV__) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write the loop inline with minimal work it needs to do? For example reading previous value seems unnecessary except for the last one. This would also avoid the overloading you had to add to popProvider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed another commit which does as you request.
However, the previous value does need to be restored for each Provider, as each Context's current value is stored separately on each Context object.
Well, if there were several nested Providers for the same Context, then yes you could only restore just the last previous value. But I don't think this would be more performative, as clearProviders()
would need to loop over the array twice - once to identify any repeated contexts, and then a 2nd time to restore whatever was the oldest previous value for each Context.
I think it's probably cheaper to redundantly write the value multiple times in these cases.
But if you want further changes, please just say so and I'll implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB This leaves contextStack
and contextValueStack
unaltered, and so retaining memory. But I guess since the PartialRenderer
instance has been destroyed, it will be dereferenced and garbage collected shortly anyway.
8088074
to
94f70f5
Compare
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: 08e9554...855cc27 react-dom
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. Thanks for fixing.
Thank you! |
…facebook#14706) * Fix react-dom/server context memory retention * Test for pollution of later renders * Inline loop * More tests
## 16.8.2 => 16.8.3 ### React DOM * Fix a bug that caused inputs to behave incorrectly in UMD builds. ([@gaearon](https://github.com/gaearon) in [#14914](facebook/react#14914)) * Fix a bug that caused render phase updates to be discarded. ([@gaearon](https://github.com/gaearon) in [#14852](facebook/react#14852)) ### React DOM Server * Unwind the context stack when a stream is destroyed without completing, to prevent incorrect values during a subsequent render. ([@overlookmotel](https://github.com/overlookmotel) in [#14706](facebook/react#14706))
## 16.8.2 => 16.8.3 ### React DOM * Fix a bug that caused inputs to behave incorrectly in UMD builds. ([@gaearon](https://github.com/gaearon) in [#14914](facebook/react#14914)) * Fix a bug that caused render phase updates to be discarded. ([@gaearon](https://github.com/gaearon) in [#14852](facebook/react#14852)) ### React DOM Server * Unwind the context stack when a stream is destroyed without completing, to prevent incorrect values during a subsequent render. ([@overlookmotel](https://github.com/overlookmotel) in [#14706](facebook/react#14706))
Fix and test case for #14705.
Note: This makes
.popProvider
'sprovider
argument optional. It's only used in dev mode anyway, but I'm not sure if this is the wrong thing to do. Please excuse me - I've never used Flow before.