-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Description
Background and motivation
As it is right now, when an IHost is asked to StopAsync for graceful shutdown, each IHostedService is requested to StopAsync in sequential order. There's no reason why each IHostedService.StopAsync task can't be executed asynchronously and awaited altogether.
Why is this a problem? Well technically everything is functioning to spec, but in practice, it's problematic.
So normally the idea of being able to set the ShutdownTimeout is great.. we can set it to say, 30 seconds, and say we expect all IHostedService instances to take no longer than 30 seconds to shutdown gracefully. The problem is that, this is an additive timeout. That is to say, if you have 10 IHostedService instances, and each one of them takes 20 seconds to shutdown, your graceful shutdown now takes 200 seconds. Why do that when you can just have everything shutdown asynchronously in 20 seconds?
One example of the problem with this is that, when deploying apps to ECS in AWS, when ECS triggers an instance of an app to shutdown (which occurs under normal circumstances such as when scaling down), the maximum amount of time it gives for the app to gracefully shutdown is 120 seconds (see stopTimeout), after that it just kills the process.
There's no reason that i can see that the IHostedService instances should be shutdown sequentially, they are async for a reason, and if shutdown order is important, than it should be managed by the consumer themselves, or at the very least, there should be an option for asynchronous IHostedService shutdown.
Regression?
This is not a regression
Analysis
Link to code causing the issue:
https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Hosting/src/Internal/Host.cs#L126
This code could easily be modified to something similar to:
HashSet<Task> tasks = new HashSet<Task>();
foreach (IHostedService hostedService in _hostedServices.Reverse()) /* Why does this need to be reversed anyways? */
{
tasks.Add(hostedService.StopAsync(token));
}
// WhenAll would be great here, but it will swallow up exceptions.. so this is the next best thing
foreach (Task task in tasks)
{
try
{
await task.ConfigureAwait(false);
}
catch (Exception ex)
{
exceptions.Add(ex);
}
}API Proposal
public class HostOptions
{
+ public HostedServiceStopBehavior HostedServiceStopBehavior { get; set; } =
+ HostedServiceStopBehavior.Asynchronous;
public BackgroundServiceExceptionBehavior BackgroundServiceExceptionBehavior { get; set; } =
BackgroundServiceExceptionBehavior.StopHost;
}
+ /// <summary>
+ /// Specifies a behavior that the <see cref="IHost"/> will honor when stopping <see cref="IHostedService"/> instances.
+ /// </summary>
+ public enum HostedServiceStopBehavior
+ {
+ /// <summary>
+ /// Indicates that the host will stop the <see cref="IHostedService"/> instances asynchronously.
+ /// </summary>
+ Asynchronous = 0,
+
+ /// <summary>
+ /// Indicates that the host will wait for the previous <see cref="IHostedService"/> instance to stop before stopping the next instance.
+ /// </summary>
+ Sequential = 1
+ }API Usage
public static IHostBuilder CreateWebHostBuilder(string[] args)
{
return Host.CreateDefaultBuilder(args)
.ConfigureServices((context, services) =>
{
services.Configure<HostOptions>(options =>
{
options.BackgroundServiceStopBehavior = BackgroundServiceStopBehavior.Sequential; // Defaults to asynchronous
}
});
}Risks
The default behavior will technically be changing, which would be a breaking change. However, it may be a "safe" breaking change in the sense that the likelihood of someone building an app which depends on this previously internal behavior is highly unlikely and IF they did happen to, there is a way to revert back to the old behavior.