dotnet / orleans

Cloud Native application framework for .NET
https://docs.microsoft.com/dotnet/orleans
MIT License
10.07k stars 2.03k forks source link

Register IStreamProviderManager in DI automatically #2236

Closed DixonDs closed 7 years ago

DixonDs commented 8 years ago

Similarly, as it was done for IGrainFactory in https://github.com/dotnet/orleans/pull/2192 , it would be very useful to have IStreamProviderManager injected as well.

xiazen commented 8 years ago

Yes we can do the same with IStreamProviderManager in the Silo side to make it injectable.

Just a heads up, system types injectable is not supported in client side yet. Just to make sure your use case does not require IStreamProviderManager injectable in client side too.

Can you tell me more about your use case on making IStreamProviderManager injectable?

And some extra information here in case you are curious: The reason why DI is not supported in client side yet is that GrainClient is a static type, we need to make it a non-static first in order to continue in improve DI in client side.

DixonDs commented 8 years ago

I have some shared classes used from both client and silo sides. In those shared classes I want to publish some messages to streams (usually indicating that some data is changed)

As for IStreamProviderManager or other system types not injectable from the client side, it is ok so far until I have an easy way to get the instance of that system type from static GrainClient meaning that I can easily do registrations myself in the client composition root.

jason-bragg commented 8 years ago

One concern with injecting IStreamProviderManager via DI is that this would allow it to be injected into classes in any context, when stream providers only function within the context of a grain or grain client.

xiazen commented 8 years ago

Yes that's what I was going to ask, by

shared classes

what do you refer to ? Maybe give us more details.

Grain and GrainClient already have access to IStreamProvider, you don't need to access them through DI.

DixonDs commented 8 years ago

I am not sure what confusion could be here. Of course, if the class injecting IStreamProviderManager is tried to be constructed from the context where IStreamProviderManager is not available, it will give some kind of exception and that is perfectly fine in this case.

Ok let me make up some example. Say I have ConfigurationService class used for storing and retrieving system-wide settings. Once ConfigurationService.SetSetting method is called I want to publish the new setting value to the dedicated stream, so that all interesting parties could subscribe to this stream and receive updates. ConfigurationService could be called from both grain client and silo. So I want to have some abstraction injected to ConfigurationService allowing to get the stream provider by name first and then the respective stream from the stream provider regardless where ConfigurationService is called from.

Grain and GrainClient already have access to IStreamProvider, you don't need to access them through DI.

I am not sure I understand this argument. It is like you are saying that you need to use DI only when you don't have access to all implementations. Is that what you mean? In any case I really don't have access to IStreamProviderManager, it is currently not accessible from GrainClient and Grain descendants.

xiazen commented 8 years ago

Sorry I think I confused you by not bring in enough context.

I don't have full context with your use case. So I was just explaining the facts I know. Stream providers only function in the context of a Grain or a GrainClient. So you can inject your StreamProvider to your ConfigurationService through Grain or GrainClient. We restrict access to StreamProvider from only Grain and GrainClient so that users wouldn't inject StreamProvider from outside those two context. If we provide user access to StreamProvider through DI, that might confuse users and they would think it is legit to use StreamProvider without those two contexts. We are just being extra careful there. Maybe we should. I don't hold a 100% opinion here that we shouldn't do that.

I totally didn't mean

you need to use DI only when you don't have access to all implementations

In your ConfigurationService example, you can get stream provider by name through a Grain or a GrainClient by calling GetStreamProviders or GetStreamProvider(string name) so I don't understand why you need to inject StreamProviderManager.

Hope this helps the confusion here.

DixonDs commented 8 years ago

A couple of points:

  1. How injecting of IStreamProvider or IStreamProviderManager is different from injecting IGrainFactory which cannot be used as well outside of those two contexts?
  2. In my example I want ConfigurationService to control what stream provider should be used, not Grain or GrainClient. Otherwise, I will have to pass the same stream provider name in all places where ConfigurationService is used.
  3. Generally, I would prefer to have single composition root in the application and do not inject dependencies in runtime. If I need to use GetStreamProvider method of Grain to get the stream provider I need, I will need to do it in each and every grain using hypothetical ConfigurationService. In case of IGrainFactory, before https://github.com/dotnet/orleans/pull/2192 was merged, I did it using custom IBootstrapProvider, as I mentioned in https://github.com/dotnet/orleans/issues/2128. In case of IStreamProvider or IStreamProviderManager, I cannot do that since it is not accessible in IBootstrapProvider.Init method.
jason-bragg commented 8 years ago

How injecting of IStreamProvider or IStreamProviderManager is different from injecting IGrainFactory which cannot be used as well outside of those two contexts?

This is a fair point. We are still figuring out if/what/how framework capabilities should work with DI, and the scoping of IGrainFactory was simply overlooked with that change. I'm not saying we would not have exposed the grain factory via DI if we had recognized this problem, only that the fact that we didn't recognize it then does not mean we should ignore it now. It is one of the issues we need to think through and understand. If you have suggestions for how we can expose capabilities via DI only where they are supported that would help resolve this concern for not only this system, but many other framework components with similar issues.

Please understand that when issues are raised about a request, it's not us pushing back on the request as much as identifying what problems need to be addressed to move forward.

Having said that, there are some differences between IGrainFactory and IStreamProviderManager.

As with the other concerns, these are not reasons why we should not make this change as much as issues we'd need to address to make the change in a responsible way. As the requester of this change, I hope that you will consider these issues and help us solve them.

DixonDs commented 7 years ago

@jason-bragg What if we have some implementation of IStreamProviderManager throwing an exception in every method if called from not accessible context or when the streams infrastructure is not initialized yet? In normal cases this implementation will just redirect to the current implementation of IStreamProviderManager. And this implementation is what we will inject through DI.

What are your thoughts?

jason-bragg commented 7 years ago

@ReubenBond has a PR that inadvertently addresses this. see "Reduce usage of stateful statics by leveraging dependency injection #2345 "

The PR does not address many of the issues I raised in this thread, so I'd not suspect that it is a final solution. With that in mind, I'd suggest you not directly access IStreamProviderManager in your application code, but instead access your own abstraction that protects the bulk of your application layer from any future changes we're likely to make in this area. For instance, if your systems used a IStreamProviderFactory something like:

public interface IStreamProviderFactory
{
    IStreamProvider Create(string name);
}

with an implementation that used the IStreamProviderManager to 'create' the stream providers by name, any future changes we make to the handling of IStreamProviderManager could be addressed in this implementation rather than in every system that uses streams.

Your system though :)

jbragg

sergeybykov commented 7 years ago

Partly resolved via #2345.