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

Memory leak in ReactDOMInput on server rendering #4870

Closed
STRML opened this issue Sep 14, 2015 · 5 comments
Closed

Memory leak in ReactDOMInput on server rendering #4870

STRML opened this issue Sep 14, 2015 · 5 comments
Assignees
@sophiebits
Milestone
0.14

Comments

@STRML
Copy link
Contributor

STRML commented Sep 14, 2015

In React 0.14.0-rc1 I've seen a massive memory leak as it appears my entire app is being retained.

After some time in the profiler, I narrowed it down to ReactDOMInput's instancesByReactID (I have a radio input in my app):

screen shot 2015-09-14 at 1 41 02 pm

Removing instancesByReactID entirely in the source fixes the leak.

To diagnose this, I added some log statements:

  mountWrapper: function (inst, props) {
    LinkedValueUtils.checkPropTypes('input', props, inst._currentElement._owner);

    var defaultValue = props.defaultValue;
    inst._wrapperState = {
      initialChecked: props.defaultChecked || false,
      initialValue: defaultValue != null ? defaultValue : null,
      onChange: _handleChange.bind(inst)
    };

    console.log("mounted input, id", inst._rootNodeID);
    instancesByReactID[inst._rootNodeID] = inst;
  },

  unmountWrapper: function (inst) {
    console.log("unmounted input, id", inst._rootNodeID);
    delete instancesByReactID[inst._rootNodeID];
  },

Then ran the app:

mounted input, id .gkkc0jr400.1.0.1.2.0.1.0.5.$basic/=1$basic.$basic
mounted input, id .gkkc0jr400.1.0.1.2.0.1.0.5.$basic/=1$advanced.$advanced
INFO: 127.0.0.1 (Chrome 46, Mac OS) - "GET /app/trade/XBTU15" 200 -b - 312.409 ms
mounted input, id .280er686xog.1.0.1.2.0.1.0.5.$basic/=1$basic.$basic
mounted input, id .280er686xog.1.0.1.2.0.1.0.5.$basic/=1$advanced.$advanced
INFO: 127.0.0.1 (Chrome 46, Mac OS) - "GET /app/trade/XBTU15" 200 -b - 587.734 ms

Notice that the unmount never logs, so these instances stay in instancesByReactID forever.

I am simply rendering using ReactDOMServer.renderToString().
@STRML
Copy link
Contributor Author

STRML commented Sep 14, 2015

The difference appears to be that in 0.13, adding the node to instancesByReactID happened in componentDidMount, which never fires on the server:

  componentDidMount: function() {
    var id = ReactMount.getID(this.getDOMNode());
    console.log('mounting', id);
    instancesByReactID[id] = this;
  },

  componentWillUnmount: function() {
    var rootNode = this.getDOMNode();
    var id = ReactMount.getID(rootNode);
    console.log('unmounting', id);
    delete instancesByReactID[id];
  },

So my console is quiet when running the app with these log statements.

@zpao
Copy link
Member

zpao commented Sep 14, 2015

cc @spicyj

@sophiebits sophiebits added this to the 0.14 milestone Sep 15, 2015
@STRML
Copy link
Contributor Author

STRML commented Sep 17, 2015

@zpao Is there a plan for this, or do you have some pointers for how the team wants to handle server rendering architecturally so mount code like this doesn't fire? Happy to do a PR, this is definitely a 0.14 blocker. Our testnet servers crash every 8hrs because of this.

@sophiebits
Copy link
Collaborator

I haven't decided how best to fix this. You can see I already marked it for the 0.14 milestone though.

sophiebits added a commit to sophiebits/react that referenced this issue Sep 25, 2015
@sophiebits
Don't store <input> instance until mount-ready
c04d10f 
Fixes facebook#4870.

This more or less matches what we were doing with the old wrapper components (not storing until componentDidMount).
@sophiebits sophiebits mentioned this issue Sep 25, 2015
Merged
sophiebits added a commit that referenced this issue Sep 25, 2015
@sophiebits
Merge pull request #4976 from spicyj/gh-4870
b82f3ab 
Don't store <input> instance until mount-ready
sophiebits added a commit to sophiebits/react that referenced this issue Sep 25, 2015
@sophiebits
Actually don't store <input> until mount-ready
623b06d 
So facebook#4976 was a lie. Fixes facebook#4870 for real (thanks @STRML).
sophiebits added a commit to sophiebits/react that referenced this issue Sep 25, 2015
@sophiebits
Actually don't store <input> until mount-ready
6e62a01 
So facebook#4976 was a lie. Fixes facebook#4870 for real (thanks @STRML).
@sophiebits sophiebits mentioned this issue Sep 25, 2015
Merged
sophiebits added a commit that referenced this issue Sep 25, 2015
@sophiebits
Merge pull request #4977 from spicyj/gh-4870
59466a0 
Actually don't store <input> until mount-ready
@STRML
Copy link
Contributor Author

STRML commented Sep 25, 2015

👍 Thanks!

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

@sophiebits sophiebits

Labels
None yet
Projects
None yet
Milestone
0.14
Development

No branches or pull requests
3 participants
@sophiebits @zpao @STRML