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 ReactServerRendering memory leak in ReactEmptyComponent #6060

Merged
merged 1 commit into from Mar 29, 2016

Conversation

Projects
None yet
8 participants
@iancmyers

iancmyers commented Feb 18, 2016

ReactEmptyComponent registers null component IDs on mount and deregisters them on unmount. However, when rendering server-side unmountComponent is never called on the component instance. As a result, null IDs accumulate over time.

The solution in the PR is simply to not register the IDs when server-side rendering. I have no idea if that's the right solution, but it's certainly a solution!

I'm also not sure if this is still an issue in 0.15. The issue was found in 0.14.7. You can see in this heapdump a ton of React ID strings, each one being retained by nullComponentIDsRegistry:

screen shot 2016-02-17 at 5 11 16 pm

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Feb 18, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@sophiebits

This comment has been minimized.

Member

sophiebits commented Feb 18, 2016

I think this would be okay since you shouldn't ever end up calling findDOMNode on the server (which is what the registry is for). As I said on Twitter, unlikely we'll do another 0.14 release – but I'll leave this open until the next major.

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Feb 18, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@jimfb

This comment has been minimized.

Contributor

jimfb commented Feb 18, 2016

@iancmyers can you verify (or @spicyj maybe you already know?) if the issue still exists on v15.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Feb 18, 2016

Should be completely fixed in master since I refactored a lot of stuff.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Feb 18, 2016

I wonder how we can catch these sorts of regressions.

@lelandrichardson

This comment has been minimized.

Contributor

lelandrichardson commented Feb 18, 2016

@spicyj Update: this isn't actually fixing it. This is passing in isServerRendering for the root node, but not for all of the other nodes in the render path. Any suggestions on how to do this a different way?

In mountComponent, we need to know if we are in "renderToString" or not.

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Feb 18, 2016

@iancmyers updated the pull request.

@iancmyers

This comment has been minimized.

iancmyers commented Feb 18, 2016

I've updated this PR with something that actually fixes the issue. It dumps all null component IDs after each server rendering transaction.

@zpao

This comment has been minimized.

Member

zpao commented Feb 18, 2016

Process note for reviewers: don't just merge this when/if it's accepted. We'll do that if we need to ship 0.14.8.

@ljharb

This comment has been minimized.

Contributor

ljharb commented Feb 22, 2016

@zpao what's the likelihood to fix this with a 0.14.8, prior to 15.0?

@jimfb

This comment has been minimized.

Contributor

jimfb commented Feb 22, 2016

@ljharb low.

@ljharb

This comment has been minimized.

Contributor

ljharb commented Feb 22, 2016

:-/ that doesn't seem to jibe well with the "commitment to stability" promised in the 15.x blog post, but perhaps it will begin after v15 is released ¯_(ツ)_/¯ (Unfortunately due to dropping IE 8 we may not be able to upgrade to it)

@jimfb

This comment has been minimized.

Contributor

jimfb commented Feb 22, 2016

@ljharb Stability refers to us not making crazy breaking API changes. We've been relatively stable for a while, and wanted to communicate that by bumping the version number (resolves issues like: #4998 and #5077). As the community continues to grow (at an astonishing rate), we are becoming more and more focused on stability, and we can expect that trend to continue. But stability does not imply backporting fixes to old versions - we still expect people to stay up-to-date. This is already fixed in master and we will be releasing v15 soon.

With regards to IE8 - the post does not imply that we will intentionally break IE8 nor does it say that IE8 will suddenly break catastrophically in IE8. It merely means we aren't going to invest effort in maintaining IE8. AFAIK, React still works in IE8 (untested). If IE8 support is important to you, and you find a bug, feel free to submit a PR and there is a good chance we would take it. Keep in mind that IE8 is six years old, and IE8, IE9 and IE10 are all unsupported by microsoft.

@zpao

This comment has been minimized.

Member

zpao commented Feb 23, 2016

I'd be willing to do a 0.14.8. Memory leaks suck and we haven't even shipped 15 yet.

@ljharb

This comment has been minimized.

Contributor

ljharb commented Feb 23, 2016

It's not backporting until v15 is released, and it's absolutely not stability to immediately stop backporting fixes to "major version minus one" - just because you've released a new version doesn't mean everyone has instantly started using it, and yes, imo, you do have an ethical obligation to not leave those users in a position where they're screwed unless they upgrade instantly.

@zpao thanks, that would help us out a lot - migrating from one major to another is much easier when the bug landscape is as empty as possible.

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Feb 23, 2016

@iancmyers updated the pull request.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Feb 25, 2016

It's also a long-standing bug that's been around since v0.11 – almost two years ago – that no one has noticed until now, when at the same time we have it fixed in master as part of a larger refactor.

I appreciate that it can be difficult to upgrade and we can do a 0.14.8 if we are able to get a proper fix for this in. The fix proposed here is incorrect. Specifically:

var X = React.createClass({
  render: function() {
    return null;
  },
  componentDidMount: function() {
    ReactDOMServer.renderToString(<div />);
    console.log(ReactDOM.findDOMNode(this));
  },
});

will now log the noscript tag instead of null, as it should.

I would suggest instead using transaction.getReactMountReady().enqueue() to defer the registerNullComponentID call within ReactEmptyComponent so that the registration does not occur until componentDidMount time (which never occurs on the server). This is slightly slower overall but should fix the issue.

Register null component IDs at componentDidMount
This pushes registering of null component IDs to componentDidMount.
The result that null component IDs are never registered during
server-side rendering, which fixes a memory leak with null component
IDs never being cleaned up server-side.
@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Mar 2, 2016

@iancmyers updated the pull request.

@sophiebits

This comment has been minimized.

Member

sophiebits commented Mar 2, 2016

👍 Fine with me now. @zpao if you want to cut a release.

@sophiebits sophiebits added this to the 0.14.x milestone Mar 2, 2016

@gaearon gaearon modified the milestones: 0.14.x, 15.0 Mar 27, 2016

@gaearon gaearon merged commit c03a057 into facebook:0.14-stable Mar 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaearon

This comment has been minimized.

Member

gaearon commented Mar 29, 2016

Thank you! This should be out in 0.14.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment