Unnecessary call on method with PreDestroy metadata tag.

[rhalukongor]

Consider the following configuration:

    public function configure():void
    {
    mediatorMap.map(Stage).toMediator(StageResizeObserver);
    context.afterInitializing(init);
    }

    protected function init():void
    {
        mediatorMap.mediate(contextView.view.stage);
    }

Now if I have a method with PreDestroy metadata tag on my StageResizeObserver, it is called although my observer is not about to be destroyed. I've had a look at the stack trace and MediatorFactory line 135: injector.unmap seems to be the cause of the method call.

Robotlegs 2.0.0b4

[darscan]

Ouch... interesting. I'll have to chat with @tschneidereit about this.

[tschneidereit]

So the problem is that the MediatorFactory temporarily maps the Mediator class to a singleton and then unmaps it again. This causes Swiftsuspenders to invoke the [PreDestroy] method because, as far as it is concerned, removing the mapping ends the mapped value's life cycle.

The MediatorFactory could use type, not singleton, mappings to work around this. Ideally, I think preferably, these mappings should be permanent instead of temporary to reduce the overhead, too.

[darscan]

I wish it were that simple. The problem is that the mediator instance that is created needs to be available to any hooks that run in that block.. Hooks are supposed to be able to preprocess mediator instances. I haven't thought of any workable solutions for this. Stumped.

https://github.com/robotlegs/robotlegs-framework/blob/master/src/robotlegs/bender/extensions/mediatorMap/impl/MediatorFactory.as#L134

[tschneidereit]

Oh, of course. Hmm, have to ponder this some.

[darscan]

Thinking about this some more I'm not sure that unmapping should automatically invoke destroy (in terms of the Injector).

[tschneidereit]

I do see how that can be undesirable. On the other hand, unmapping does mean that the injector isn't able to handle the instance anymore, so for all it cares, it get destroyed.

Then again (need a third hand here!), unmapping doesn't especially cry "destroy this, burn it with fire!" so much as "forget about this, this isn't the mapping you're looking for". So yeah, this is mostly about what's most intuitive, and I honestly don't know.

[creynders]

Is there a particular reason you can't manually create the mediator instance and map it as a value? AFAIK, when unmapping a value no PreDestroy methods are called, right?

[tschneidereit]

Good idea: that'd be a solution to this particular problem, yes.

Still, the basic question remains whether unmapping should invoke [PreDestroy] or not.

[creynders]

My 2 cents: PreDestroy may be a bit misleading. The destruction of an instance is a possible consequence of unmapping, but there are exceptions, like mediators but also pinned instances. IMO, either the tag should be renamed or it should only be called when an instance is effectively no longer detained by the framework.

[creynders]

Thinking about this a bit more I actually find it pretty strange (in the current strategy) that when a value is unmapped the PreDestroy methods aren't called. I can imagine a valid use case with for example a service that has been mapped as a singleton and does some cleanup on PreDestroy, then some requirements change which lead to the service needing initial configuration and the developer decides to instantiate, configure and map the service as a value. If she's not very attentive she'll never realize that the PreDestroy method isn't called anymore.

So, I think what's most clear would be to let the dependencyproviders have the PreDestroy methods called upon umapping by default, but allowing for overriding that behaviour when necessary i.e. in RL with mediators and pinned instances. I think adding a autoDestroy parameter to Injector#unmap and isolating the code from SingletonProvider#destroy to a global function would be enough on the SwiftSuspenders side. On the RL side, only a few changes would be necessary: calling the global function when the mediator or pinned instance is about to be released for GC.

[rhalukongor]

:+1: creynders: I believe Swift Suspenders are a bit too "involved". Sure an autoDestroy would save the day; well a custom definable autoDestroyStrategy:String could be better, I think some of the Swift Suspenders' responsibilities should move to the frameworks.

I remember thinking about writing extensions for closing the bridge with the Swiz (no offence; but I like the persistent controllers and the eventHandler meta tag). I dropped the idea right after finding out that S.S. did not provide a describeTypeCache: Calling describeType twice is expensive. S.S. also has missing features.

To conclude, I believe a rethink on the design of S.S. is necessary, it could be split into two: A core framework with responsibilities moved to the frameworks and the missing stuff, and a superset for people who likes it as it is now.

[darscan]

I've committed a potential fix for this. It should also speed things up for the common case. Leaving open until fix confirmed by @rhalukongor

[darscan]

Closing for now. Re-open if you find any problems.