"Any object created by object A is object A's responsibility to destroy." + API #869

Closed
dcporter opened this Issue Nov 20, 2012 · 4 comments

Projects

None yet

2 participants

@dcporter
Member

There's an underabundance of attention paid in most SproutCore projects (NTM the framework itself) to the .destroy end of the object lifecycle. Object creation is well-understood, but destruction is generally assumed to take care of itself. GC in browsers is such that this is often true (many SC objects can be GC'd without being explicitly destroyed), but not often enough to prevent leakage.

The golden rule of object lifecycle management is that "Any object created by object A is object A's responsibility to destroy". There's no support whatsoever in SC for this, however, so any class that chooses to follow it has to implement its own tracking and destroying.

I'd like to implement a simple API in SC.Object (or mixed in there) for tracking object ownership and triggering destruction. The goal is to make it as simple as possible to obey the golden rule by providing an "ownership" concept whereby an object can declare that it created another object and will take responsibility for destroying it. Owned objects must also be aware of this, so that if they're destroyed at some other time, they're not improperly retained by the owner.

Here's an off-the-top-of-my-head API design:

  • property ownerObject: the object's owner. This should be set via the .create({}) hash if possible, or immediately afterwards via takeOwnershipOfObject.
  • method takeOwnershipOfObject(obj): receiver object takes ownership of obj. If obj is already owned by somebody else, an exception is thrown. (Changing owners would be bad.) This would add the object to an internal list of owned objects. You'd call it from the receiver when it created the object. If the object is going to live on the receiver as a key (e.g. ScrollView#horizontalScrollerView) then you can optionally pass in a key. When the owner object is destroyed, it destroys all owned objects.
  • property doesOwnObject(obj): returns boolean indicating whether the receiver is the owner of obj.
  • method destroyOwnedObject(obj): destroys obj, as long as it's owned by the receiver. Otherwise does nothing. You can call this any time you want to destroy an object you created (e.g. if you're swapping views in a ContainerView).
  • method ownedObjectWasDestroyed(obj): when an object is destroyed, it needs to alert its owner that it no longer needs tracking. This removes the owned object from internal ownership tracking.

Finally, #destroy (or #destroyMixin) would be updated to destroy all remaining owned objects, and to alert its owner that it's done for.

The three use case examples that I can think of off the top of my head include:

  • Every childView passed in as a class would be automatically destroyed, while childViews that arrived instantiated would be left alone. (Currently all children are destroyed, regardless.)
  • SC.ContainerView lets you pass in classes, which it will instantiate for you. At that point, it would call this.takeOwnershipOfObject. When swapping that view out for a new one, the container would just call this.destroyOwnedObject, which only destroys objects that it's supposed to. Finally, when the container is destroyed, no further effort would be required to ensure that its final nowShowing is also destroyed.
  • SC.ScrollView currently creates, but does not destroy, instances of ScrollerView and ContainerView. With SC.Ownable, it would take ownership of those views at create time, causing them to be automatically destroyed at destroy time. In rare circumstances, ScrollView doesn't create these views itself; in these cases, the views would be properly left intact.

(Objects would still be responsible for nulling out their own properties on destroy, e.g. ScrollView's container and scroller views. It would be easy to create a declarative API for this as well, but I'm on the fence about the need. Also, I think that once objects are destroying themselves properly, nulling out properties will be less important, as isolated groups of destroyed objects should be GC'd together.)

A) How ... readable is that-all?
B) Thoughts?
C) Is this better than just shouting about the golden rule and doing our best to follow it?

Thanks,
Dave

TL;DR: The golden rule of object lifecycles says "If object A creates object B, object A is in charge of making sure .destroy() is called on object B at some point". Bulleted above is an off-the-cuff API proposal introducing & supporting the concept of object ownership to make it easier to follow this rule.

@dcporter dcporter referenced this issue Nov 20, 2012
Closed

Memory Leaks #848

@publickeating
Member

I'm still getting up to speed on some of the implications of ownership, but this makes sense. I'd like it if the API was even simpler though. I think it will only be used by the internal controls and I don't know that the created object really needs back-references up to who created it.

I'm just thinking out loud here, but maybe:

object = this.createAsNeeded(item);  // creates the item if needed and if so, references it in internal "created objects" array.

// later

this.destroy(); // also calls destroy() on the objects in its internal "created objects" array

If you only take ownership by creating an object with this special method and since an object can only be created once, I think you can simplify the API down to one special method. For the case that someone else destroys the object you created, then if we attempt to call destroy on our created objects and find one already destroyed, we should log a Developer Warning and just continue (since it probably doesn't really hurt things for it to be destroyed already, but it's not good coding practice).

@dcporter
Member

Two scenarios drove the complexity of my API proposal. The first is when an owned object is created in the creation hash of the owner object:

SC.Object.create({ innerObj: SC.Object.create() }) // outer object should own inner object

The solution I came up with was to allow after-the-fact way of taking ownership. But even in that scenario, it requires some ugly, pervasive init code to do the ownership-taking, and certainly doesn't feel graceful. Would it be possible instead to have some auto-detection of the above scenario, and make ownership automatic in that case? I'm feeling very positive on that approach; are there any implications there that I'm missing?

Second was the destroyed-by-someone-else scenario that you addressed, but I'm worried about a very long-lived object creating myriad short-lived objects that end up destroyed somewhere else. These short-lived objects will be retained by the creator-owner-factory object's ownership list, and may cause a serious memory leak. I don't think it hurts to give owned objects a reference to their owner, and it helps in this scenario. As with childView / parentView, this reciprocal reference makes sense in any one-to-one or many-to-one relationship.

With an eye on the above, how about:

  • auto-ownership where any object created within the creation hash of another object is owned by that object.
  • auto-destruction where calling .destroy() on an object destroys all of its owned objects as well.
  • property ownerObject : the owner of an object. If passed in via create() then ownership is established at create time.
  • method createIfNeeded(objOrClass) : creates and returns objOrClass; if created, takes ownership. (Include hash argument for use in creating?)
  • method destroyOwnedObject(obj) : same as before.
  • method ownedObjectWasDestroyed(obj) : same as before.
@publickeating
Member

I think this was really just an issue with some poorly coded builtin views and too much property overloading (i.e. accepting classes, strings, instances for an example view). I believe that all of the cases of un-destroyed or overly-destroyed internal view instances have now been cleaned up with the addition of the createdByParent property in this commit: 08957b8

The long and short of it is, that any view that accepts classes or instances with the intention to add them as a childView needed a simplified way of tracking which kind it got, class or instance. Some views did this okay, some did it poorly and some did it not at all. In any case, the standard way is to use createChildView like always, but then to check the createdByParent property when removing child views (note that the parent may not be destroyed at the same time, it may just be swapping child views).

I don't think any additional API is required at this time. createChildView is essentially createIfNeeded from above and it seems like a lot of work/CPU cycles to try and prevent sloppy coding practices by inspecting each property of a hash on create to determine if it was already created or not (I'm actually not sure how this would work anyhow, because SC.Object doesn't create it's sub-objects automatically, that only happens with SC.View childViews — which is just createChildView again).

Closing for now! Thanks @dcporter

@dcporter
Member
dcporter commented Jun 3, 2013

👍 Sounds good. If most built-in views are doing their jobs now then a generalized solution has a lot less value for the effort & added complexity; meanwhile createdByParent sounds like a great lightweight solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment