Memory Leaks

[teddyzeenny]

Hello :) While investigating memory leak(s) in our tests, I came across a couple in this repo.

• ResultCollection is not getting cleaned up when another collection is created here. Every time the validations run, the previous result collection is not collected. I suspect this may be an Ember core issue where object and array proxies don't get cleaned up (related issue).

• This Validations variable survives across app resets/destruction (which means it survives between tests) because it's used by the ValidationsMixin whose lifetime is the same as the containing module's lifetime The issue is that to create Validations we pass a reference of the first instance that needs it. This causes the instance that triggered the creation of the Validations class to survive even after the application is destroyed. If that instance was created by the container, it has a reference to that container, which means the container doesn't get cleaned up, and since the container has a reference to the application and all singletons, the application and most of its instances don't get cleaned up. So at the end of every test, we'll have an extra application in memory. With enough tests we were quickly reaching Gigabytes of memory.

• We are also keeping a reference of the first instance that uses the validations mixin in the validation rules object whose lifetime is the module's lifetime and not the application's lifetime. This also keeps the model instance and its container and application alive. Cloning v here before assigning model to it may be enough.

I hope this is helpful, and thank you for this great addon!

[offirgolan]

Hi @teddyzeenny thank you for compiling this!

This Validations variable survives across app resets/destruction (which means it survives between tests) because it's used by the ValidationsMixin whose lifetime is the same as the containing module's lifetime The issue is that to create Validations we pass a reference of the first instance that needs it.

Is there a specific reason this is causing you issues? This was actually done on purpose since the mixin should be only created once per model class (not instance).

We are also keeping a reference of the first instance that uses the validations mixin in the validation rules object whose lifetime is the module's lifetime and not the application's lifetime.

Im not following you here. Each validator that is created has a reference to the model it is associated with it.

[teddyzeenny]

Is there a specific reason this is causing you issues? This was actually done on purpose since the mixin should be only created once per model class (not instance).

Correct, the mixin should only be created per class, however, it's keeping a reference to the first instance that uses it (notice how this is passed - this referring to the model instance, not class). As a result, the mixin (which is created once per class/module and never garbage collected - rightfully so) is keeping a reference to an instance, which should be (but isn't) garbage collected when the app is destroyed. TL;DR the first instance that uses the mixin can no longer get garbage collected ever, even if you destroy the Ember application. And since that instance is container-created, it has a reference to the entire container, which means any app using ember-cp-validations never gets garbage collected after destruction (can be verified by taking a memory snapshot after running a test or destroying an app that uses the validations mixin).

Im not following you here. Each validator that is created has a reference to the model it is associated with it.

Which makes sense, but unfortunately that validator is also referenced by the validations mixin / which prevents instances from ever getting garbage collected after they have been destroyed.

Both issues come down to one problem. The validations mixin is not (and shouldn't be) garbage collected after an app is destroyed. And that makes perfect sense since the mixin is per-class and therefore needs to survive beyond an Ember application instance. The problem is that this mixin is keeping a reference to the instances that use it which prevents these instances from getting garbage collected after destruction. Since these instances also contains references to the container and in turn the application, entire Ember applications never get garbage collected after at least one instance uses the validations mixin,

[offirgolan]

@teddyzeenny if you follow the model trail it leads here.

The validations class never actually stores the reference to the model instance, only uses it to get access to the owner + some other meta data about the instance type. Im not certain that the memory leak is coming from there unless there is something else that is holding it which I am not aware of.

Either that or I'm missing something trivial here 🤔

[teddyzeenny]

The closure here of the long living ValidationsClass prevents the model argument above it from being GCed.

[simonihmig]

I was hunting for a memory leak, and was also identifying the validations mixin as the culprit. I then found this issue here, and I believe it is the same that @teddyzeenny tracked down quite nicely!

Here is what I have found:

In the heap dump I was tracking down the retainer path of the Container object, and as you can see some ember-cp-validations stuff was at least part of it. The validations mixin is used here on a component (frontend/pods/components/invitation-form/component), and its validate() method, which lives on its prototype and thus is not GCed when the application is destroyed, holds a clojure reference to the validations and its model (which is the component). So AFAICT this matches what @teddyzeenny described: as long as there is a reference to the model from the prototype, the model cannot be GCed, and that also prevents GC of its owner (the container) and all of its referenced objects (controllers, services, components etc.).

I double checked this by removing the validation mixin from the component, and indeed in the following heap snapshot the Container does not show up anymore, i.e. it gets GCed properly!

[simonihmig]

@offirgolan I played around with your PR #568 quite a while. Posting this here to keep the thread together...

So, it does seem to fix the issue described above. With the change I don't see the retainer path from the screenshot above anymore!

That's the good news. The bad news is that I still see a not cleaned up container instance in the heap snapshot:

The retainer path show some references to Meta objects. AFAIK they are used by Ember for CPs, right?

I don't see a clear reference to ember-cp-validations here, however when I again remove the validations mixin from the object used in this test run, the container is GCed correctly! 🤔

[simonihmig]

I might try to create a minimal reproduction, to make sure it's not something particular with my app here...

[teddyzeenny]

@simonihmig I wrote a very quick hack a few months ago that removes all the memory leaks. The fix is definitely not production level so I couldn't PR it here, but I had to fix it quickly as I was on a tight deadline and we literally couldn't run our tests. This is the commit and this is the branch. This branch, 100% confirmed, does not leak between tests, so you may find it useful to see if the issue is from your app or, if the leaks are gone with my branch, from the addon.

[simonihmig]

@teddyzeenny thanks a lot! 👍

It did indeed fix the issue! I still see the issue depicted in the 2nd screenshot in other parts of the app (running other tests), but that might be unrelated to e-cp-v then.

With your branch at least the test case I have been investigating here is running for the first time with all validations enabled, and no leak (at least no container instance)!

[offirgolan]

I might try to create a minimal reproduction, to make sure it's not something particular with my app here...

@simonihmig if you can share something I can easily work off of, I can start tackling this issue.

@teddyzeenny thanks for sharing the commit. I'll see what parts of it I can use / get some inspiration from.

[simonihmig]

@offirgolan here is a reproduction: https://github.com/simonihmig/e-cp-v-memleak

I added some information how to reproduce in the Readme. Also worth noting:

• This is using Ember 2.18. Strangely enough I was getting the Container leak always (also when not using e-cp-v at all) when using Ember 3.0! 😳
• Using @teddyzeenny's branch, the leak is fixed. This matches with what I experienced in my actual app...

[offirgolan]

@simonihmig thanks for taking the time to put that together. I'll take a closer look soon when I have some more time on my plate.

[ming-codes]

I have another attempt here to fix the leak. https://github.com/ming-codes/ember-cp-validations/tree/bugfix/memleak

Here's the diff https://github.com/offirgolan/ember-cp-validations/compare/v3.5.3...ming-codes:bugfix/memleak