ElectNewt / Distribt

Other
166 stars 44 forks source link

Avoid use BuildServiceProvider in startup code #41

Open scorrao opened 1 month ago

scorrao commented 1 month ago

In ServiceBus.cs at Distribt.Shared.Setup call to BuildServiceProvider to access a list of handlers an create IMessageHandlerRegistry. https://github.com/ElectNewt/Distribt/blob/471077d783cdbed5a9bf34be1e172f0e4912d693/src/Shared/Distribt.Shared.Setup/Services/ServiceBus.cs#L69-L71

Call to BuildServiceProvider make problems with singletons services.

Calling BuildServiceProvider multiple times can cause serious trouble, because each call to BuildServiceProvider results in a new container instance with its own cache. This means that registrations that are expected to have the Singleton lifestyle, suddenly are created more than once. This is a problem called Ambiguous Lifestyle. https://stackoverflow.com/a/66264937

Microsoft has a warning related https://learn.microsoft.com/en-us/aspnet/core/diagnostics/asp0000?view=aspnetcore-8.0

I changed the constructor of MessageHandlerRegistry to able to get the necessaries services from ServiceProvider

-    public MessageHandlerRegistry(IEnumerable<IMessageHandler> messageHandlers)
+    public MessageHandlerRegistry(IServiceProvider serviceProvider)
    {
-        _messageHandlers = messageHandlers;
+        using (var scope =  serviceProvider.CreateScope())
+            _messageHandlers = scope.ServiceProvider.GetServices<IMessageHandler>();
    }

This change require change also multiple extension class to fix the dependency injection. What do you think? i don't known if it can impact in another feature

ElectNewt commented 1 month ago

hey sorry, I did not saw this. What you're saying makes sense, so yes, you can change it if you want, if not I will do it (but not sure when) .

There might be more examples where buildserviceprovider is being used, it will be good to have a look where and create tickets for those too. hopefully they are in startup methods and the change is simple.