martyjs / marty

A Javascript library for state management in React applications
http://martyjs.org
MIT License
1.09k stars 76 forks source link

renderToString memory leak #335

Closed thecaddy closed 9 years ago

thecaddy commented 9 years ago

We are using the marty express middleware using koa and we noticed we were seeing a pretty severe memory leak and we narrowed it down to the renderToString method. https://github.com/martyjs/marty-express/blob/master/index.js

We narrowed it down further to the render function in the application context. https://github.com/martyjs/marty-lib/blob/master/modules/application/applicationContainer.js

We are still investigating to see if we can pinpoint the issue. Here is a dump of our memory using node-memwatch and it looks like every time we render on the server side that all the data and react that is rendered is being stored in memory.

{ before: { nodes: 336157, size_bytes: 47598312, size: '45.39 mb' },
  after: { nodes: 1673271, size_bytes: 132722624, size: '126.57 mb' },

  { what: 'Array',
    size_bytes: 22434376,
    size: '21.4 mb',
    '+': 419078,
    '-': 2494 }, 
 { what: 'Closure',
    size_bytes: 9829296,
    size: '9.37 mb',
    '+': 136753,
    '-': 235 },
  { what: 'ReactCompositeComponentWrapper',
    size_bytes: 5507528,
    size: '5.25 mb',
    '+': 52957,
    '-': 0 },
  { what: 'ReactDOMComponent',
    size_bytes: 4026912,
    size: '3.84 mb',
    '+': 41947,
    '-': 0 },
  { what: 'ReactDOMTextComponent',
    size_bytes: 123040,
    size: '120.16 kb',
    '+': 1538,
    '-': 0 },
  { what: 'ReactElement',
    size_bytes: 6812856,
    size: '6.5 mb',
    '+': 94624,
    '-': 1 },
  { what: 'system / Context',
    size_bytes: 9230736,
    size: '8.8 mb',
    '+': 129776,
    '-': 148 }
jhollingworth commented 9 years ago

Investigating

jhollingworth commented 9 years ago

I've created a branch of the chat example to try and replicate this issue which i'm running with node 0.12 and using memwatch-next to logging out heap stats. If I run while true; do curl http://127.0.0.1:5000/; done for a while it seems a lot of memory is being used although it does eventually get cleared.

current base: 59760792 ; estimated_base: 59760792 ; usage_trend: 0
current base: 217834864 ; estimated_base: 217834864 ; usage_trend: 0
current base: 595563440 ; estimated_base: 595563440 ; usage_trend: 0
current base: 38304768 ; estimated_base: 38304768 ; usage_trend: 0
current base: 123842328 ; estimated_base: 123842328 ; usage_trend: 0
current base: 396223208 ; estimated_base: 396223208 ; usage_trend: 0
current base: 760554800 ; estimated_base: 760554800 ; usage_trend: 0
current base: 45458816 ; estimated_base: 45458816 ; usage_trend: 0
current base: 117148952 ; estimated_base: 117148952 ; usage_trend: 0
current base: 387706512 ; estimated_base: 144204708 ; usage_trend: 0
current base: 751897880 ; estimated_base: 204974025 ; usage_trend: 2.8
current base: 942412424 ; estimated_base: 278717864 ; usage_trend: 6.6
current base: 45609824 ; estimated_base: 255407060 ; usage_trend: 4.3
current base: 119092240 ; estimated_base: 241775578 ; usage_trend: 2.5
current base: 391581752 ; estimated_base: 256756195 ; usage_trend: 4.3
current base: 731904968 ; estimated_base: 304271072 ; usage_trend: 10

Still not sure why this is happening though, will look a bit more

thecaddy commented 9 years ago

thanks for the quick reply. I will go take a more thorough look at the example and try switching back to node 0.12. I'm on the latest version of iojs currently. Were you seeing a similar trend with the heap diff as above? var hd = new memwatch.HeapDiff();

Attached is what it looks like on my servers.

screen shot 2015-06-07 at 8 04 31 am

thecaddy commented 9 years ago

Closing this as it turns out it was caused by a dangling event handler that we failed to unmount, sorry about that. It caused a massive leak within Marty, we are looking into ways to possibly add an error or warning in Marty to notify us when we make these mistakes.