Stores and Actions cannot be removed from memory

[vladimir-rovensky]

I was profiling our application and it seems like there's a potential memory leak in Reflux - the Keep object holds references to all the created Stores and Actions (createdStores, createdActions). I don't think there's any official way to remove these references, which causes a leak if your stores aren't singletons.

I know that as per the official Flux rules, stores should be singletons, but this rule is not exactly written in stone. Would it be possible to provide official API to release stores and actions completely? If not, I think it should at least be mentioned in the docs.

[devinivy]

It's true that reflux-core keeps a reference to actions and stores. In 99.9% of cases this wont be an issue, because apps typically keep their actions and stores for life, and do not create arbitrarily many actions and stores.

I don't think we'll add a way to explicitly "release" stores and actions due to the awkwardness of having that option, and the potential issues that could arise. That said, I don't think there's any reason why the "keep" can't be a weak map rather than an object to allow for those actions/stores to be garbage-collected. Anyone know of a good cross-browser weak map?

[itkach]

Anyone know of a good cross-browser weak map?

Looks like weak references have to be supported natively. ES6 defines WeakMap and modern browsers appear to mostly implement it.

It would be nice, though, to have explicit destroyStore/destroyAction to complement createStore/createAction. Ability to create stores dynamically is useful in certain applications and it's awkward to do it otherwise.

[vladimir-rovensky]

I agree with the above, especially since it's now possible to create a store as an ES6 class. Having a class implies you can create multiple instances, it should be possible to destroy them as well. As far as I know, it should be possible to implement a WeakMap non-naitvely (e.g. babel-polyfill does this).

I'm still not convinced that a WeakMap is the best option though. Even with the weak map, you'd still need to manually unsubscribe from all the actions that the store is listening on. If would be nice if there was an official API to do this, as well as completely destroy the store.

[FlorianWendelborn]

I think I'm currently experiencing this with server rendering. Any idea on how to work around this? It's a huge memory leak and nearly 99% of my heap references Keep.js.

[vladimir-rovensky]

I ended up having to remove the references manually, in a pretty hackish way: let destroyStore = function(store) { _.remove(Reflux.__keep.createdStores, s => s === store); };

I'm not aware of any better option ATM, please let me know if you find one.

[FlorianWendelborn]

@vladimir-rovensky thanks for sharing. Did you use this in server-rendering too? Where/when did you call that function?

[BryanGrezeszak]

@dodekeract and @vladimir-rovensky - Let me know how that code turn out for you in your projects. If it work well as a solution then we can look into getting them implemented as an easy API feature.

I planned to move to this once I had some other front-end things better fixed up, but if you guys solve this before I even get there then all the better.

[FlorianWendelborn]

@BryanGrezeszak can you explain why this is even necessary? Why don't the stores clear when I use server-rendering? If this really is the reason for the memory leak I'm seeing then reflux is IMHO unusable/broken for server-rendering.

[vladimir-rovensky]

We don't use server rendering, so I can't help you there unfortunately. I use this to get rid of those stores that aren't singletons, in other words stores that have multiple instances that come and go. I pretty much use the method in place of a destructor/dispose() of the store. I believe this should be in the official API, preferably as some sort of dispose() method on the Store class. After all, being able to create an instance of a class and not being able to properly dispose of it feels incomplete.

[BryanGrezeszak]

@dodekeract

Honestly, I have no idea. That side of reflux-core is not something I've delved deep into.

Based on my reading in this issue, I'm assuming that the problem is the Keep.js file in reflux-core.

At first glance I admit I don't see why the Keep.js needs to exist. It would seem to me that as long as the user has references alive to any possible store/action it is unneeded since they wouldn't garbage collect anyway, and if the user doesn't have references to them alive, then it's unneeded because there's no reason to keep them from garbage collecting.

As far as I can tell this commit is where it was inserted, and at the time it seems like it was for addressing this issue.

In my eyes it's seeming like this issue here is bigger than the desire for some introspection. And reflux-core survived a long time without this file, so I don't think it's needed.

REQUEST for both of you:

Keep.js also has a reset method that clears its arrays. If one (or both) of you don't mind, could you test out something like just calling Reflux.__keep.reset() on a 5 second interval in your program (sorta nullifying the Keep.js file) just to see what our real world effects are? Does it truly solve the issue? Are there any side-effects? If it solves it without side-effect then that probably means that the best foot forward probably is to just kill Keep.js altogether.

[vladimir-rovensky]

Based on a short testing, running window.setInterval(() => { Reflux.__keep.reset(); }, 3000); has no side effects at all, everything works fine as expected. I also run Reflux.__keep.reset(); before running our unit tests to prevent leaks in watch mode, again with no issues.

I was under the impression the the Keep was there for diagnostics and such, like if someone wants to write a Chrome extension showing all the registered stores for instance. I can't think of a scenario wherein a store could be garbage collected once the Keep is removed, the user code still has to have a reference to the Store somewhere.

[BryanGrezeszak]

@vladimir-rovensky - And for your implementation the leaks are gone too?

Yeah, I was off base on the garbage collection thing. All I could think of for a reason was an improper attempt at "keeping" instead of garbage collecting. Once I dug deeper I found it was for diagnostic type stuff.

That said, wanting some diagnostic data is NOT worth a giant memory leak. Especially not as a default way of operating.

I'm thinking maybe the proper thing to do would be something within reflux-core where it only "keeps" stuff when a variable to do so has specifically been turned on. Otherwise it does not.

[spoike]

You are correct @vladimir-rovensky that the purpose for Keep is for diagnostics/debugging, which was my original intention (to tap into actions and stores that have been created by the client since reflux-core didn't keep them in any context by itself).

Unfortunately I never got to create a Chrome extension and it was done without any thought about server-side rendering, so I can imagine that this is a confusing source of memory leak woes (unless you do reset).

[vladimir-rovensky]

Yes, clearing the Keep did solve the memory issues for us. I think either way is fine, whether you choose to remove the Keep, or add some API to remove a store from it, both will do the trick.

[BryanGrezeszak]

@spoike - thanks for the input.

I think that settles it. It's a good feature...but probably not as a default. Rather than keeping by default and forcing you to use reset, we should probably NOT keep by default, unless the dev flicks on some type of var to tell it to keep.

[FlorianWendelborn]

@BryanGrezeszak I'll hopefully find time to test this later today. If it either doesn't impact or resolves my memory issues - can we just remove it (at least per default)?

[BryanGrezeszak]

@dodekeract - I'm working on reflux-core right now to make it NOT store anything by default. As of how I'm doing it right now, you'd have to call Reflux.__keep.useKeep() before Keep.js would actually start storing references to anything. If you don't specifically tell it to then it'll act as if Keep.js doesn't even exist.

[FlorianWendelborn]

@BryanGrezeszak unfortunately

setInterval(() => {
    Reflux.__keep.reset();
    console.info('resetting keep');
}, 3000);

didn't get rid of the memory leak. When stresstesting it still grows indefinitely. :(

Is it possible that the issue I'm seeing is related to the usage of ES6 stores as classes?

[BryanGrezeszak]

@dodekeract - well, that should mean that whatever leak you're having it's not actually related to Keep.js, but I thought you said that in your stack it keeps referencing Keep.js as what's holding all the extra? There's just 2 arrays in there and reset clears them out.

Is most of your heap still referencing Keep.js?

[BryanGrezeszak]

@vladimir-rovensky and @dodekeract - I've updated the repo to the new changes in reflux-core. (not npm or bower yet, though).

If possible could both of you try out the new code in the repo.

@vladimir-rovensky - it'll probably fix your issue, since doing reset was fixing it. @dodekeract - if resetting didn't fix it then I doubt this will...but then that means that your issue is different from his and we should probably open a different issue for it. If you don't mind it still would be helpful for you to try the new repo code and see what it does for you.

Gonna wait to publish npm/bower until I get feedback from at least one of you, just because it's good to know its not breaking something other than the stuff I've tested it on.

[FlorianWendelborn]

@BryanGrezeszak I tested the new code via git and it seems to resolve a part of the memory leak. All the references to keep.js are gone. In addition to that everything works in the codebase I'm working on.

[vladimir-rovensky]

I briefly tried the new version, everything seems fine. Thanks guys!

[BryanGrezeszak]

Sweet! Thanks so much for helping me out with that guys! Glad to know it solved it for one, and at least made it better for another.

Later today I'll make it an official release with npm/bower and such, and I'll let you both know when that's done so that you can update your projects.

[BryanGrezeszak]

@vladimir-rovensky and @dodekeract - v5.0.2 with these changes is now published to repo, npm, and bower.

[twgraham]

@BryanGrezeszak will these changes work for those using the ES6 api's? I'm trying to write a bunch of unit tests for my stores now and I noticed that they are persisting between tests using the Reflux.initStore() method (as expected). I need to be able to clean up these references between tests though. I tried the steps listed above with keep to no avail 😞

For the meantime I've thrown a reset method on all my stores to set it back to the original state... I guess I'd prefer if I didn't have to do this because I'm lazy 😆

[BryanGrezeszak]

@twgraham - these changes are the same for either API.

However, that's not what this particular issue changed.

The ability to easily clear all stores is something that's on my radar for dealing with server usage, and will be happening soon. Feel free to make an issue, or if not I'm going to be making an issue for it myself soon anyway.