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.

Sign up for GitHub

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

Jump to bottom

renderComponentToString with events leaks unless component unmounted #954

Closed
glenn-murray-bse opened this issue Jan 23, 2014 · 8 comments
Closed

renderComponentToString with events leaks unless component unmounted #954

glenn-murray-bse opened this issue Jan 23, 2014 · 8 comments

Comments

@glenn-murray-bse
Copy link

Repeated calls to renderComponentToString with evented components (i.e. onClick={this.handleClick}) will effectively leak memory.

Call unmountComponent on rendered components lest event listening closures remain non-garbage collected.

Only long lived processes with repeated render calls will notice.

http://jsfiddle.net/tS8Nv/1/

Adding component.unmountComponent() at ReactServerRendering.js:54 would fix this, but would break tests which render react root document nodes with an Invariant Violation.

var Link = React.createClass({
  leak: function(event) {

  },
  render: function() {
    return <a onClick={this.leak} >Test</a>;
  }
})

function log(str) {
  console.log(str);
}

var root = <html><body><Link /></body></html>
React.renderComponentToString(root, log);
root.unmountComponent();

renderComponentToString is API bound to be async so this sample is not future proof.
@plievone
Copy link
Contributor

So renderComponentToString should somehow prevent this call https://github.com/facebook/react/blob/0af9c3e/src/core/ReactDOMComponent.js#L146 or its effects? (I guess there isn't any use case where one first renders to string in browser and then inserts the markup manually, expecting events to work)

Btw perhaps some day putListener could be putHandler or putCallback, actual top level listeners are different so consistent terminology would help reading the codebase.

glenn-murray-bse pushed a commit to ascom-au/react that referenced this issue Jan 24, 2014
Failing test for memory leak issue facebook#954
d422ebd 
renderComponentToString registers events. If the calling code
does not unmount the component or unregister the listeners
this will leak memory. Passing this test should fix this.
glenn-murray-bse pushed a commit to ascom-au/react that referenced this issue Jan 24, 2014
Flag to prevent registration of events facebook#954
0b5ad13 
Flag on EventPluginHub to disable event registration.
For the purposes of server side rendering not registering events.
Could be cleaner if used a transaction with an optional step.
Alternatively could be an ExecutionEnvironment variable.
Could also be flag passed to mount component, however non DOM
components have no comprehension of events.
glenn-murray-bse pushed a commit to ascom-au/react that referenced this issue Jan 24, 2014
Test coverage for facebook#954 to allow disabling of event registration
8dbeb85 
EventPluginHub.setRegistrationEnabled(boolean) will toggle
whether putListener will register event listeners.
@glenn-murray-bse
Copy link
Author

Given that (for my use case) renderComponentToString should not register listeners, I created a failing test case d422ebd, a fix 0b5ad13 and test coverage for the fix 8dbeb85

The exposed changes are:

EventPluginHub.setRegistrationEnabled(boolean)
EventPluginHub.isRegistrationEnabled() // returns boolean

Calls to React.renderComponentToString will no longer register event listeners, and thus, won't leak memory.

glenn-murray-bse pushed a commit to ascom-au/react that referenced this issue Jan 24, 2014
White box test possible side effects of disabling events facebook#954
4ce61c1
@glenn-murray-bse glenn-murray-bse mentioned this issue Jan 24, 2014
Closed
@glenn-murray-bse
Copy link
Author

#860 (comment)

@glenn-murray-bse
Copy link
Author

The negative case test case d422ebd from the closed pull request may still be useful.

@glenn-murray-bse
Copy link
Author

Tested for memory leak using node webkit agent heap inspection with git head 34e6a51

Closure leaks due to event registration no longer occur since 526be15

@petehunt
Copy link
Contributor

@glenn-murray-bse thanks! would love to find a way to automate the workflow you're using and put it on CI.

@glenn-murray-bse
Copy link
Author

My current workflow involves a GUI (the chrome dev tools) so would be difficult to codify.

There are programatic heap dumpers for node (eg https://github.com/bnoordhuis/node-heapdump, https://github.com/davepacheco/node-heap-dump), but none I could find which automate comparisons between heap snapshots.

Either of those modules would probably provide a good place to start.

@petehunt
Copy link
Contributor

Cool I'll take a look. I'm experimenting with automated tooling around this kind of stuff (#620) so if anyone comes up with any ideas be sure to let me know :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

renderComponentToString memory leak fix for #954 ascom-au/react
3 participants
@plievone @petehunt @glenn-murray-bse