mhinze / ShortBus

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

Make ShortBus compatible with existing usages of other IoC containers #2

Closed jrnail23 closed 10 years ago

jrnail23 commented 10 years ago

I'm kicking myself for not finding ShortBus sooner -- I went and wrote mostly the same thing for my current project, although I use Autofac.

In my implementation, I abstracted away from Autofac by encapsulating the handler lookup into an IMessageHandlerResolver (catchy name, right?).

It looks like it would be relatively trivial to do something similar here.

I'd be happy to take that on, if you think it would improve the project.

mhinze commented 10 years ago

Sorry to be late in replying to this, I was somehow not watching this project.

Thanks for the good feedback, we appreciate that.

As for the other IOC containers.. SURE, that would be awesome. I am hoping we can make it as simple or simpler as what they do in MVC:

ShortBus.DependencyResolver.SetResolver(ObjectFactory.GetInstance, ObjectFactory.GetAllInstances);
dvins commented 10 years ago

+1

jrnail23 commented 10 years ago

Thanks for incorporating this into the project! I just pulled down the nugets though, and I'm a little concerned with the usage of IContainer (Autofac) rather than ILifetimeScope. My concern is that when you use IContainer, I believe you're effectively bound to resolving dependencies from the root lifetime scope, instead of whatever your current lifetime scope may be. That potentially opens the door for resource leaks in the form of dependencies intended to be scoped to the current lifetime scope (typically a child scope, like scope per http request, etc.), yet effectively ending up being used as a singleton (since the root lifetime scope lasts the duration of the application). Does this sound like a valid concern, or am I missing something in the implementation that handles this sort of thing?

mhinze commented 10 years ago

Sounds like a valid concern. We can change our implementation or allow consumers who choose a more fine grained lifetime scope to implement IDependencyResolver themselves. I will not be making the change, as much as I owe it to myself to check it out, I haven't used Autofac beyond reading the docs.

On Fri, Dec 27, 2013 at 7:11 AM, jrnail23 notifications@github.com wrote:

Thanks for incorporating this into the project! I just pulled down the nugets though, and I'm a little concerned with the usage of IContainer (Autofac) rather than ILifetimeScope. My concern is that when you use IContainer, I believe you're effectively bound to resolving dependencies from the root lifetime scope, instead of whatever your current lifetime scope may be. That potentially opens the door for resource leaks in the form of dependencies intended to be scoped to the current lifetime scope (typically a child scope, like scope per http request, etc.), yet effectively ending up being used as a singleton (since the root lifetime scope lasts the duration of the application). Does this sound like a valid concern, or am I missing something in the implementation that handles this sort of thing?

— Reply to this email directly or view it on GitHubhttps://github.com/mhinze/ShortBus/issues/2#issuecomment-31259768 .

kmc059000 commented 10 years ago

@jrnail23 concerns are correct. In my implementation at work, we use ILifetimeScope instead of IContainer, and I agree that the change should be made.

jrnail23 commented 10 years ago

My first swipe at a workaround involved injecting a Func into a custom resolver, like this:

    public class AutofacDependencyResolver2 : IDependencyResolver
    {
        private readonly Func<ILifetimeScope> _scope;

        public AutofacDependencyResolver2(Func<ILifetimeScope> scope)
        {
            _scope = scope;
        }

        public object GetInstance(Type type)
        {
            return _scope().Resolve(type);
        }

        public IEnumerable<T> GetInstances<T>()
        {
            return _scope().Resolve<IEnumerable<T>>();
        }
    }

I think this will handle it properly, but I'm not 100% certain. I'll exercise it a bit and report back.

jrnail23 commented 10 years ago

It looks like this won't do the job, since the lifetime scope will always resolve to the root container. I'm not sure how to get it to work otherwise, mainly due to Mediator's static dependency on the DependencyResolver. I don't really see a hook or seam that will allow the resolver to use a specific lifetime scope.

Are you guys totally married to the static DependencyResolver? If Mediator had IDependencyResolver injected into its constructor, lifetime scope management wouldn't be a problem, as it could receive a new resolver instance with the appropriate scope every time it was created.

I understand the current use of the Ambient Context pattern, but it just seems like this aspect of Mediator is very much at the heart of its responsibilities, rather than a cross-cutting concern (which is where Ambient Context works best, IMO).

Your thoughts?

mhinze commented 10 years ago

Not married to it, Mediator used to have a constructor dependency on StructureMap's IContainer.

jrnail23 commented 10 years ago

I can put it in there if you like... I was thinking that if you do see some value in the static resolver, that could be set as a default, in an empty ctor overload (non-breaking change)

mhinze commented 10 years ago

Eh, it's convenient, I suppose, but we resolve the Mediator anyway, so it's not an issue for us.

On Fri, Dec 27, 2013 at 12:31 PM, jrnail23 notifications@github.com wrote:

I can put it in there if you like... I was thinking that if you do see some value in the static resolver, that could be set as a default, in an empty ctor overload (non-breaking change)

— Reply to this email directly or view it on GitHubhttps://github.com/mhinze/ShortBus/issues/2#issuecomment-31274167 .