search

rebus-org / Rebus

:bus: Simple and lean service bus implementation for .NET
https://mookid.dk/category/rebus
Other
2.31k stars 362 forks source link

Drop IContainerAdapter contract #422

Closed artganify closed 8 years ago

artganify commented 8 years ago

Short reason: It's not very nice. :)

Long reason:

Currently, the IContainerAdapter is used not only to provide a custom IoC container for message handler resolving, but in many cases also to register the IBus contract along with a static accessor for the current IMessageContext. This can be seen e.g. here:

First problem: While this is very convenient, it actually breaks the Register-Resolve-Release pattern which many modern dependency injection frameworks follow. For example, Autofac doesn't 'naturally' allow the registration of services within the container after an instance of IContainer has been created. The only way is to create a new instance of ContainerBuilder and update the underlying IComponentRegistry with it. Same for e.g. SimpleInjector, where the container first should be verified (and because of that, the SimpleInjector extension contains a FakeMessageContext for verification).

Second problem: Since the current IContainerAdapter expects the implementations to register the IBus instance within them on SetBus(), there is absolutely no way to control HOW the bus is going to be registered when using the default IoC container extensions. For example: I'm exposing the service bus through Autofac to various MEF components and because of that, I need to declare the registered dependency as export (using Autofac.Integrations.Mef). Just because of a single line during registration, I had to write my own IContainerAdapter. Or what if someone wants to provide additional metadata during registration? Or a custom lifecycle (okay, doesn't make sense for a service bus, but whatever..)?

My suggestion: Drop the IContainerAdapter contract completely. IoC container extensions can just implement IHandleActivator, because that's the only thing expected from them: To resolve/activate message handlers. In order to keep a convenient way to wire up everything, extension developers should be encouraged to e.g. provide a registration provider in a way the respective IoC container understands. E.g. for Autofac, it would be a IModule.

Example:

https://github.com/artganify/NicerRebusAutofac

What do you think?

gertjvr commented 8 years ago

+1

Other libraries have gone a similar route MediatR with single and multile handler factories.

We are maintaining a custom ContainerAdaptor for Rebus.Autofac which handles registration in the Autofac.Module and doesn't recreate the container on bus registration.

mookid8000 commented 8 years ago

Hi @artganify , thank you for your elaborate and opinionated piece :grin:

I agree with you that the RRR pattern is a little bit jumbled because Rebus requires you to pass one single reference in, which will then be used to a) register IBus/IMessageContext, and b) resolve handlers later on.

While the things you say make sense to me, I fear that changing these things would just upset other IoC container people.... one thing that I bet we can all agree on, is that many people who use an IoC container have strong opinions and are using their particular IoC container because of said strong opinions.

At the moment, I like Rebus' slightly quirky container adapter API because it is pretty consistent across all IoC containers, and the different adapters all provide (almost) the same guarantees with regards to bus and handler disposal.

If I were to change it at this point, I could maybe be talked into providing a way to opt-out of having the bus and message context automatically registered in the container adapter passed in. Something like this - (yeah, I know - it looks like crap :laughing:):

Configure.With(new AutofacContainerAdapter(container, registerBusAndStuff: false))
    .(...)

would provide a way to specify that the container adapter is only meant to be used for looking up handlers.

What do you think?

mookid8000 commented 8 years ago

Hi @artganify , did you have a chance to think about this?

artganify commented 8 years ago

Hello @mookid8000. Sorry for the late reply.

First off,

Configure.With(new AutofacContainerAdapter(container, registerBusAndStuff: false))

No. Please don't do that. It looks horrible. :D Also, since that would obviously depend on the IContainerAdapter implementation, it's not something which can be controlled by e.g. the Rebus configurator and thus results in the same as writing a custom implementation.. if anybody wants that.. for some reason,.. I don't know.

Well, let's go through the individual points:

I agree with you that the RRR pattern is a little bit jumbled because Rebus requires you to pass one single reference in, which will then be used to a) register IBus/IMessageContext, and b) resolve handlers later on.

This not really affects the RRR, if I understood that one correctly. At most, the IContainerAdapter breaks the Single Responsibility Principle or the Interface Segregation Principle because, as you said it, essentially the same contract is being used for wiring up the bus within the container and also resolving the handlers for a specific message type. But that's not a problem per se, the problem I was talking about is that most applications (especially business software) have some very fixed 'stages': At first, there's the composition root where every service and dependency is being registered within a container, then later on dependencies are being resolved and used. After they are not required anymore, they are being released (mostly controlled by their lifecycle.). Only in exceptional cases, e.g. if you use a plugin or composition framework, the initially built up composition root or container is changed - and that's why many IoC frameworks (independently of strong opinions, favorites e.g.) separate registration and resolving. Since you expect a fully configured IContainer to wire up the container, there is, as I mentioned: The container already built, the compositon root complete and most importantly: No way to control how the bus should be registered. And that's an actual problem, like also @gertjvr mentioned.

I bet we can all agree on, is that many people who use an IoC container have strong opinions.

Yes, fully agreed. But I don't think that has anything to do with the problem I described above. I worked with many IoC containers over the past years and while each has it's features and flaws to love or to hate, they all somewhat work the same way or provide similar interfaces.

At the moment, I like Rebus' slightly quirky container adapter API because it is pretty consistent across all IoC containers

That's only because Rebus is so cool and popular, of course there are many IoC container implementations and of course they are using the contract you provided. :D

If I were to change it at this point

Isn't there a Rebus2 in the making or pretty close to release? What other chance do you get to fix existing problems etc.? ;)

So, long story short: I too find the IContainerAdapter contract quirky and it made me, and other people write their own IHandlerActivator implementations and register the bus ourselves which I think is just unnecessary work for something so trivial. However since you really seem to like the way it is right now, just keep it. I'm going to publish my own extension packages for containers so everyone can decide for themselves. :)

artganify commented 8 years ago

And here we go:

https://github.com/artganify/RebusExtensions https://www.nuget.org/packages/Rebus.Extensions.Autofac/1.0.0

Would that be fine for you if such a package co-exists with the official one?

mookid8000 commented 8 years ago

Yes!! I love the fact that you are challenging the way I did the Autofac package, and having your package as an alternative "competing" package is a wonderful addition to the ecosystem :grin: :+1: :ok_hand:

mookid8000 commented 8 years ago

Your Topshelf integration looks nice too - I did a little bit of a run-up to building one myself, but I never got around to actually do anything. I think I'll take a look at your package instead :smile:

mookid8000 commented 8 years ago

I'll close this one for now - we can continue discussing things and possibly reopen if necessary :)

artganify commented 8 years ago

@mookid8000

Thank you for your kind words. My packages are not meant to be competing, just an alternative. I really love Rebus, it's one of the best service bus frameworks I (and my company) ever worked with, but it has some quirks which do not play nicely with existing environments sometimes. So I think there should be various ways to do things, hence the package. I was just worried you might not agree with this as people could confuse it for the official one and then blame Rebus if something goes wrong.

The Topshelf integration was something I wanted to do for a long time too. But I don't really know if it's that useful since the only real feature it provides is starting the bus together with the hosted service. However one also needs to access the bus for actually using it, so it can rely either on DI or the static accessor I decided to go with, which is honestly pretty nasty. So that definitely needs some rework.

I'm fine with closing the issue as for now. :)

NKnusperer commented 8 years ago

I'am struggle with the same problem while trying to use Microsoft.Extensions.DependencyInjection. When configuring services in the Startup of the application I can only register types, the container to resolve them is build later when out of scope. The system we use is very similar to the ASP.NET Core startup procedure but has nothing to do with web stuff, you might look at it to better understand the problem (Application Startup in ASP.NET). Currently on the only official way seems to use some third party IoC framework like Autofac which I don't really like to introduce just for this single use case.

The only thing I can image currently is to invent some kind of IHandlerActivatorFactory which gets the container injected and can be used to create a IHandlerActivator instance at runtime. However this way I still wouldn't able to inject IBus somewhere.

@mookid8000 any idea how to work around this issue? Also whats the deal with this static MessageContext.Current which is registered by every IoC container? Can this also be used when having multiple IBus instances?

mookid8000 commented 8 years ago

@NKnusperer I think this must be solvable somehow – I would love to build a Rebus.Microsoft.Extensions.DependencyInjection package that makes it really easy and convenient. If I get the time one of the following days, I will see what I can do.

Also whats the deal with this static MessageContext.Current which is registered by every IoC container? Can this also be used when having multiple IBus instances?

The Current property is just a static gateway to a message context that is constructed from the current transaction context. The transaction context is bound to the current thread in a way that flows correctly between threads, even when you await stuff. And yes – it works as it should no matter how many bus instances you have 😄

NKnusperer commented 8 years ago

Would be cool if you would find a solution, I'am currently stuck with this :unamused:

mookid8000 commented 8 years ago

@NKnusperer I have been looking a little bit at "Microsoft Extensions Dependency Injection" (cool name!) – if I understand it correctly, it resembles the "common service locator" thing that some projects used a couple of years ago.

If this is correct, the correct way to use Rebus with this abstraction in place, would be to simply bypass it completely – so e.g. if you are using Autofac, you would pull in Rebus.Autofac and configure it with your Autofac container, which you would then hand to ASP.NET Core.

MOREOVER – and I cannot stress this enough :) – I DO NOT recommend using the same IoC container for your web application controllers as you are using for your Rebus handlers!

While logically they may be part of the same application, they have completely different lifestyles (API controllers follow the web request, Rebus handlers follow the current message)... and since one of an IoC container's foremost responsibilities is that of managing the lifestyle of your application services' instances, it can quickly get messed up completely if you mix them together in one single container.

THEREFORE: I recommend you simply new up an additional Autofac container (or whatever you are using) and keep it in the background, remembering to dispose it when the application shuts down of course 😉

This will also make your life more fun when, in the future, you decide to scale out the processing of messages and deploy it independently of your web application.