dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.37k stars 4.75k forks source link

[API Proposal]: Add `IHostedLifecycleService` to support additional callbacks #86511

Closed steveharter closed 1 year ago

steveharter commented 1 year ago

Background and motivation

In order to support scenarios that need hook points before and after IHostedService.StartAsync() and StopAsync(), this proposal adds a new interface IHostedLifecycleService with these hooks points. It derives from IHostedService and is not injected separately from IHostedService.

This interface is located in the Microsoft.Extensions.Hosting.Abstractions assembly which will be supported by the default host (in the Microsoft.Extensions.Hosting assembly). Other hosts will need to implement IHostedService in order to support this new interface.

See also:

API Proposal

These located in the Microsoft.Extensions.Hosting.Abstractions assembly.

namespace Microsoft.Extensions.Hosting
{
+    public interface IHostedLifecycleService : IHostedService
+    {
+        // These are awaited before 'IHostedService.StartAsync()' are run.
+        Task StartingAsync(CancellationToken cancellationToken);

+        // These are awaited after 'IHostedService.StartAsync()' are run.
+        Task StartedAsync(CancellationToken cancellationToken);

+        // These are awaited before 'IHostedService.StopAsync()' are run.
+        Task StoppingAsync(CancellationToken cancellationToken);

+        // These are awaited after 'IHostedService.StopAsync()' is run.
+        Task StoppedAsync(CancellationToken cancellationToken);
+    }
}

These located in the Microsoft.Extensions.Hosting assembly:

namespace Microsoft.Extensions.Hosting
{
    public class HostOptions
    {
        // Similar to ShutdownTimeout used in Host.StopAsync(), this creates a CancellationToken with the timeout.
        // Applies to Host.StartAsync() encompassing IHostedLifecycleService.StartingAsync(), IHostedService.StartAsync()
        // and IHostedLifecycleService.StartedAsync().
        // The timeout is off by default, unlike ShutdownTimeout which is 30 seconds.
        // This avoids a breaking change since existing implementations of IHostedService.StartAsync() are included in the timeout.
+       public TimeSpan StartupTimeout { get; set; } = Timeout.InfiniteTimeSpan;
    }
}

API Usage

IHostBuilder hostBuilder = new HostBuilder();
hostBuilder.ConfigureServices(services =>
{
    services.AddHostedService<MyService>();
}

using (IHost host = hostBuilder.Build())
{
    await host.StartAsync();
}

public class MyService : IHostedLifecycleService
{
    public Task StartingAsync(CancellationToken cancellationToken) => /* add logic here */ Task.CompletedTask;
    public Task StartAsync(CancellationToken cancellationToken) => /* add logic here */ Task.CompletedTask;
    public Task StartedAsync(CancellationToken cancellationToken) => /* add logic here */ Task.CompletedTask;
    public Task StopAsync(CancellationToken cancellationToken) => /* add logic here */ Task.CompletedTask;
    public Task StoppedAsync(CancellationToken cancellationToken) => /* add logic here */ Task.CompletedTask;
    public Task StoppingAsync(CancellationToken cancellationToken) => /* add logic here */ Task.CompletedTask;
}

Design notes

Ordering

The existing IHostApplicationLifetime which can be used for essentially the same hook points for "Started", "Stopping" and "Stopped" (but not "Starting") although those run serially and do not support an async, Task-based model. The new hook points contain more local semantics and thus are run before the corresponding IHostApplicationLifetime ones which are more "global" and may want to post-process any changes made. The IHostLifetime callbacks always come first\last.

The full lifecycle:

Exceptions and guarantees

Exception semantics are basically the same: exceptions from callbacks are caught, and when all callbacks are called, the exception(s) are logged and re-thrown.

All callbacks are guaranteed to be called (minus special cases in shutdown). For backwards compat, this means that for the default host at least, once start is called, all handlers for starting, start and stopped will be called even if an exception occurs in one of the earlier phases. The same applies for stop.

The above semantics do not hold for exception thrown from IHostApplicationLifetime callbacks which log but do not re-throw.

Threading

The newer options HostOptions.ServicesStartConcurrently and HostOptions.ServicesStopConcurrently added in V8 support an opt-in for a concurrent mode which runs both the StartAsync and StopAsync callbacks concurrently plus the new callbacks. When the newer concurrent mode is not enabled, the callbacks run serially.

When the concurrent mode is enabled:

Timeouts

Alternative Designs

An optional feature for ease-of-use for adding a simple callback via func\delegate (instead of overriding all 6 methods when only 1 is needed) was prototyped but no longer considered for v8 because it would have inconsistent and potentially confusing concurrency, exception and ordering semantics that are different from implementing IHostedLifecycleService directly.

The implementation would likely use a single instance of IHostedLifecycleService with a chained delegate for each callback type (starting, started, stopping, stopped and potentially start+stop) with the semantics differing from using an implementation of IHostedLifecycleService:

To get consistent semantics with direct IHostedLifecycleService implementations requires new public APIs so the host can know about this pattern -- the prototype simply used the abstractions assembly, and the hosting assembly was unaware of this.

E.g.

// For consistency with other IServiceCollection extensions, use the DependencyInjection namspace
namespace Microsoft.Extensions.DependencyInjection
{
    public static class ServiceCollectionHostedServiceExtensions
    {
        // Helper callbacks specifying a delegate for ease-of-use:

+       public static IServiceCollection AddServiceStarting(
+           this IServiceCollection services,
+           System.Func<IServiceProvider, CancellationToken, Task> startingFunc);

+       public static IServiceCollection AddServiceStart(
+           this IServiceCollection services,
+           System.Func<IServiceProvider, CancellationToken, Task> startingFunc);

+       public static IServiceCollection AddServiceStarted(
+           this IServiceCollection services,
+           System.Func<IServiceProvider, CancellationToken, Task> startedFunc);

+       public static IServiceCollection AddServiceStopping(
+           this IServiceCollection services,
+           System.Func<IServiceProvider, CancellationToken, Task> stoppingFunc);

+       public static IServiceCollection AddServiceStop(
+           this IServiceCollection services,
+           System.Func<IServiceProvider, CancellationToken, Task> startingFunc);

+       public static IServiceCollection AddServiceStopped(
+           this IServiceCollection services,
+           System.Func<IServiceProvider, CancellationToken, Task> stoppedFunc);
    }
}

Risks

No response

ghost commented 1 year ago

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

Issue Details
### Background and motivation In order to support scenarios that need a hook point when starting a Host, this is cumbersome today and only supports the hosting model. This proposal: - Adds a new interface `IServiceStartup` with a single `StartupAsync()` method. - This is located in the `Microsoft.Extensions.DependencyInjection.Abstractions` assembly to allow it to be used outside of hosting. - This interface will be implemented in the default host and called before other services that implement `IHostedService.StartAsync()`. Other hosts or layering on top of DI would also do this in order to support it. - Since there is no "EndAsync" method this is more performant than using `IHostedService` which has both `StartAsync` and `StopAsync`. See also: - https://github.com/dotnet/runtime/issues/84347 - https://github.com/dotnet/runtime/issues/43149 - R9's [StartupHostedService ](https://github.com/dotnet/r9/blob/cfc48d4cd493b854080c57f2afd92aeff26b6091/src/ToBeMoved/Hosting.StartupInitialization/Internal/StartupHostedService.cs#L17) A prototype is located [here](https://github.com/dotnet/runtime/compare/main...steveharter:runtime:Startup) where the unit tests verify a similar R9 callback approach and also verify that the validation effort done in V6 could be implemented with this. ### API Proposal ```csharp namespace Microsoft.Extensions.DependencyInjection; public partial interface IServiceStartup { System.Threading.Tasks.Task StartupAsync(System.Threading.CancellationToken cancellationToken); } ``` ### API Usage See the referenced prototype above. ### Alternative Designs _No response_ ### Risks _No response_
Author: steveharter
Assignees: steveharter
Labels: `api-suggestion`, `area-Extensions-DependencyInjection`
Milestone: 8.0.0
ericstj commented 1 year ago

cc @tekian @geeknoid @davidfowl @eerhardt @rafal-mz

rafal-mz commented 1 year ago

I think it would be useful to have some top-level built in timeout. Somebody could implement this interface as something that does not return, and then the server would stuck without any diagnostic about the reason.

steveharter commented 1 year ago

I think it would be useful to have some top-level built in timeout. Somebody could implement this interface as something that does not return, and then the server would stuck without any diagnostic about the reason.

An implementation of StartupAsync() that wraps a list of callbacks could of course add their own timeout around the whole thing. For example, if we add "builder" APIs for this where we have just one instance of IStartupAsync, that makes it possible to add several callbacks to that instance which then could have its own timeout across all of them.

It looks like the existing host implementation has the same issue with the implementation not returning (i.e. no timeout). If so, then I don't think we would want to add one just the new startup services. Perhaps for both IServiceStartup.StartupAsync and IHostedService.StartAsync(), however?

davidfowl commented 1 year ago

This shouldn't be part of DI if isn't used in the DI container

ericstj commented 1 year ago

Microsoft.Extensions.Hosting.Abstractions seems a better fit. It looks like this is low enough in the stack. It's not used by Options at the moment, but it seems like it could be without upsetting any existing layering. That's consistent with the existing ServiceCollectionHostedServiceExtensions

Below is the runtime layering:


graph TD;    
    Logging-->DependencyInjection;
    Logging-->Logging.Abstractions;
    Logging-->DependencyInjection.Abstractions;
    Logging-->Options;
    Hosting-->Logging;
    Hosting-->Logging.EventLog;
    Hosting-->Logging.Debug;
    Hosting-->FileProviders.Physical;
    Hosting-->Logging.Configuration;
    Hosting-->Configuration;
    Hosting-->DependencyInjection;
    Hosting-->Configuration.Json;
    Hosting-->Configuration.Binder;
    Hosting-->Configuration.EnvironmentVariables;
    Hosting-->Logging.EventSource;
    Hosting-->Configuration.FileExtensions;
    Hosting-->Logging.Abstractions;
    Hosting-->Hosting.Abstractions;
    Hosting-->Configuration.Abstractions;
    Hosting-->DependencyInjection.Abstractions;
    Hosting-->FileProviders.Abstractions;
    Hosting-->Options;
    Hosting-->Logging.Console;
    Hosting-->Configuration.UserSecrets;
    Hosting-->Configuration.CommandLine;
    Logging.EventLog-->Logging;
    Logging.EventLog-->Logging.Abstractions;
    Logging.EventLog-->DependencyInjection.Abstractions;
    Logging.EventLog-->Options;
    Logging.Debug-->Logging;
    Logging.Debug-->Logging.Abstractions;
    Logging.Debug-->DependencyInjection.Abstractions;
    Configuration.Ini-->Configuration;
    Configuration.Ini-->Configuration.FileExtensions;
    Configuration.Ini-->Configuration.Abstractions;
    Configuration.Ini-->FileProviders.Abstractions;
    Logging.TraceSource-->Logging;
    Logging.TraceSource-->Logging.Abstractions;
    Logging.TraceSource-->DependencyInjection.Abstractions;
    FileProviders.Physical-->FileSystemGlobbing;
    FileProviders.Physical-->Primitives;
    FileProviders.Physical-->FileProviders.Abstractions;
    Configuration.Xml-->Configuration;
    Configuration.Xml-->Configuration.FileExtensions;
    Configuration.Xml-->Configuration.Abstractions;
    Configuration.Xml-->FileProviders.Abstractions;
    Logging.Configuration-->Logging;
    Logging.Configuration-->Configuration;
    Logging.Configuration-->Configuration.Binder;
    Logging.Configuration-->Options.ConfigurationExtensions;
    Logging.Configuration-->Logging.Abstractions;
    Logging.Configuration-->Configuration.Abstractions;
    Logging.Configuration-->DependencyInjection.Abstractions;
    Logging.Configuration-->Options;
    Configuration-->Primitives;
    Configuration-->Configuration.Abstractions;
    DependencyInjection-->DependencyInjection.Abstractions;
    Configuration.Json-->Configuration;
    Configuration.Json-->Configuration.FileExtensions;
    Configuration.Json-->Configuration.Abstractions;
    Configuration.Json-->FileProviders.Abstractions;
    Http-->Logging;
    Http-->Logging.Abstractions;
    Http-->DependencyInjection.Abstractions;
    Http-->Options;
    Configuration.Binder-->Configuration.Abstractions;
    Configuration.EnvironmentVariables-->Configuration;
    Configuration.EnvironmentVariables-->Configuration.Abstractions;
    Logging.EventSource-->Logging;
    Logging.EventSource-->Primitives;
    Logging.EventSource-->Logging.Abstractions;
    Logging.EventSource-->DependencyInjection.Abstractions;
    Logging.EventSource-->Options;
    Configuration.FileExtensions-->FileProviders.Physical;
    Configuration.FileExtensions-->Configuration;
    Configuration.FileExtensions-->Primitives;
    Configuration.FileExtensions-->Configuration.Abstractions;
    Configuration.FileExtensions-->FileProviders.Abstractions;
    Options.ConfigurationExtensions-->Configuration.Binder;
    Options.ConfigurationExtensions-->Primitives;
    Options.ConfigurationExtensions-->Configuration.Abstractions;
    Options.ConfigurationExtensions-->DependencyInjection.Abstractions;
    Options.ConfigurationExtensions-->Options;
    Options.DataAnnotations-->DependencyInjection.Abstractions;
    Options.DataAnnotations-->Options;
    Caching.Abstractions-->Primitives;
    Hosting.Abstractions:::classHA-->Configuration.Abstractions;
    Hosting.Abstractions-->DependencyInjection.Abstractions;
    Hosting.Abstractions-->FileProviders.Abstractions;
    Configuration.Abstractions-->Primitives;
    FileProviders.Abstractions-->Primitives;
    Options:::classO-->Primitives;
    Options-->DependencyInjection.Abstractions;
    Logging.Console-->Logging;
    Logging.Console-->Logging.Configuration;
    Logging.Console-->Options.ConfigurationExtensions;
    Logging.Console-->Logging.Abstractions;
    Logging.Console-->Configuration.Abstractions;
    Logging.Console-->DependencyInjection.Abstractions;
    Logging.Console-->Options;
    Configuration.UserSecrets-->FileProviders.Physical;
    Configuration.UserSecrets-->Configuration.Json;
    Configuration.UserSecrets-->Configuration.Abstractions;
    Caching.Memory-->Primitives;
    Caching.Memory-->Logging.Abstractions;
    Caching.Memory-->Caching.Abstractions;
    Caching.Memory-->DependencyInjection.Abstractions;
    Caching.Memory-->Options;
    Configuration.CommandLine-->Configuration;
    Configuration.CommandLine-->Configuration.Abstractions;
    classDef classHA fill:#f96
    classDef classO fill:#9f3

This would mean that Options would add Microsoft.Extensions.Hosting.Abstractions, Microsoft.Extensions.Configuration.Abstractions, and Microsoft.Extensions.FileProviders.Abstractions to it's closure when implementing https://github.com/dotnet/runtime/issues/84347. All pretty small. The only other option (pun not intended) would be Microsoft.Extensions.Primitives which would not add anything to Options' closure, but that seems wrong.

We'd also need to think about #43149. This would need to go in Microsoft.Extensions.Hosting.Abstractions or higher as well. That seems reasonable since it's about describing an interaction of DI with startup (hosting concept).

@Davidfowl / @DamianEdwards, what do you think about timeout? Is it OK to add a timeout that corresponds to both this and IHostedService startup?

DamianEdwards commented 1 year ago

what do you think about timeout? Is it OK to add a timeout that corresponds to both this and IHostedService startup?

This could only be a cooperative timeout right? So if we did have one, I would assume it would be encapsulated in the CancellationToken passed in. But generally speaking I don't think we need one for the startup case as the point is to be able to add logic that runs as part of startup in a logically blocking way. If you want to timebox a startup task, just put that logic in the startup task. Shutdown of course is different as the process can just end after the timeout has expired.

steveharter commented 1 year ago

This shouldn't be part of DI if isn't used in the DI container

Microsoft.Extensions.Hosting.Abstractions seems a better fit.

Moving to Microsoft.Extensions.Hosting.Abstractions makes sense provided that addresses the concerns based on this comment from @tekian where "hosting" may not used, for example, by Azure Functions SDK. I'm assuming now that the "hosting" comment means the super-high-level Microsoft.Extensions.Hosting assembly and not the lower-level Microsoft.Extensions.Hosting.Abstractions.

steveharter commented 1 year ago

This could only be a cooperative timeout right? So if we did have one, I would assume it would be encapsulated in the CancellationToken passed in.

It should work the same way as today's ShutdownTimeout + Host.StopAsync() which uses the cancellation token.

Also, the startup timeout would need to be off by default, to prevent a breaking change for long-running IHostedService.StartAsync() implementations. However, if we only include the new IServiceStartup.StartupAsync() in the timeout (and not IHostedService.StartAsync()) then we could have a default of say, 30 seconds, which is the default for ShutdownTimeout.

DamianEdwards commented 1 year ago

Are we saying it will stop "blocking" startup after the timeout is reached?

steveharter commented 1 year ago

Are we saying it will stop "blocking" startup after the timeout is reached?

Not sure what is being "blocked" here. The idea of the timeout is to help diagnose any slow implementations of the new StartupAsync() by throwing a TaskCanceledException when the timeout is reached (the token needs to be passed\used around cooperatively by any services). I don't think it is a recoverable error.

Whether we extend the timeout to the existing StartAsync() is an open option. We can either: 1) Have the timeout encompass calling all instances of IServiceStartup.StartupAsync() and IHostedService.StartAsync() 2) Have the timeout encompass calling all instances of just the new IServiceStartup.StartupAsync() 3) ...other...

If we do option (2) and the timeout is hit, I assume the TaskCanceledException will prevent IHostedService.StartAsync() from being called.

The flow for option (1) when IHost.StartAsync is called (on the default host for now):

eerhardt commented 1 year ago

This shouldn't be part of DI if isn't used in the DI container

If we were to do this as part of DI, we would really need for it to be in all the DI implementations, not just MEDI. We would want IHostedServiceStartup (or whatever we would call it in the DI world) to be in Microsoft.Extensions.DependencyInjection.Abstractions and then for all the DI implementations to respect it during Build/CreateServiceProvider().

Given that it takes a long time to add DI features, and we can do this outside of DI, I think this proposal makes sense. Unless we really think that this must be part of DI. @davidfowl - how passionate are you about making this a DI feature? (Interestingly enough, this issue is in area-Extensions-DependencyInjection, which I assume is a mistake.)

startup timeout

I kind of think this should be a separate proposal. We have StartAsync() calls today without a timeout. I don't see a reason why we must do both in the same API proposal. Obviously they affect each other, but I wouldn't want to block this feature on figuring out startup timeout semantics.

ghost commented 1 year ago

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

Issue Details
### Background and motivation In order to support scenarios that need a hook point when starting a Host that runs before the existing hook point. This proposal: - Adds a new interface `IHostedServiceStartup` with a single `StartupAsync()` method. - This is located in the `Microsoft.Extensions.Hosting.Abstractions` assembly which will be supported by the default host and called before other services that implement `IHostedService.StartAsync()`. Other hosts besides the default host would also do this in order to support it. - Since there is no "EndAsync" method this is more performant than using `IHostedService` which has both `StartAsync` and `StopAsync`. See also: - https://github.com/dotnet/runtime/issues/84347 - https://github.com/dotnet/runtime/issues/43149 A prototype is located [here](https://github.com/dotnet/runtime/compare/main...steveharter:runtime:Startup). ### API Proposal #### These located in the Microsoft.Extensions.Hosting.Abstractions assembly. ```diff namespace Microsoft.Extensions.Hosting { + public partial interface IHostedServiceStartup + { + Task StartupAsync(CancellationToken cancellationToken = default(System.Threading.CancellationToken)); + } } // For consistency with other IServiceCollection extensions, use the DependencyInjection namspace namespace Microsoft.Extensions.DependencyInjection { public static class ServiceCollectionHostedServiceExtensions { // Callback specifying a delegate. + public static IServiceCollection AddServiceStartup( + this IServiceCollection services, + System.Func startupTask); // Callback specifying an implementation class. + public static ServiceCollection AddServiceStartup + <[DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TServiceStartup> + (this IServiceCollection services) + where TServiceStartup : class, IHostedServiceStartup; } } ``` #### These located in the Microsoft.Extensions.Hosting assembly: ```diff namespace Microsoft.Extensions.Hosting { public class HostOptions { // Similar to ShutdownTimeout used in Host.StopAsync(), this creates a CancellationToken with the timeout. // Applies to Host.StartAsync() encompassing both IHostedServiceStartup.StartupAsync() and IHostedService.StartAsync(). // The timeout is off by default, unlike ShutdownTimeout which is 30 seconds. This avoids a breaking change // since IHostedService.StartAsync() is included in the timeout. // We use TimeSpan.Zero to signify no timeout; these semantics will also be applied to ShutdownTimeout for consistency. + public TimeSpan StartupTimeout { get; set; } = TimeSpan.Zero; } } ``` ### API Usage See the referenced prototype above. Example of using a delegate: ```cs IHostBuilder hostBuilder = CreateHostBuilder(services => { services.AddServiceStartup((provider, cancellationToken) => { // return Task.CompletedTask; }); }); using (IHost host = hostBuilder.Build()) { await host.StartAsync(); } ``` ### Alternative Designs _No response_ ### Risks _No response_
Author: steveharter
Assignees: steveharter
Labels: `api-suggestion`, `area-Extensions-Hosting`
Milestone: 8.0.0
ericstj commented 1 year ago

I don't think anyone was suggesting to put the call to the startup logic in DI. Merely put the interface there, since it would avoid introducing new dependencies. Just putting the interface in DI.Abstractions doesn't take a long time, though it does "feel wrong" as @davidfowl previously mentioned.

I wouldn't want to block this feature on figuring out startup timeout semantics.

This feature is meant to completely replace https://github.com/dotnet/extensions/tree/main/src/ToBeMoved/Hosting.StartupInitialization which did have a timeout. So while I agree they could be separate, addressing the timeout question for startup methods is an important one for completeness to remove the Hosting.StartupInitialization. If it's easier to hold that as a separate discussion that's OK, but please make sure it remains in scope for 8.0.

steveharter commented 1 year ago

Waiting on validation from @tekian or substitute to verify this proposal will remove the need for workaround logic linked above

tekian commented 1 year ago

@steveharter Proposal looks good, thank you. Placing this under Microsoft.Extensions.Hosting.Abstractions makes sense and I think in no way it goes against https://github.com/dotnet/runtime/issues/84347, quite the opposite. We can switch to use it for eager options validation.

steveharter commented 1 year ago

Note to cloud native reviewers @tekian @rafal-mz and ASP.NET reviewers @DamianEdwards @davidfowl this is marked "ready for review" and will be reviewed soon (earliest would be June 8th).

hwoodiwiss commented 1 year ago

Just to clarify this:

Note that IHostedService doesn't have a timeout for StartAsync() but does for StopAsync(). This existing timeout is 30 seconds in the default host and 5 seconds with ASP.NET although that is https://github.com/dotnet/aspnetcore/issues/48605.

Applications using WebApplication.CreateBuilder(args) use a GenericWebHostService which is an IHostedService so has a 30 second default ShutdownTimeout. Older applications using WebHost.CreateDefaultBuilder(args) use a WebHost which has the 5 second default ShutdownTimeout at the moment.

halter73 commented 1 year ago

Could we do this without the new IHostedServiceStartup interface? We could rename AddServiceStartup to AddHostedServiceStartup and have the Func<IServiceProvider, CancellationToken, Task> overload add an IHostedService with a no-op StopAsync implementation.

The big upside of this is that the Func<IServiceProvider, CancellationToken, Task> startupTask (we need to rename that) will at least still be run if Microsoft.Extensions.Hosting.Abstractions gets hoisted to the 8.0 nuget package while the Host implementation in Microsoft.Extensions.Hosting is from .NET 7 or before. In that case, if we tried to use IHostedServiceStartup for this, any calls to AddServiceStartup would just get ignored silently and never run. 😱

In the API usage example, AddServiceStartup is called before any other IHostedService is added anyway, so it'd work the same way if it added an IHostedService instead of an IHostedServiceStartup. And trying to add new hooks that simply run before other hooks which already have ordering leads to madness. What's next IPreHostedServiceStartup? Or IPrePreHostedServiceStartup since this proposal should really be called IPreHostedServiceStartup already? @Tratcher

eerhardt commented 1 year ago

add an IHostedService with a no-op StopAsync implementation.

I added one reason I don't like that approach here.

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.


The big upside of this is that the Func<IServiceProvider, CancellationToken, Task> startupTask (we need to rename that) will at least still be run if Microsoft.Extensions.Hosting.Abstractions gets hoisted to the 8.0 nuget package while the Host implementation in Microsoft.Extensions.Hosting is from .NET 7 or before.

I don't consider that a "big" upside. While we don't explicitly say this scenario is unsupported, it is pretty typical behavior that if you don't update to the new assemblies, you don't get new behavior.

Tratcher commented 1 year ago

And trying to add new hooks that simply run before other hooks which already have ordering leads to madness. What's next IPreHostedServiceStartup? Or IPrePreHostedServiceStartup since this proposal should really be called IPreHostedServiceStartup already? @Tratcher Chris Ross FTE

Completely agree, hooks upon hooks leads to this: https://learn.microsoft.com/en-us/iis/application-frameworks/building-and-running-aspnet-applications/aspnet-integration-with-iis#runtime-fidelity

BeginRequest. The request processing starts.
AuthenticateRequest. The request is authenticated. IIS and ASP.NET authentication modules subscribe to this stage to perform authentication.
PostAuthenticateRequest.
AuthorizeRequest. The request is authorized. IIS and ASP.NET authorization modules check whether the authenticated user has access to the resource requested.
PostAuthorizeRequest.
ResolveRequestCache. Cache modules check whether the response to this request exists in the cache, and return it instead of proceeding with the rest of the execution path. Both ASP.NET Output Cache and IIS Output Cache features execute.
PostResolveRequestCache.
MapRequestHandler. This stage is internal in ASP.NET and is used to determine the request handler.
PostMapRequestHandler.
AcquireRequestState. The state necessary for the request execution is retrieved. ASP.NET Session State and Profile modules obtain their data.
PostAcquireRequestState.
PreExecuteRequestHandler. Any tasks before the execution of the handler are performed.
ExecuteRequestHandler. The request handler executes. ASPX pages, ASP pages, CGI programs, and static files are served.
PostExecuteRequestHandler
ReleaseRequestState. The request state changes are saved, and the state is cleaned up here. ASP.NET Session State and Profile modules use this stage for cleanup.
PostReleaseRequestState.
UpdateRequestCache. The response is stored in the cache for future use. The ASP.NET Output Cache and IIS Output Cache modules execute to save the response to their caches.
PostUpdateRequestCache.
LogRequest. This stage logs the results of the request, and is guaranteed to execute even if errors occur.
PostLogRequest.
EndRequest. This stage performs any final request cleanup, and is guaranteed to execute even if errors occur.

If you really want ordering then design a startup pipeline like we have for middleware.

halter73 commented 1 year ago

I don't think the cost of keeping around the no-op IHostedService after startup would be too high. If you implement it yourself, you can unreferenced anything you want to after start completes. If you use the Func<IServiceProvider, CancellationToken, Task>, we can null that out for you.

ericstj commented 1 year ago

@halter73 sounds like you're giving implementation feedback. Even with an interface it could be plugged into an IHostedService. That's what the existing implementation does.

The problem with IHostedService (now) is that there is no way to guarantee ordering. You can try to insert the service in front https://github.com/dotnet/extensions/blob/f4952b69a04e9bef266089cbc3059675693fbaa0/src/ToBeMoved/Hosting.StartupInitialization/Internal/StartupInitializationBuilder.cs#L67-L69 but the user might still specify concurrent start: https://github.com/dotnet/runtime/pull/84048

If we're really worried about the hosting mismatch we could insert a sentinel IHostedService, IHostedServiceStartup that guarantees that it's IHostedServiceStartup.StartupAsync was called before it's IHostedService.StartAsync.

Tratcher commented 1 year ago

The problem with IHostedService (now) is that there is no way to guarantee ordering.

Then let's make a way. Trying to special case stuff with additional stages is unsustainable.

How about this:

+    public interface IOrderedHostedService : IHostedService
+    {
+        int Order { get; }
+    }

IHostedService instances without an order assume 0. Negative values come first.

Alternatively, what about a singleton service designed to sort the IHostedService instances?

Or we go back to the explicit ordering model: First added wins. As long as the user has control over the order things are added, it should be ok.

steveharter commented 1 year ago

The problem with IHostedService (now) is that there is no way to guarantee ordering. You can try to insert the service in front but the user might still specify concurrent start

Then let's make a way. Trying to special case stuff with additional stages is unsustainable.

I think of it more as a bifurcation and not just ordering control in a single list. For example, scenarios when framework code needs to always come before (or after) application code. For this feature, the "framework code" is the new callback here which for cloud native needs to run before existing application code which may not even know or care about the framework code. That's why there's effectively 2 lists here.

Ordering for a single (simple) list won't work if HostOptions.ServicesStartConcurrently is true. We'd need to add something like:

+    public interface INonConcurrentHostedService
+    { // Here, just a signature; overrides ServicesStartConcurrently=true
+    }

which may also be useful for the 2-list case as well to address cases where validation, for example, needs to finish before other startup logic (singleton preheating, for example).

How about this:

+  public interface IOrderedHostedService : IHostedService
+    {
+        int Order { get; }
+    }

It should really be a fixed value; I don't think we want to worry about the value changing or have to re-sort on every use for example. But it could be made to work. In general, having the application code specify order of the "known" services (as today) works fine; it's the newly-desired framework-added services that need special treatment.

rafal-mz commented 1 year ago

I think that the problem is that currently we don't have clear point when application is ready to boot. I understand IHostedService pipeline as startup phase, while IStartupService as 'preparing-to-start' phase. The same way, as there are two phases for WebApplication - one for config and essentials setup, and second for composing your app.

Use caces for IStartupService:

With such distinction in mind, hosted service implementations may assume that app configuration is correct. This is the direct implementation of 'fail fast' principle - app crashes immediately when configuration is incorrect, instead of crashing in the middle of doing real work. We need runtime component for that, because in distributed environment - other services state becomes part of our configuration. Current IHostedService interface makes it much harder to achieve, since customer may register such hosted services as first: https://github.com/ThreeMammals/Ocelot/blob/develop/src/Ocelot/Configuration/Repository/FileConfigurationPoller.cs https://github.com/bitwarden/server/blob/master/src/Core/HostedServices/ApplicationCacheHostedService.cs

terrajobst commented 1 year ago

Video

namespace Microsoft.Extensions.Hosting
{
    public interface IHostedServiceStartup
    {
        Task StartupAsync(CancellationToken cancellationToken = default);
    }
}

// For consistency with other IServiceCollection extensions, use the DependencyInjection namespace
namespace Microsoft.Extensions.DependencyInjection
{
    public static partial class ServiceCollectionHostedServiceExtensions
    {
       public static ServiceCollection AddServiceStartup
           <[DynamicallyAccessedMembersAttribute(DynamicallyAccessedMemberTypes.PublicConstructors)] TServiceStartup>(
           this IServiceCollection services)
           where TServiceStartup : class, IHostedServiceStartup;

       public static IServiceCollection AddServiceStartup(
           this IServiceCollection services,
           Func<IServiceProvider, CancellationToken, Task> startupFunc);
    }
}
Tratcher commented 1 year ago

Use caces for IStartupService: We should validate options before we resolve anything, We should ensure that our downstream services are reachable, We may ensure our database schema is correct, Pre-heat our Caches

It seems like these examples are themselves order dependent, so the new interface doesn't solve the ordering problem, it just subdivides it. Ultimately, you'll end up with ordering dependencies between these scenarios that we'll have to solve. Solving the broader problem of how to order IHostedServices would address that now.

steveharter commented 1 year ago

It seems like these examples are themselves order dependent, so the new interface doesn't solve the ordering problem, it just subdivides it. Ultimately, you'll end up with ordering dependencies between these scenarios that we'll have to solve. Solving the broader problem of how to order IHostedServices would address that now.

Yes this doesn't solve the "inner dependency" issue.

However, if the dependencies are known ahead-of-time here's some strategies:

If the dependencies are not known ahead-of-time, such as framework not knowing about application code, or by having two extensions that don't know about each other, then the logical ordering layering here is one more tool to help with that.

terrajobst commented 1 year ago

Video

namespace Microsoft.Extensions.Hosting;

public interface IHostedLifecycleService : IHostedService
{
    Task StartingAsync(CancellationToken cancellationToken);
    Task StartedAsync(CancellationToken cancellationToken);
    Task StoppingAsync(CancellationToken cancellationToken);
    Task StoppedAsync(CancellationToken cancellationToken);
}

public partial class HostOptions
{
    public TimeSpan StartupTimeout { get; set; }
}