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

ObserveChanges plugin causing memory leak #4400

Closed
jeremy-smith-maco opened this issue Jul 13, 2017 · 14 comments

Comments

@jeremy-smith-maco
Copy link
Contributor

commented Jul 13, 2017

Description

Our Handsontable had a memory leak for some reason and couldn't work out why. Looking at the memory profile we noticed that the afterLoadData hook was being fired even though we didn't specify it in the options. We noticed it was being added by the ObserveChanges plugin but we don't have it enabled and is supposed to be disabled be default. We explicitly disabled it in our table options and fixed our memory issue. Not sure exactly what was causing it but something to do with that plugin. We added it to a jsfiddle example and it caused it to leak memory too.

Steps to reproduce

  1. Add observeChanges: true to your table options
  2. Recursively load data
  3. Leak memory

Demo

http://jsfiddle.net/Lp4qn55v/745/

Your environment

  • Handsontable version: 1.2.0
  • Browser Name and version: Google Chrome Version 59.0.3071.115
  • Operating System: Windows 10
@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

Hi @jeremy-smith-maco
Have you got sorting in your project? ObserveChanges is fired when you enable sorting.

@jeremy-smith-maco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2017

Yes, we have sorting in our project. We have now turned off observeChanges plugin.

@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

Sorting may not be working as expected without observeChanges.

Here https://handsontable.com/blog/articles/how-to-build-a-custom-version-of-handsontable is a table with dependencies. You can check which plugins are needed to run features.

@jeremy-smith-maco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2017

In the fiddle provided above (haven't tested our project yet) the sorting works the same with and without the observeChanges plugin. So doesn't seem to affect it unless it affects it non-visually?

@jeremy-smith-maco

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2017

Everything seems to be working fine in our project too.

@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

Hm... actually this is an interesting case as it looks like the changes made by one of our devs may have an impact on the sorting plugin. Thanks for spotting that!

`inform #8874, #8958,' '# 1454'

@jeremy-smith-maco

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2018

I have looked more into this issue. It seems that the issue is in the JSON patch library that is being used. The library does a comparison of two objects on two lines here and here which will always evaluate to false unless they are the exact same object reference used. Each time it evaluates to false in the getMirror() function, it will create a new Mirror object and add it to the local array of Mirrors. Over time this builds up and eventually the browser runs out of memory.

This comparison should be changed to use a different method of object comparison. One suggestion is just to stringify both objects when comparing them (i.e. JSON.stringify(obj1) === JSON.stringify(obj2)). There are some limitations to this method such as functions or DOM nodes within the objects get ignored during comparison and the variables must be in order in the objects otherwise this will fail comparison. This is a simple solution which I will put a PR up for but if the developers want some other method of comparison then that is fine but this method will work and stop memory leaks in most cases unless their object variables are out of order.

Another solution would be to manually cleanup the Mirror objects array after a certain amount of time or after a certain amount of Mirror objects are in the array or something.

EDIT: Implementing my proposed solution above seems to break a lot of tests. It seems like it should be comparing object references but there still needs to be some solution for it leaking memory. At least the cause is narrowed down to the array of Mirrors which is this line. What is the point of keeping the Mirror objects around in that array? Why can you not just delete the Mirror object when it has 0 observers in its array? We have implemented my above solution in our local project but it can still cause memory issues if the data is changed a lot as each data change would create a new Mirror object and store it in that array.

@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

Thank you for your effort, Jeremy. I am not an expert in this field but it looks like a reasonable explanation for this bug. Would it be too much to ask for a pull request?

I have asked our Support developer to take a look at this issue but he will be able to do it no sooner than on 15th of Jan.

jeremy-smith-maco added a commit to jeremy-smith-maco/handsontable that referenced this issue Jan 11, 2018
@jeremy-smith-maco

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2018

I have added a PR with a suggested solution. Developers may have a different solution but this worked for our project at least.

@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

Thank you @jeremy-smith-maco

jeremy-smith-maco added a commit to jeremy-smith-maco/handsontable that referenced this issue Jan 15, 2018
@dragoshzava

This comment has been minimized.

Copy link

commented Jan 18, 2018

Hey, we have run into similar issues with thousands of rows and multiple tables.
This fixed our memory leak issue as well.
Thanks @jeremy-smith-maco !

jeremy-smith-maco added a commit to jeremy-smith-maco/handsontable that referenced this issue Jan 28, 2018

@wojciechczerniak wojciechczerniak modified the milestones: Backlog, April 2018 Mar 12, 2018

@dragoshzava

This comment has been minimized.

Copy link

commented Apr 5, 2018

Hello, we have found another issue with this memory leak.

We have created an api using puppeteer (Puppeteer is a Node library which provides a high-level API to control headless Chrome or Chromium over the DevTools Protocol. It can also be configured to use full (non-headless) Chrome or Chromium.) which allows end users to export their tables in different formats as png/pdf.

The issue is that Chrome crashes when handsontable loads because the plugin ObserveChanges creates a big amount of listeners for huge data (above 1000 rows).

It is reproducible on linux/ubuntu machines 100%.
If you are interested i can provide an example.

Just an FYI, if the handsontable has ObserveChanges: False, everything works fine.

@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2018

Thank you for sharing the details @dragoshzava

It looks like this issue is scheduled to be fixed in April. We will test your scenario as well.

@wojciechczerniak wojciechczerniak modified the milestones: April 2018, May 2018 Apr 5, 2018

@wojciechczerniak wojciechczerniak removed this from the May 2018 milestone May 14, 2018

@wojciechczerniak wojciechczerniak added this to the June 2018 milestone May 14, 2018

@swistach swistach modified the milestone: June 2018 Jun 5, 2018

@jansiegel jansiegel assigned jansiegel and unassigned jansiegel Oct 9, 2018

@pnowak pnowak self-assigned this Nov 7, 2018

pnowak added a commit that referenced this issue Nov 9, 2018
pnowak added a commit that referenced this issue Nov 16, 2018
swistach added a commit that referenced this issue Nov 22, 2018
Issue #4400 Fixed ObserveChanges plugin memory leak (#4710)
* Issue #4400 Fixed ObserveChanges plugin memory leak

* Issue #4400 Added check to see if the observers in the mirror is zero

* Issue #4400 Fixed if block formatting
@AMBudnik

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

I'm happy to see a memory leak fixed. The fix is published in the newest version 6.2.1
Thank you for your continuous help @jeremy-smith-maco

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.