mhinze / ShortBus

In-process mediator with low-friction API
MIT License
210 stars 41 forks source link

Providing greater control over DI lifetime scope #17

Closed jrnail23 closed 10 years ago

jrnail23 commented 10 years ago

Changed AutofacDependencyResolver ctor to take ILifetimeScope instead of IContainer, providing greater flexibility, as IContainer can only be used at the root level in Autofac. Also, injected IDependencyResolver into Mediator ctor, to allow mediator to resolve dependencies from various lifetime scopes, as determined by consuming devs.

jrnail23 commented 10 years ago

Sure, I originally intended to duplicate the "BasicExample" tests, using Autofac, instead of StructureMap, but then I hit a bit of a wall on one part. It seems that a couple of those tests resolve contra-/co-variant handlers (I can never keep those straight -- in this case, the example is supposed to resolve "Ping" as the handler for "PingALing"), and I'm currently in the dark on getting Autofac to do the same. I'll dig in and see if I can get that working, and I'll add those examples as soon as I can.

UPDATE: to get Autofac to support variant handlers, I had to register a ContravariantRegistrationSource (this was new for me):

builder.RegisterSource(new ContravariantRegistrationSource());
zachariahyoung commented 10 years ago

I had already converted my "BasicExample" to autofac and also had issues with the "PingALing" test. I had know idea that was the issue.

Also why are you setting AutofacDependencyResolver and Mediator to InstancePerLifetimeScope? Or at least how did you know these should use InstancePerLifetimeScope instead of the default?

jrnail23 commented 10 years ago

@zachariahyoung, check out this gist for exploratory tests to demonstrate (if you grab my pull request or my fork, paste it into there and they should run as expected).

BTW, it did show me that you can safely register the Mediator and IDependencyRespolver as the default (instance per dependency). The only catch is that IDependencyResolver needs to be registered with a lifetime scope equal to or broader than the Mediator's scope.

I set them up to InstancePerLifetimeScope primarily to demonstrate that it could be done -- my initial objective was to provide greater control over the scope that ShortBus uses to resolve message handlers and their dependencies.

That having been said, my initial objective stems from two issues I saw with default/previous behavior of ShortBus & ShortBus.Autofac: using DependencyResolver as a Singleton via static dependency, as well as the use of Autofac.IContainer in AutofacDependencyResolver.

This had a couple of implications:

  1. Any dependencies resolved from DependencyResolver.Current would be resolved from the root/global scope instead of any active current lifetime scope, since Autofac.IContainer can only represent the root/global lifetime scope (IContainer was probably the wrong abstraction to use for ShortBus.Autofac – ILifetimeScope or IComponentContext would have been more appropriate).

    When using Autofac in an ASP.Net MVC app, for example, you might typically register a dependency (such as an NHibernate ISession) as InstancePerHttpRequest under the Autofac.MVC integration package. All that actually does is declare that the dependency will be scoped such that only one instance will exist per specific lifetime scope tagged as “AutofacWebRequest.” At the beginning of each HttpRequest, Autofac.MVC begins a new lifetime scope named “AutofacWebRequest” (effectively a child container), from which any requested dependencies will be resolved.

    The problem comes when we try to resolve one of those InstancePerHttpRequest dependencies from the DependencyResolver (which uses the root container, as opposed to the AutofacWebRequest lifetime scope). Importantly, the root scope doesn’t really know anything about the AutofacWebRequest lifetime, so Autofac will throw an exception telling us that the AutofacWebRequest lifetime scope could not be found. On the other hand, if we were to resolve such dependencies from the appropriate lifetime scope, we’d be fine.

  2. The fact that DependencyResolver was set up as a Singleton/static dependency means that in order to resolve dependencies from a contextual lifetime scope, we’d actually have to swap out the dependency resolver per scope with a new instance containing the appropriate scope. In an application that uses multiple threads (such as an ASP.net MVC app), that could be a pretty dangerous proposition.

So to wrap up, the changes I made were intended to prevent the mediator from being bound to that Singleton scope/global state. Effectively, the mediator (and it’s dependency resolver) went from Singleton scoped to instance per lifetime scope (or whichever scope you choose to configure).

God that was a lot of typing... I hope that makes sense at all.

mhinze commented 10 years ago

Autofac is exotic

jrnail23 commented 10 years ago

It can get pretty interesting, that's for sure