dotnet / orleans

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

Support Microsoft.Extensions.Hosting.Abstractions #4702

Closed ReubenBond closed 5 years ago

ReubenBond commented 6 years ago

In Orleans 2.0, we added SiloHostBuilder, which was designed to align with IHostBuilder from the Microsoft.Extensions.Hosting.Abstractions package (which wasn't yet released).

The intention was that SiloHostBuilder would be temporary and we would add support for the generic host once it became available.

It's available now, so we can consider adding support for it.

ASP.NET folks have a similar issue opened for WebHostBuilder: https://github.com/aspnet/Hosting/issues/1421. They've scheduled it for 3.0.0, so I imagine it will be a year or more before they move over.

I have a PoC for this on a branch here: https://github.com/dotnet/orleans/compare/master...ReubenBond:feature-generic-host

cc @galvesribeiro & @attilah who asked for this today

ReubenBond commented 6 years ago

There is an open question about API style:

Top-level builder

Attach all configuration methods to IHostBuilder directly.

hostBuilder
  .AddOrleans()
  .UseLocalhostClustering()
  .AddMemoryGrainStorage("MemoryStore");

Pro: It's more obvious that there is a single DI container Con: Extension methods don't have a clear target (is UseLocalhostClustering for Orleans or for SignalR? - less obvious)

Sub-builder

Create a new type, IOrleansBuilder, which has an IHostBuilder property.

hostBuilder.AddOrleans(builder =>
  builder
    .UseLocalhostClustering()
    .AddMemoryGrainStorage("MemoryStore"));

Pro: It's clear that those methods belong to Orleans and they will appear via intellisense in the correct context. Con: It's not clear that there is a single DI container being used, so changes to services made by the AddOrleans configuration delegate will affect the shared container. Eg, if the user thinks that they can configure logging differently for Orleans and ASP.NET Core then they are mistaken.

As a potential workaround for that con, we can create a new builder type which makes the relationship to IHostBuilder more explicit:

public interface IOrleansBuilder
{
    IHostBuilder HostBuilder { get; }
}

This can then be implemented explicitly to hide HostBuilder.

public class OrleansBuilder : IOrleansBuilder
{
    private readonly IHostBuilder builder;
    public OrleansBuilder(IHostBuilder builder) => this.builder = builder;
    IHostBuilder IOrleansBuilder.HostBuilder => this.builder;
}

That helps to hide the IHostBuilder so that end-users aren't tempted to call Build() from within the AddOrleans delegate or to try configuring logging separately for Orleans and another hosted service.

At the same time, extension writers see IHostBuilder clearly and understand that there is only one IHostBuilder, so the pattern and implications ought to be more clear to them.

Extension methods will look like this:

public static class OrleansBuilderExtensions
{
    public static TBuilder ConfigureThingy<TBuilder>(
      this TBuilder builder,
      Action<OptionsBuilder<ThingyOptions>> configure) where TBuilder : IOrleansBuilder
    {
        // Do something with HostBuilder, like configure services.
        return builder;
    }
}

Then we can expose the OrleansBuilder class (not the IOrleansBuilder interface) in the AddOrleans delegate:

public static IHostBuilder AddOrleans(
  this IHostBuilder hostBuilder,
  Action<OrleansBuilder> configure)
{
    configure(new OrleansBuilder(hostBuilder));
    return hostBuilder;
}

This way, the end-user can use extension methods defined against IOrleansBuilder without having easy access to methods on IHostBuilder such as ConfigureServices, ConfigureServiceProvider, and Build.


I believe that the proposed pattern with the sub-builder presents a better user experience to the top-level builder. We could consider similar changes for the client builder, but I don't have a well-formed opinion on that yet.

Suggestions welcome 😊

xref: https://github.com/aspnet/Hosting/issues/1279 cc @jdom

jdom commented 6 years ago

My vote is for option 2 (sub builder). We talked and tried many things with option 1 and variants of 2, but we concluded that configuring in the context of an Orleans builder is very important. There are already a few things that work in that way in the Microsoft.Extensions namespace, such as when you configure Logging or use .AddOptions(), etc, those builders actually end up configuring the one shared container, even though that Intellisense isn't entirely obvious that it's not a new instance of the Logging infrastructure, etc. This is where familiarity plays in our favor, and hopefully future frameworks will implement sub-builders too (I'd double check with @davidfowl now that there is a closer roadmap to integrate ASP.NET with the generic host).

Also, all the temporary hosting work we did today in Orleans 2.0 would make that option straightforward for our users to move to (albeit some minor tweaking of 1 or 2 lines of code in their setup). In fact, the team agreed to remove all the duplicate extension methods that configure IServiceCollection directly and just kept the ones on ISiloHostBuilder particularly to make the transition to option 2 easier on our users, since we deemed the sub builder context as very important even back then. We need to make sure that our sub builder no longer has extension methods to configure generic cross-cutting concerns such as logging (unless of course it's an Orleans-specific customization).

I'm not too convinced about hiding IHostBuilder on the sub-builder though. I haven't formed an opinion one way or the other, but at least initially I'm not seeing the confusion/harm about being transparent about it. If they happen to call .Build() on it, and then they try to continue to configure it, then it will be immediately obvious that it doesn't work.

SebastianStehle commented 6 years ago

Some examples for the syntax of variant 1: MVC Authorization Identity IdentityServer

But variant 1 would just return the subbuilder

jason-bragg commented 6 years ago

While my views on this are still quite malleable, atm, I also advocate a sub-builder. I see an Orleans silo as a hosted service, and would prefer to configure it as such using an ISiloBuilder. The move to IHostBuilder should not necessarily deprecate ISiloHostBuilder, as silo host builder can be maintained as a façade over IHostBuilder to support the common configuration pattern of a host with a single Orleans silo.

ReubenBond commented 6 years ago

@SebastianStehle the thing is, ASP.NET is in a transition and is still evolving. Those patterns were created for the Startup Class configuration model, which predates the Generic Host work. ASP.NET folks have said in the past that they want to get rid of that model since it has problems. For example it requires a temporary container and doesn't play well with hosting (eg, how do you pass context from Service Fabric to it?).

We deprecated the Startup Class with the 2.0 transition and are already on a model very similar to the Generic Host. Now we need to consider what the .NET ecosystem is going to look like once others make the transition.

The issue with AddX() returning IXBuilder is that it breaks continuity with the chaining style used everywhere else. Some methods return the original builder for chaining, some return a different builder, some methods accept a configuration delegate for configuration and others don't. That's not consistent.

The sub-builder model is more consistent.

Still, it's important to align with whatever the rest of the ecosystem is moving towards, so I'm fine with whatever that ends up being.

SebastianStehle commented 6 years ago

Thx for the long explanation. Was not aware of that

attilah commented 6 years ago

Generally the sub-builder is a better way to go IMHO. In real world that builder could be a no-op, without a Build method just a marker interface to stuff the extension methods on, a'la Logging and MVCs builder.

For non-breaking change I'd suggest to make the method public which registers the internal Orleans things and have that on IServiceCollection instead of ISiloHostBuilder.

This has multiple positive outcomes:

Extensions are also affected, did not check (because we've a lot), but I suspect that none or at least not to many of them registering internal classes in extension methods attached to the ISiloHostBuilder interface, if they're a modification also needed there. Which again would not be breaking anyone.

Regarding the opening up of that internal method: It could be moved into an Internal namespace within the current one, but be public. ASP.NET team also follows this pattern for things that can change in the future.

It would be good to see it in 2.1.

ReubenBond commented 6 years ago

If we put methods on IServiceCollection instead of IHostBuilder/ISiloHostBuilder, then we cannot access HostBuilderContext, which gives us configuration/environment/etc.

davidfowl commented 5 years ago

We've also landed on the sub builder design. It's time to wake this thread up! 😄

ReubenBond commented 5 years ago

Thanks, @davidfowl - I think we're on the same page.

To enable this earlier than 3.0, we can follow this plan:

For 3.0, we can clean up:

This would be a breaking change, but I think the end result is better than having two entirely separate interfaces. There's only minor ease-of-upgrade benefit in keeping a separate SiloHostBuilder post-3.0. I'm in favor of unification.

The key interface:

public interface ISiloBuilder
{
  IDictionary<string, object> Properties { get; }

  ISiloBuilder ConfigureServices(Action<HostBuilderContext, IServiceCollection> configureDelegate);
}

Both methods just forward their counterparts on IHostBuilder, the interface exists largely for intellisense purposes and to hide invalid methods like Build().


As an alternative to removing ISiloHostBuilder, we could make it extend ISiloBuilder this would mean changing all extension methods from:

ISiloBuilder AddMyProvider(this ISiloBuilder builder, ...)

to

T AddMyProvider<T>(this T builder, ...) where T : ISiloBuilder

so that users don't lose the ability to write

var silo = new SiloHostBuilder().AddMyProvider(...).Build();

(since otherwise AddMyProvider would return ISiloBuilder which has no Build() method)

It might be worth going with this approach so that we can support adding a silo to the host builder in its own, separate container in the future, which would likely involve adding a new interface derived from ISiloBuilder.

galvesribeiro commented 5 years ago

Working on a PR aligned to @ReubenBond description...

davidfowl commented 5 years ago

cc @glennc

SebastianStehle commented 5 years ago

Can you not keep the ISiloHostBuilder, so that extension projects do not need an update to support GenericHost?

galvesribeiro commented 5 years ago

@SebastianStehle better to break then extensions than break the runtime directly IMHO

SebastianStehle commented 5 years ago

Yes, I had a second look to this interface and I also do not see and option to keep it.

ReubenBond commented 5 years ago

I assume that this proposal works for everyone. @galvesribeiro's PR looks good to me

matthewgrimagig commented 5 years ago

Using the existing ISiloHostBuilder, I make use of UseServiceProviderFactory, however this does not seem to be possible when using the UseOrleans extension method. Am I missing something please?

galvesribeiro commented 5 years ago

@matthewgrimagig instead of using that to configure the container on Orleans directly, check the Configuring Generic Host Container here.

matthewgrimagig commented 5 years ago

thanks @galvesribeiro, no idea how I didn't think about that. Any idea when 2.3.0 will be released?

galvesribeiro commented 5 years ago

Dunno. @reubenbond perhaps have a clue.

sergeybykov commented 5 years ago

We can release it as early as next week. Just waiting for people to try 2.3.0-rc1 to see if we missed something.