-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Comments
Tagging subscribers to this area: @dotnet/area-extensions-options Issue DetailsBackground and motivationUsage of Following measures should be considered:
API Proposalnamespace 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 DesignsNo response RisksMoving types around.
|
See #47821 (comment) for some reasons why it was originally done this way. Maybe Hosting needs an |
@eerhardt Thanks for the context. What would be the difference between |
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. |
@tekian can you help me understand the problem statement here? I see
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? |
@ericstj The idea is to decouple the options validation from the hosting layer, allowing libraries (not only services) to depend only on In particular, I propose to decouple |
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? |
This issue has been marked |
@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 |
There are couple of ways how this could be achieved. We could expose 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 The hosted service in 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. |
@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 |
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. |
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 @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. |
I think we don't necessary need a new abstraction for that. The list of The platform support problem of 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. |
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 If we put I suppose the question is now -- do we split off the |
@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? |
@steveharter will be drafting an API proposal for |
We changed the interface name from 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))] |
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. TheValidateOnStart()
functionality should be moved to theMicrosoft.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 theValidateOnStart()
functionality and then obtained by a host (for the default host, in theMicrosoft.Extensions.Hosting
assembly) with a call toGetService<IValidateOnStart>()
in order to trigger the validation during startup.The new interface located in the
Microsoft.Extensions.Options
assembly:The existing
ValidateOnStart()
extension method is located on theMicrosoft.Extensions.DependencyInjection.OptionsBuilderExtensions
extension class in theMicrosoft.Extensions.Hosting
assembly. TheMicrosoft.Extensions.Hosting
assembly needs to forward this to theMicrosoft.Extensions.Options
assembly:Original request
Background and motivation
Usage of
ValidateOnStart
today is tightly coupled with Microsoft.Extensions.Hosting package. The call registers bothValidationHostedService
and instructsValidateOnStart
infrastructure to validateT
-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:
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.ValidationHostedService
intoHost
bootstrapping as a shared option infrastructure. It becomes part of defaultHost
and doesn't need to be registered with everyValidateOnStart
call.API Proposal
(Shape of the API remains the same)
API Usage
(Usage of the API remains the same.)
Alternative Designs
No response
Risks
Moving types around.
The text was updated successfully, but these errors were encountered: