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.

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

Memory Leak #131

Closed
GlennGeenen opened this issue Mar 19, 2018 · 5 comments
Closed

Memory Leak #131

GlennGeenen opened this issue Mar 19, 2018 · 5 comments

Comments

@GlennGeenen
Copy link

We use react-sizeme in a large application and resizeDetector stays in memory (it is a singleton). The result is that all react-sizeme children stay in memory and the retained memory from resizeDetector becomes larger and larger. We forked react-sizeme to create resizeDetector in the wrappedComponent so that resizeDetector is removed when the wrappedComponent is unmounted.

Do you have a reason to keep the resizeDetector as a singleton?

@ctrlplusb
Copy link
Owner

Hey @GlennGeenen, thanks for raising this!

Interesting that you are experiencing memory leaks. Do you think that the removeListeners are failing to be called for component unmounts?

I made it a "singleton" as I had assumed that having multiple instances of the resize detector may cause perf issues. Have you been able to test your modification against a large amount of tracked components?

@GlennGeenen
Copy link
Author

The problem is that as long as a react-sizeme component exists in memory it won't release the resize detector (what means in most cases it stays in memory all the time). In theory it's no problem to have resizeDetector in memory since it should remove all listeners. We tried several things to fix the element-resize-detector uninstall without any success. So the way we solved it is by just throwing away the resizeDetector in SizeAwareComponent as seen in: https://github.com/GlennGeenen/react-sizeme/blob/master/src/sizeMe.js

I can't say what the performance difference is (no complains yet) but we use our fix in production since the memory leak was 1MB per page visit. So I guess the memory leak is in element-resize-detector (I'll make an issue) but we have no clue how to fix it.

@ctrlplusb
Copy link
Owner

We've received a reply from @wnr on this. We should prefer uninstall rather than removeAllListeners. Will apply this recommendation asap. 👍

@ctrlplusb
Copy link
Owner

Released in v2.4.2, thanks for your help in identifying this issue! ❤️

@pleunv
Copy link

pleunv commented Apr 5, 2018

Just a quick heads up, this approach seems to be losing an element reference somewhere along the way. After upgrading from 2.4.1 to 2.4.2 I end up with 2 exceptions being thrown in https://github.com/wnr/element-resize-detector/blob/master/src/detection-strategy/scroll.js:

Uncaught TypeError: Cannot read property 'onExpand' of undefined
    at HTMLDivElement.onExpandScroll (scroll.js:397)
Uncaught TypeError: Cannot read property 'onShrink' of undefined
    at HTMLDivElement.onShrinkScroll (scroll.js:401)

I currently don't really have time to investigate further and went back to 2.4.1, if I find some time I'll dig a bit and file a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants