dotnet / orleans

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

[2.0 RC2 ]recommends IServiceCollection.AddIncomingGrainCallFilter to restore Public #4205

Closed lfzm closed 6 years ago

lfzm commented 6 years ago

@ReubenBond

2.0 RC2 IServiceCollection.AddIncomingGrainCallFilter is recommended to restore Public, because IServiceCollection is required to intercept component configuration when developing components, but 2.0RC2 cannot use IServiceCollection when configuring AddIncomingGrainCallFilter.

jsteinich commented 6 years ago

The extension methods are now on ISiloHostBuilder: https://github.com/dotnet/orleans/issues/4184

lfzm commented 6 years ago

@jsteinich The ISiloHostBuilder extension method can not use IServiceCollection , such as an authorized filter, you need to inject the authorization service through IServiceCollection , if you can not use IServiceCollection it needs to be divided into two injection, I think it is not reasonable.

ReubenBond commented 6 years ago

@aqa510415008 the ISiloHostBuilder version should be able to do everything the IServiceCollection version can.

If you disagree, could you show some code to demonstrate?

lfzm commented 6 years ago

@ReubenBond
ISiloHostBuilder does not have IServiceCollection attribute. If ISiloHostBuilder needs to configure other components, it needs to call ISiloHostBuilder.ConfigureServices . This will increase the difficulty of development and the difficulty of beginners learning. It is recommended to use IServiceCollection.AddIncomingGrainCallFilter..

ReubenBond commented 6 years ago

How does the IServiceCollection overload help you with that? You can't (or should not) access services from the IServiceCollection after the container is built - you should be using IServiceProvider instead.

Here's an example of a filter which injects services from the container:

public class GrainCallFilterWithDependencies : IIncomingGrainCallFilter
{
    private readonly SerializationManager serializationManager;
    private readonly Silo silo;
    private readonly IGrainFactory grainFactory;

    public GrainCallFilterWithDependencies(SerializationManager serializationManager, Silo silo, IGrainFactory grainFactory)
    {
        this.serializationManager = serializationManager;
        this.silo = silo;
        this.grainFactory = grainFactory;
    }

    public Task Invoke(IGrainCallContext context)
    {
        if (string.Equals(context.Method.Name, nameof(IGrainCallFilterTestGrain.GetRequestContext)))
        {
            if (RequestContext.Get(GrainCallFilterTestConstants.Key) is string value)
            {
                RequestContext.Set(GrainCallFilterTestConstants.Key, value + '2');
            }
        }

        return context.Invoke();
    }
}

If you'd like, we can add some overloads so that we can support delegates which have dependencies, also, but they are not required.

lfzm commented 6 years ago

image @ReubenBond I am applying .net Mvc Authentication to Orleans. This is the configuration of Authentication. It needs to reference IServiceCollection, or there are other solutions.

ReubenBond commented 6 years ago

Why don't you just make the method work from the ISiloHostBuilder instead?

public static ISiloHostBuilder ConfigureAuthentication(this ISiloHostBuilder builder, Action<AuthenticationBuilder> configure)
{
  return builder.ConfigureServices(services =>
  {
    services.TryAddScoped<A, B>();
    /// ... 
    configure(new AuthenticationBuilder(services));
  });
}
lfzm commented 6 years ago

@ReubenBond This method needs to return AuthenticationBuilder , AuthenticationBuilder contains IServiceCollection

ReubenBond commented 6 years ago

@aqa510415008 the code I posted does not work for you? Just configure the AuthenticationBuilder in a delegate

siloBuilder.ConfigureAuthentication(authenticationBuilder =>
{
  authenticationBuilder.AddTwitter(...);
});
lfzm commented 6 years ago

@ReubenBond This process is very troublesome, IServiceCollection.AddIncomingGrainCallFilter is much more convenient, AddIncomingGrainCallFilter is processing business logic

lfzm commented 6 years ago

@ReubenBond Thank you, I accept your suggestion, but I have another problem, and the AuthenticationBuilder uses Options, which is troublesome to deal with.

ReubenBond commented 6 years ago

@aqa510415008 the AuthenticationBuilder uses options? Could you show me in more detail? Maybe message me on Gitter and we can discuss the issue: https://gitter.im/dotnet/orleans

lfzm commented 6 years ago

@ReubenBond IServiceCollection and IConfiguration are required to generate the AuthenticationBuilder. SiloHostBuilder.AddIncomingGrainCallFilter can solve the problem I am facing, but it is very troublesome and requires the use of many delegates. For beginners Orleans' settings are close to the best of .net MVC settings.

ReubenBond commented 6 years ago

@aqa510415008 you can get IConfiguration from the ISiloHostBuilder, but not from IServiceCollection - that is one of the reasons why I strongly recommend ISiloHostBuilder instead of IServiceCollection for extension methods.

lfzm commented 6 years ago

@ReubenBond I now configure Orleans like .net Mvc Startup so I'm used to using IServiceCollection. Orleans is also accustomed to using IServiceCollection for the first time, so I recommend using IServiceCollection

ReubenBond commented 6 years ago

I still don't understand why, @aqa510415008. If we have to choose between all APIs being on IServiceCollection and all APIs being on ISiloHostBuilder then we must choose ISiloHostBuilder because it has important functionality which is not present on IServiceProvider. Some extensions need that functionality, so they cannot live on IServiceCollection.

Also, some methods only apply to silos, but if they are on IServiceCollection then clients can access them too (which is confusing) and some methods behave differently depending on whether they are used on the client or silo, so we need to distinguish them.

SebastianStehle commented 6 years ago

One (minor) advantage of IServiceCollection is that you have access to IConfiguration

ReubenBond commented 6 years ago

@SebastianStehle IConfiguration is available on IServiceProvider, but it's only added to IServiceCollection just before the container is built.

Generally speaking, you cannot rely on IServiceCollection having anything unless you put it there (and even then, it might be overwritten or removed by a later modification)

SebastianStehle commented 6 years ago

It is available in the ConfigureServices delegate. This is what I meant.

But generally speaking I do not understand, why you use ISiloHostBuilder that much. Because I thought you want to use the HostBuilder from AspNetCore and also allow CoHosting. So that I can have AspNetCoreMvc, Orleans, WCF and GRPC in the same project (if necessary) and let them share the service container.

Almost all libraries for AspNetCore use something like this: https://github.com/IdentityServer/IdentityServer4/blob/f33688e2db3779bed8e798b83778a4dfe1d912d9/src/IdentityServer4/Configuration/DependencyInjection/IdentityServerServiceCollectionExtensions.cs

Of course you can migrate it, but its a major change for a 2.1 then.

ReubenBond commented 6 years ago

Ah, right, but ConfigureServices is available on ISiloHostBuilder. ConfigureServices takes a delegate Action<HostBuilderContext, IServiceCollection>. The HostBuilderContext there gives you access to the "host" IConfiguration as configured by ConfigureHostConfiguration(...) calls. Later, when the IServiceProvider is built, services can access the "app" IConfiguration as configured by ConfigureAppConfiguration(...) calls. The app configuration is based on the host configuration.

SebastianStehle commented 6 years ago

Yes, it is just a little bit "strange", e.g. when you have a filter that is dependent on a configuration you would probably register them in the right order (even though it does not matter from the technical perspective):

public static void AddMyFilter(this ISiloHostBuilder builder)
{
    builder.ConfigureServices((services, context) => 
    {
        services.Configure<MyOptions>(context.Configuration.Get("my"));
    });

    builder.AddIncomingGrainFilter<MyFilterThatNeedsMyOption>();
}

For me the big advantage of the IServiceCollection is the pattern, not the functionality. Therefore I think the usage should be as close to AspNetCore as possible.

ReubenBond commented 6 years ago

Our pattern is probably more in-line with the next iteration of ASP.NET configuration, based upon the generic host. https://github.com/aspnet/Hosting/blob/dev/samples/GenericWebHost/Program.cs https://github.com/aspnet/Hosting/issues/1163

lfzm commented 6 years ago

I support @SebastianStehle, @ReubenBond Maybe you are right, but IServiceCollection is a habit developed by AspNetCore, this can not be changed

ReubenBond commented 6 years ago

Read this issue: https://github.com/aspnet/Hosting/issues/1279#issuecomment-347258640

One comment says:

Note ASP.NET Core isn't moving to the generic host model until at least 3.0 as it will require several breaking changes for us.

Orleans has already made most of these changes, but ASP.NET has not made them yet. That is why the patterns feel different. Orleans is already using the pattern which ASP.NET will consider for ASP.NET Core 3.0 when they completely remove the Startup class.

lfzm commented 6 years ago

@ReubenBond Is orleans implement the Http protocol in this way? Or use Dotnetty?

lfzm commented 6 years ago

image

ReubenBond commented 6 years ago

One method:

builder.UseAdoNetClustering(optionsBuilder =>
{
    optionsBuilder.Configure((AdoNetClusteringSiloOptions options, IConfiguration cfg) =>
    {
        // do things with cfg and options
        cfg.GetSection("AdoNetClustering").Bind(options);
    });
});