Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Decouple ValidateOnStart from Hosting #84347

Closed
tekian opened this issue Apr 5, 2023 · 20 comments · Fixed by #88546
Closed

[API Proposal]: Decouple ValidateOnStart from Hosting #84347

tekian opened this issue Apr 5, 2023 · 20 comments · Fixed by #88546
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Options partner-impact This issue impacts a partner who needs to be kept updated
Milestone

Comments

@tekian
Copy link

tekian commented Apr 5, 2023

Usage of ValidateOnStart() today is located in the Microsoft.Extensions.Hosting assembly\package. This is problematic as it greatly inflates dependency tree of packages that need to validate their options eagerly when the hosting package is not otherwise needed. The ValidateOnStart() functionality should be moved to the Microsoft.Extensions.Options package.

In order for a host to call a "Validate()" method, a new public interface needs to be added, called IValidateOnStart here. This interface will be injected by the ValidateOnStart() functionality and then obtained by a host (for the default host, in the Microsoft.Extensions.Hosting assembly) with a call to GetService<IValidateOnStart>() in order to trigger the validation during startup.

The new interface located in the Microsoft.Extensions.Options assembly:

namespace Microsoft.Extensions.Options
{
+    public interface IValidateOnStart
+    {
+        void Validate();
+   }
}

The existing ValidateOnStart() extension method is located on the Microsoft.Extensions.DependencyInjection.OptionsBuilderExtensions extension class in the Microsoft.Extensions.Hosting assembly. The Microsoft.Extensions.Hosting assembly needs to forward this to the Microsoft.Extensions.Options assembly:

+[assembly: System.Runtime.CompilerServices.TypeForwardedTo(
+    typeof(Microsoft.Extensions.DependencyInjection.OptionsBuilderExtensions))]

Original request

Background and motivation

Usage of ValidateOnStart today is tightly coupled with Microsoft.Extensions.Hosting package. The call registers both ValidationHostedService and instructs ValidateOnStart infrastructure to validate T-type upon startup. This is problematic as it greatly inflates dependency tree of packages that need to validate their options eagerly.

Following measures should be considered:

  • Move ValidateOnStart to Microsoft.Extensions.Options package. This decouple registration from implementation. Code that expresses the desire for eager validation doesn't involve registration of ValidationHostedService in the same call. The new API enables a cleaner separation of concern.
  • Incorporate ValidationHostedService into Host bootstrapping as a shared option infrastructure. It becomes part of default Host and doesn't need to be registered with every ValidateOnStart call.

API Proposal

namespace Microsoft.Extensions.Options
{
    public static class OptionsBuilderExtensions
    {
        public static OptionsBuilder<TOptions> ValidateOnStart<TOptions>(this OptionsBuilder<TOptions> builder) 
            where TOptions : class;
    }

(Shape of the API remains the same)

API Usage

(Usage of the API remains the same.)

Alternative Designs

No response

Risks

Moving types around.

@tekian tekian added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 5, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 5, 2023
@ghost
Copy link

ghost commented Apr 5, 2023

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Usage of ValidateOnStart today is tightly coupled with Microsoft.Extensions.Hosting package. The call registers both ValidationHostedService and instructs ValidateOnStart infrastructure to validate T-type upon startup. This is problematic as it introduces cyclical dependencies and greatly inflates dependency tree of packages that need to validate their options eagerly.

Following measures should be considered:

  • Move ValidateOnStart to Microsoft.Extensions.Options package. This decouple registration from implementation. Code that expresses the desire for eager validation doesn't involve registration of ValidationHostedService in the same call. The new API enables a cleaner separation of concern.
  • Incorporate ValidationHostedService into Host bootstrapping as a shared option infrastructure. Implementation becomes part of default Host and doesnt need to be registered with every ValidateOnStart` call.

API Proposal

namespace Microsoft.Extensions.Options
{
    public static class OptionsBuilderExtensions
    {
        public static OptionsBuilder<TOptions> ValidateOnStart<TOptions>(
              this OptionsBuilder<TOptions> builder, 
              bool validateOnStart = true) where TOptions : class;
    }

(Shape of the API remains the same)

API Usage

(Usage of the API remains the same.)

Alternative Designs

No response

Risks

Moving types around.

Author: tekian
Assignees: -
Labels:

api-suggestion, area-Extensions-Options

Milestone: -

@tarekgh tarekgh added this to the 8.0.0 milestone Apr 5, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Apr 5, 2023
@tarekgh tarekgh added the partner-impact This issue impacts a partner who needs to be kept updated label Apr 5, 2023
@tarekgh
Copy link
Member

tarekgh commented Apr 5, 2023

CC @joperezr @geeknoid @davidfowl

@tekian tekian changed the title [API Proposal]: Decouple ValidateOnStart from ValidationHostedService [API Proposal]: Decouple ValidateOnStart from Hosting Apr 5, 2023
@eerhardt
Copy link
Member

eerhardt commented Apr 5, 2023

See #47821 (comment) for some reasons why it was originally done this way.

Maybe Hosting needs an IStartupService enumerable service that gets resolved and all of them notified on start of the Host. My suggestion in the above conversation was a hook in the DI container (to know when the container was built), but that was rejected because we can't rely on the specifics of our DI container implementation. It would have to be a feature across all DI implementations for us to take advantage of it.

@tekian
Copy link
Author

tekian commented Apr 6, 2023

@eerhardt Thanks for the context. What would be the difference between IStartupService and IHostedService? Regardless of which of the two it would use, I believe "the desire to validate options eagerly" can be safely de-coupled from "implementation that performs it". We could have AddValidateOnStart(this IServiceCollection... inside Microsoft.Extensions.Hosting package that would add the service. And ValidateOnStart(this IOptionsBuilder... would be in Microsoft.Extensions.Options.

@eerhardt
Copy link
Member

eerhardt commented Apr 6, 2023

What would be the difference between IStartupService and IHostedService?

The difference would be that it wouldn’t need to be kept alive for the entire process just to call a no-op Stop method on it during shutdown. It would just get created, notified, and disposed during Start.

@ericstj ericstj added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Apr 17, 2023
@ericstj
Copy link
Member

ericstj commented Apr 17, 2023

@tekian can you help me understand the problem statement here? I see

This is problematic as it greatly inflates dependency tree of packages that need to validate their options eagerly.

Does this mean you are able to accomplish your goals today but you end up depending on more packages than you'd like? How does this correspond to the customer experience?

@tekian
Copy link
Author

tekian commented Apr 20, 2023

@ericstj The idea is to decouple the options validation from the hosting layer, allowing libraries (not only services) to depend only on Microsoft.Extensions.Options instead of Microsoft.Extensions.Hosting. Microsoft.Extensions.Hosting has a significant dependency footprint.

In particular, I propose to decouple ValidationHostedService from ValidateOnStart, even introduce a separate provider for eager options validation that can be registered and configured during the Host buildout. This way, consumers could replace it with their own implementations if desired.

@ericstj
Copy link
Member

ericstj commented Apr 20, 2023

If a library needs hosting anyways to do anything useful in an application what do you accomplish by trying to remove the dependency?

Are you trying to say you want to support someone doing eager validation without using hosting at all? If so, wouldn’t that need more API than what is proposed here. Where is the API that the user would trigger to indicate Start, or the observable state that an option would wish to be initialized?

@ericstj ericstj added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 20, 2023
@ghost
Copy link

ghost commented Apr 20, 2023

This issue has been marked needs-author-action and may be missing some important information.

@tekian
Copy link
Author

tekian commented Apr 24, 2023

@ericstj Yes, the goal is to support libraries that are agnostic of hosting and services that don't use or support hosting. (e.g. Azure Functions SDK). From that perspective, we could look at current ValidationHostedService as a template for ValidateOnStart service that anybody could resolve from a DI and call. In Azure Functions SDK that would likely be a IFunctionInvocationFilter, in pure ASP.NET Core services IHostedService.

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Apr 24, 2023
@tekian
Copy link
Author

tekian commented Apr 24, 2023

There are couple of ways how this could be achieved.

We could expose ValidateOnStartOptions and an extension method ValidateOnStart(this IServiceProvider...).

public sealed class ValidateOnStartOptions
{
    public IDictionary<(Type optionsType, string optionsName), Action> Validators { get; } 
}

public static class OptionsValidation
{
    public static void ValidateOnStart(IServiceProvider serviceProvider)
    {
        var options = serviceProvider.GetService<IOptions<ValidateOnStartOptions>>();
        EagerValidation.ValidateOnStart.Instance.Validate(options);
    }
}

internal class ValidateOnStart
{
    internal static readonly ValidateOnStart Instance = new();

    public void Validate(IOptions<ValidateOnStartOptions> options)
    {
        // ...
    }
}

Or we could expose an API for the service itself and hide options.

public interface IValidateOnStart
{
    void Validate();
}

public static class OptionsValidation
{
    public static void ValidateOptions(IServiceProvider serviceProvider)
    {
        var validate = serviceProvider.GetRequiredService<IValidateOnStart>();
        validate.Validate();
    }
}

internal class ValidateOnStartOptions
{
    public IDictionary<(Type optionsType, string optionsName), Action> Validators { get; } 
}

internal class ValidateOnStart : IValidateOnStart
{
    public void Validate()
    {
        // ...
    }
}

All of this would/could be part of Microsoft.Extensions.Options.

The hosted service in Microsoft.Extensions.Hosting would/could change to following.

internal sealed class ValidationHostedService : IHostedService
{
    private readonly IValidateOnStart _validateOnStart;

    public ValidationHostedService(IValidateOnStart validateOnStart)
    {
        _validateOnStart = validateOnStart;
    }
 
    public Task StartAsync(CancellationToken cancellationToken)
    {
        _validateOnStart.Validate();
        return Task.CompletedTask;
    }
 
    public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask;
}

Or similar for the other proposal.

@ericstj
Copy link
Member

ericstj commented Apr 25, 2023

@davidfowl @eerhardt what do you think about this separation of responsibility? It allows libraries to opt in their types to eager validation without depending on hosting.

One challenge this still has is that it would still require someone to register that ValidationHostedService. That could be avoided by making the host resolve the registered singleton (either IValidateOnStart or IOptions<ValidateOnStartOptions>) and call Validate, rather than adding the ValidationHostedService.

@davidfowl
Copy link
Member

This sounds fine to me. It's less about validating on start and more about exposing an API that does the validation decoupled from hosting. Anyone can call it and hosting can too.

@eerhardt
Copy link
Member

I like the goal and think it is worthwhile to pursue a solution to it.

I think there is a more general pattern here between this issue and #43149. The pattern I see is "I want to run some code when the service container is built". There is also #84847, which is very similar except it is asking for a hook just before the container is built.

I question whether this should be at the Hosting layer or built into DependencyInjection. Note there are other app models like Blazor WASM and MAUI that don't use Microsoft.Extensions.Hosting. MAUI has a similar concept here: https://learn.microsoft.com/en-us/dotnet/api/microsoft.maui.hosting.imauiinitializeservice.initialize, which is retrieved and notified when the application is built at startup time:

https://github.com/dotnet/maui/blob/b18e80609dd00957cafcc886bf76743cf32e2759/src/Core/src/Hosting/MauiAppBuilder.cs#L149-L165

@DamianEdwards was following a similar pattern in https://github.com/aspnet/Benchmarks/pull/1836/files#r1175547227 using an IHostedService just to get a Database initializer executed when the app starts up.

In summary, I think we need a general mechanism for running code "at startup". Doing Option validation is just one of those things that needs this.

@rafal-mz
Copy link

rafal-mz commented Apr 25, 2023

I think we don't necessary need a new abstraction for that. The list of IHostedService is already executed sequentually at the startup. As far as I know, the BackgroundServices are run in the background, and until IHostedService finish we won't accept traffic.

The platform support problem of IHostedService also includes Azure Functions. Giving that DI today is not asynchronous, I think it is easier to bring IHostedServices to the platform that miss it rather than creating another model. I might miss some context though.

I think we could have a slot for first hosted service in the list, the same way as we have for the last service today:

Something like:

   if (services.Count != 0 && services[0].ImplementationType == typeof(StartupHostedService))
  {
      return;
  }

  services
      .RemoveAll<StartupHostedService>()
      .Insert(0, ServiceDescriptor.Singleton<IHostedService, StartupHostedService>());

The startup hosted service then, could accept a list of some jobs that are executed independently in concurrent fashion. For instance with the following signature:

/// <summary>
/// Holds the initialization function, so we can pass it through <see cref="IServiceProvider"/>.
/// </summary>
public interface IStartupInitializer
{
    /// <summary>
    /// Short startup initialization job.
    /// </summary>
    /// <param name="token">Cancellation token.</param>
    /// <returns>New <see cref="Task"/>.</returns>
    Task InitializeAsync(CancellationToken token);
}

To register such function, you would provide a timeout for such job. You could also use a function not to introduce an interface for simple cases:

AddStartupInitializer(this IServiceCollection services, Func<IServiceProvider, CancellationToken, Task> initializer);

AddStartupInitializer<T>(this IServiceCollection services)  where T : class, IStartupInitializer;

We could also consider adding a name for functions, so that there are no duplicate initializers.
I think such model fits nicely with the options validation model, populating database with the seed, checking DNS records at startup and so on.

@ericstj
Copy link
Member

ericstj commented Apr 27, 2023

So it sounds like folks are more/less OK with exposing a Startup method from options and we have some questions about how that might be plugged into Hosting.

@rafal-mz it just so happens we were already planning on adding a version of IStartupInitializer to hosting. It makes sense to have this feature use that functionality for registration. Probably we need to think about where IStartupInitializer lives and what is the gesture to register them. Do we require some explicit API call, or is it sufficient to just have one in the service collection?

If we put IStartupInitializer low enough and made consumption automatic in hosting, then we could pull off the move of the ValidateOnStart functionality without even adding any public API from Options (though we still could do that if we wanted to) or requiring explicit registration (though we could still do that if we wanted to). Perhaps this was what @tekian was implying the whole time but I was being too dense to follow. Regardless I'm with you now 😄.

I suppose the question is now -- do we split off the IStartupInitializer into a separate issue or just include it here? cc @steveharter who's starting to look at #43149.

@joperezr
Copy link
Member

joperezr commented May 8, 2023

@ericstj just making sure that the ball keeps on rolling here, from your previous message I'm not sure if you think that this is ready for review or if there is something we still want to validate via either @steveharter and/or @rafal-mz. Would you mind sharing what are the next steps here?

@ericstj
Copy link
Member

ericstj commented May 11, 2023

@steveharter will be drafting an API proposal for IStartupInitializer-functionality. Once this is done the work for Options (this issue) and DI (#43149) can proceed and depend on IStartupInitializer.

@steveharter steveharter added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration api-suggestion Early API idea and discussion, it is NOT ready for implementation api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Jun 27, 2023
@bartonjs
Copy link
Member

bartonjs commented Jul 6, 2023

Video

We changed the interface name from IValidateOnStart (describing when it's called) to IStartupValidator (describing what it is).

namespace Microsoft.Extensions.Options
{
    public interface IStartupValidator
   {
        void Validate();
   }
}

and we acknowledge the movement of OptionsBuilderExtensions

+[assembly: System.Runtime.CompilerServices.TypeForwardedTo(
+    typeof(Microsoft.Extensions.DependencyInjection.OptionsBuilderExtensions))]

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 6, 2023
@joperezr
Copy link
Member

joperezr commented Jul 7, 2023

@tekian @geeknoid looks like you guys weren't present on the above discussion. Any concerns with the decisions made above?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 8, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Options partner-impact This issue impacts a partner who needs to be kept updated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants