Forums

You're right, the destruction phase of a component's lifecycle is important to clean things up to avoid memory leaks.

The most common thing to clean up is event subscriptions that the component has made on other components, whether those be DOM nodes or custom components like widgets and models. Any events a component subscribes to on itself (i.e., this.after('fooChange', ...);) will be automatically cleaned up when calling a Y.Base-based class' `destroy()` method.

What is not automatically cleaned up are things like event subscriptions on other components or internal data objects. A class like Y.ModelList uses both of these things and therefore its `destructor` has been implemented such that will clean these things up. You can use ModelList as a guide for what to clean up, and how to do it.

ModelList's `destructor()` method simply calls it's `_clear()` method:
https://github.com/yui/yui3/blob/master ... st.js#L202

The `_clear()` method will empty out all of the list's internal data storage, and detach all of its models from itself:
https://github.com/yui/yui3/blob/master ... st.js#L974

The `_detachList()` method will remove the list as an event bubble target of each model:
https://github.com/yui/yui3/blob/master ... t.js#L1007

ModelList does all this to break references to these other objects which it normally holds. This way garbage collector can do its job when the developer's code is no longer holding onto a reference to the list.

I usually write my destructor immediately after my initializer so that whenever I create something I write the code to dispose of it right below.

If I create an object, I destroy it. The owner of the object is responsible of its destruction. The only exception is if the object is shared with some other object, then, it all depends on who owns it and if the object is equally shared, no one can safely destroy it on its own. Some means usually have to be provided to coordinate this.

Widget tries to destroy all events of elements within its bounding box if destroy is called with a true arguments, but this is not the default as it is expensive. It will get rid of those listed in _UI_ATTRS and UI_EVENTS, which you would rarely touch and take no time to deal with. As for the rest, it is better to detach everything you attached, it is faster and it is clearer. And if you attached something to some element outside of the bounding Box, you simply have to, there is no way it can figure that out.

I usually have an empty array into which I push the handles of all the events I create, whenever, wherever they are created (that is, the return value of each on(), after() or delegate() call). Then, in the destructor, I simply loop through this array calling detach on each handle.

I do agree with Mark: I wished there was some documentation around that explains this topic thoroughly.

Still one of Marc's questions makes me wonder. What when you have an array that is filled with other arrays, or with objects that contains arrays? Is emptying the 'master'-array enough to prevent memoryleaks? Or do we need to loop through that as well? (would be great with complex structures).

Regards,
Marco.

Marco:

If those arrays contain objects that have destructor methods, you should call those destructors, but you don't need to delete the items in the array itself.

Juan:

Good point and I'm afraid the code I looked at seems not to be thorough in its cleanup. As far as I can tell, the Widget destructor does call the remove method on the bounding box, which will call the destroy method of that node, however, it does not set the 'recurse' argument to true so that it doesn't actually go all the way into the widget contents.

This is somewhat scary since in complex widgets you don't have real control over how many Node instances might have been created, for example, in a DataTable, if the cell has been clicked, a Node instance might have been created if the developer asks for it, but that is quite off the control of DataTable itself so it cannot really know how many might have been created.

If this is so, we have a big memory leak in our hands. I am sure, or at least I really hope, that I missed something and things are cleared up somewhere that I missed. I have to go now, but I'll do some tests later on.

Anyway, perhaps a static lazy Node cache purge might be a good idea. Some async task that could be launched and go through the Node cache and check if their entries still point to valid nodes, a few nodes at a time.

@Juan,

Consider this ticket http://yuilibrary.com/projects/yui3/ticket/2532640.

In my opinion, widgets structure should be changed slightly:

Within widget._destroyBox() there should be a declaration:
deep = this._destroyAllNodes ? this._destroyAllNodes : this.DESTROY_ALLNODES;

(where DESTROY_ALLNODES=true; <-- standard)

Thus 'recurse' argument is standard set to true. Only if those who don't want to suffer the performance-penalty could set it off by making widgetinstance.DESTROY_ALLNODES=false. But they need to make sure they clean up their Node-instances themselves.

At the moment, most of us who don't realize suffer a posible memoryleak of created Node-instances.

Marco.