Support destruction of instances and teardown of entire containers

[tschneidereit]

So here's the general idea:

Instead of supporting full teardown of individual object graphs, at least for now, Swiftsuspenders will only support destruction of individual instances and full teardown of entire containers.

Destruction of individual instances can be triggered in two different ways:

• unmapping of singletons, which invokes [PreDestroy] methods on the singleton
• invocation of injector.destroy(instance), which invokes [PreDestroy] methods on the given instance

Invoking injector.teardown iterates over a Dictionary holding references to all objects created by the injector and invokes their [PreDestroy] methods. This means that the order of instance destruction isn't necessarily guaranteed, though. It's mostly going to be insertion order, which seems fine, but AFAIK, there are some corner cases in the VM that cause that order to be broken. At the very least, the order isn't guaranteed.

The remaining open questions are whether the dictionary holding created objects should be weakly keyed or not and what exactly to hold in that dictionary.

The main argument against weak keys is that the container can't guarantee the orderly destruction of instances as they might just get collected. This means that neither their [PreDestroy] methods would be invoked nor the PRE_DESTROY and POST_DESTROY events dispatched.

The main argument for weak keys, on the other hand, is that keeping references to all created instances would create a serious leaking hazard as it would be pretty easy to forget destruction of instances.

A compromise could be to only hold references to instances containing [PreDestroy] methods and singletons (or, more generally, providers that register their provided instances in some way). The problem with that is that not only might it be hard to distinguish which instances will trigger PRE_DESTROY and POST_DESTROY events, it might also not fully alleviate the leaking hazard as developers might not realize that they're extending a class containing [PreDestroy] methods.

@darscan and @devboy, I'd be very interested in your feedback (and everybody else's, of course!) as you both indicated that this'd be an important feature for you.

[darscan]

Good stuff. I'm not sure I fully understand the "leaking hazard" of the strong key approach (which I prefer). Even if you forget to call teardown() (which we would do automatically in Robotlegs), the entire injector, along with those keys, should be collected once all references to the injector itself are gone. No?

[darscan]

And, if we went with the strong key approach, we could use an array instead of a dictionary. This would allow ordered teardown (which should be the order of instantiation reversed).

[tschneidereit]

The leaking hazard is that individual instances might live far longer than anticipated. Take commands in Robotlegs: Right now, you use and then forget about them, which means that they'll get collected. If the container holds strong references to every instance it ever creates, that means that you have to explicitly destroy every instance you want to have a shorter lifespan than the container itself. I honestly don't think that every user of Swiftsuspenders and Robotlegs is going to be that diligent (and wouldn't be too sure about myself in all cases, TBH)

[darscan]

Right, hadn't considered things like commands. Although in those instances, we can ensure that the extensions destroy them automatically. We can run through the RL codebase and ensure... um

Thought: how does this all relate to injectInto()? Does it only apply to instances created by the injector?

[tschneidereit]

The "um" part is what I'm worried about, yes ;)

Regarding injectInto(): it has to apply to instances running through that, too, I think. Otherwise, it's very confusing when exactly you can use [PreDestroy] and when you can't.

I think.

On the other hand, there's a nice symmetry in "the injector createth, the injector destroyeth" and "you create, you destroy".

[rolandzwaga]

Hey there,

always interested in topics such as this :) In Spring Actionscript 2.0 I'm using a similar approach, every singleton is being registered by an ObjectDestroyer (which is part of the context). Once the context gets destroyed (by calling its dispose()) method the ObjectDestroyer loops through all of its registered instances and performs its destruction logic. In Spring's case it checks for [PreDestroy] metadata, or if a destroyMethod is defined on the object definition that is associated with the instance. If an object definition is defined it will also loop through all of the explicitly defined properties and sets their values to null. (Just to help the GC a little).

I fully agree with your "the injector createth, the injector destroyeth" statement ;) Yet, this destroy mechanism is also invoked for each view that is removed from the stage, so I guess in my case it would be "the injector injecteth, the injector destroyeth" ;) Besides that, instances may also have been injected by passing them into the context's manage() method, I use this to enable injecting instances that were created server-side, for example, in that case its up to the developer to invoke the destroy() method at the appropriate time.

SwiftSuspenders 2 is looking pretty great! Congratulations!

cheers,

Roland

[darscan]

Roland!

so I guess in my case it would be "the injector injecteth, the injector destroyeth"

Agreed. There are too many cases where injectInto() is the only option. It should behave normally.

So, I reckon: strong (ordered) references to any objects with [PreDestroy] metadata.

or if a destroyMethod is defined on the object definition that is associated with the instance.

Heh, this leads to another discussion entirely: being able to externally define injections points etc for classes that don't have metadata or are otherwise sealed. External configuration FTW.

[rolandzwaga]

Shaun! ;)

"So, I reckon: strong (ordered) references to any objects with [PreDestroy] metadata." Well, that's the way I do it, I'm not saying that this is the best or only way ;) My container has a strong reference to each singleton it creates anyway (The InstanceCache takes care of that), since I need some kind of reference to them when injecting other instances with them. But its true that there might be edge cases where this could lead to a leak. I must admit, I haven't run into them yet, but that in no way means that these cases don't exist ;) Having some sort of finalization in the VM would be great though, so we could listen for an event that gets dispatched after no more references to the instances are present. I think I read somewhere that Actionscript 4.0 might have them, let us hope...

"Heh, this leads to another discussion entirely: being able to externally define injections points etc for classes that don't have metadata or are otherwise sealed. External configuration FTW."

That is indeed an interesting discussion. Right now, I favour using object definitions that are defined in an external configuration, because I feel that having [Inject] tags scattered throughout my source code is hard to keep track of after a while, especially if your projects tend to be very large. But, at the same time, having the metadata inside your class is also pretty nice when you're sifting through the source, otherwise you'll need to check the configuration every time you're wondering how an object gets configured by the IoC container. I do like the expressiveness of a fluent API that defines the injections, it looks nice and readable. But as always, once your project gets bigger and bigger, it gets harder to read. The XML and MXML configs of Spring have this same problem, I'm trying to manage this by supporting nested configurations, but at the end you really need to manage this as the user of an IoC container. (Obviously I recommend people to use multiple contexts, not one giant monolithic one).

cheers,

Roland

[tschneidereit]

@rolandzwaga thanks a lot for the input and the nice words! Getting the perspective of someone who's already implemented this stuff is very helpful indeed.

so I guess in my case it would be "the injector injecteth, the injector destroyeth"

Agreed. There are too many cases where injectInto() is the only option. It should behave normally.

Cool, so injectInto() will cause instances to be registered, too, thanks.

So, I reckon: strong (ordered) references to any objects with [PreDestroy] metadata.

I'm still not sure about the ordered part. The downside is that it makes destroying instances out of order much more expensive as they have to be removed from the array (or linked list, probably). Also, I'm curious about the use cases for ordered destruction. I can imagine circular dependencies between instances that are to be destroyed and all of that, but I can't really think of a case where such a dependency would be problematic for un- (or non-preditably-)ordered destruction.

But its true that there might be edge cases where this could lead to a leak. I must admit, I haven't run into them yet, but that in no way means that these cases don't exist ;)

So you're only supporting [PreDestroy] for singletons and views, where the latter get explicitly destroyed via calls from outside the container, then? My problem in Swiftsuspenders is that I want to support [PreDestroy] on instances that I wouldn't normally keep strong references to, not only singletons. I doubt that there's any leaking hazard with singletons alone. Otherwise, every IoC container would necessarily have that and I have tested the hell out of Swiftsuspenders to prevent them ;)

or if a destroyMethod is defined on the object definition that is associated with the instance.

Heh, this leads to another discussion entirely: being able to externally define injections points etc for classes that don't have metadata or are otherwise sealed. External configuration FTW

External configuration is fully supported through Injector#addTypeDescription already. Granted: The API could be nicer, but for that I'm actually hoping for some external contribution (hint hint).

Having some sort of finalization in the VM would be great though, so we could listen for an event that gets dispatched after no more references to the instances are present. I think I read somewhere that Actionscript 4.0 might have them, let us hope...

I seriously doubt that's going to happen. Not only would it require the VM to let the developer know about GCs being run (which in itself is highly problematic), it would also probably be pretty useless without some form of deterministic behavior regarding when an instance is collected to be useful. That, in turn, would restrict implementation choices that are currently completely internal to the VM. As an example, it would probably make it impossible to switch to incremental GCs later on (although that might already have happened in Tamarin, not sure).

[rolandzwaga]

"So you're only supporting [PreDestroy] for singletons and views, where the latter get explicitly destroyed via calls from outside the container, then?"

Well, [PreDestroy] is supported AUTOMATICALLY for singletons and views currently. It is possible for a non-singleton to be managed by the destroyer as well, but it would require manually registering the instance with the ObjectDestroyer, and calling its destroy() method, again, manually, when the object needs to be discarded. I haven't figured out a way to do this automatically, right now I don't think its possible to be honest. But I'd love to be proven wrong :)

By the way, as for the ORDERED part, when a Spring context is disposed all of the instances registered with the destroyer get destroyed in the order that the container created them. So, when using an external configuration, this is largely the order that the appear in the config. (There's also such a thing as lazy initializing singletons, so this isn't entirely true, hehe)

cheers,

Roland

[darscan]

@rolandzwaga btw, with swiftsuspenders everything is lazy - there is no eager instantiation, even for singletons.

Hmm.. perhaps order doesn't matter so much after all. I can't come up with a scenario where it would be a problem.

@tschneidereit Ah, will take a look at that TypeDescription. On first glance it's quite obvious that it's not designed to be user facing (order of operations matters, for example). Perhaps we just need a friendly TypeDescription builder. Anyway, I'll stop harping on about that then :)

[tschneidereit]

I haven't figured out a way to do this automatically, right now I don't think its possible to be honest. But I'd love to be proven wrong :)

Well, it is possible to automatically do proper teardown for all instances in case of container teardown. That requires strong references with all of their problems as described above, though.

Hmm.. perhaps order doesn't matter so much after all. I can't come up with a scenario where it would be a problem.

Cool, I'll implement it with a dictionary for now, then. Makes switching from weak to strong references easy can be replaced with a linked list later on.

@tschneidereit Ah, will take a look at that TypeDescription. On first glance it's quite obvious that it's not designed to be user facing (order of operations matters, for example). Perhaps we just need a friendly TypeDescription builder. Anyway, I'll stop harping on about that then :)

I just couldn't come up with a good API, TBH. If you can, please send a pull request :) If not, I think it's best left for higher-level stuff to deal with that.

Thank you so much to both of you for the great feedback - I think I've got a pretty clear picture of what to implement now.

[rolandzwaga]

Oh, I forgot by the way, when destroying an object that has an associated object definition and it nulls out the defined properties, the destroyer actually checks if the injected instance of a property is a non-singleton, in that case it also destroys that instance. (Since it is non-singleton I know that this injection is only used by that particular instance).

Interesting that everything is lazy in swiftsuspenders, Spring just ported the eager instantiation logic from Spring Java with the option for laziness, I guess this isn't a HUGE boon since none of the swiftsuspender users ever seemed to have asked for it.

cheers,

Roland

[rolandzwaga]

"Well, it is possible to automatically do proper teardown for all instances in case of container teardown. That requires strong references with all of their problems as described above, though."

Hm, indeed, this would mean I'd have to register all non-singletons with the destroyer as well, but in that case the non-singletons would stay around for the lifetime of the context, right? So, in Robotlegs' case where commands are created a lot, and those are supposed to be transients, this would result in all commands staying in memory for the life-time of the context, right? That doesn't seem desirable, or am I misunderstanding?

cheers,

Roland

[tschneidereit]

Oh, I forgot by the way, when destroying an object that has an associated object definition and it nulls out the defined properties, the destroyer actually checks if the injected instance of a property is a non-singleton, in that case it also destroys that instance. (Since it is non-singleton I know that this injection is only used by that particular instance).

I'll have to think about the nulling out thing and recursive destruction of properties of singletons. I see how that makes things like helpers easier to deal with, but I'm not sure about it.

I guess this isn't a HUGE boon since none of the swiftsuspender users ever seemed to have asked for it.

Well, @darscan did ;) You can use map().toValue() to achieve the same effect, though.

Hm, indeed, this would mean I'd have to register all non-singletons with the destroyer as well, but in that case the non-singletons would stay around for the lifetime of the context, right? So, in Robotlegs' case where commands are created a lot, and those are supposed to be transients, this would result in all commands staying in memory for the life-time of the context, right? That doesn't seem desirable, or am I misunderstanding?

Nope, you're not misunderstanding at all. That's precisely why I'm wary about this approach. For now, I'll check out how only registering singletons and objects with [PreDestroy] methods goes, but I might remove the latter.

[darscan]

Hm, indeed, this would mean I'd have to register all non-singletons with the destroyer as well, but in that case the non-singletons would stay around for the lifetime of the context, right?

Yup, that's the issue we were discussing right at the beginning of the thread. Not desirable at all. Which is why the idea is that only instances with [PreDestroy] are actually stored. Also, we'd make sure to manually destroy all transients in RL.

I guess this isn't a HUGE boon since none of the swiftsuspender users ever seemed to have asked for it.

Heh, except for me. Shunned I was, shunned ;)

[tschneidereit]

Heh, except for me. Shunned I was, shunned ;)

I wouldn't say shunned so much as ignored in the friendliest of ways!

[rolandzwaga]

"Yup, that's the issue we were discussing right at the beginning of the thread. Not desirable at all. Which is why the idea is that only instances with [PreDestroy] are actually stored. Also, we'd make sure to manually destroy all transients in RL."

good point here, in this case the library USING the IoC container becomes responsible for detecting the non-singletons, registering them and destroying them. So, would robotlegs make use of Spring, they could use the existing API's to make that happen. (By the way, I'm not lobbying for robotlegs to switch to Spring, don't worry Till hahaha)

So, IMHO, that should be the end conclusion, right? The IoC container is only responsible for singletons, any transients become the responsibility of the invoking code. Agreed?

"Heh, except for me. Shunned I was, shunned ;)"

Yes, but that has largely to do with your personal hygiene... ;)

cheers,

Roland

[darscan]

Shunned!

map().toValue()

That's not quite the same. For that to work you have to be able to provide the value - which you may not be able to, perhaps because its dependencies have not been mapped yet. Anyhoo, this is not that discussion :)

[tschneidereit]

Shunned!

map().toValue()

That's not quite the same. For that to work you have to be able to provide the value - which you may not be able to, perhaps because its dependencies have not been mapped yet. Anyhoo, this is not that discussion :)

It is not, no :) I have some concerns about the current mapping API, mostly caused by @alecmce bringing up an issue with it. That might create a place for that discussion after all ;)

[tschneidereit]

Fixed with commit 4f3ed475d14 and the half dozen or so preceding it.