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]: Add IHostedLifecycleService to support additional callbacks #86511

Closed
steveharter opened this issue May 19, 2023 · 30 comments · Fixed by #87335
Closed

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

steveharter opened this issue May 19, 2023 · 30 comments · Fixed by #87335
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Hosting blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented May 19, 2023

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

  • The intent is that the new callbacks are cooperative amongst users and only used for services such as validation and warm-up scenarios that need to occur before\after the existing IHostedService.StartAsync() and IHostedService.StopAsync().

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:

  • IHostLifetime.WaitForStartAsync
  • IHostedLifecycleService.StartingAsync
  • IHostedService.Start
  • IHostedLifecycleService.StartedAsync
  • IHostApplicationLifetime.ApplicationStarted
  • IHostedLifecycleService.StoppingAsync
  • IHostApplicationLifetime.ApplicationStopping
  • IHostedService.Stop
  • IHostedLifecycleService.StoppedAsync
  • IHostApplicationLifetime.ApplicationStopped
  • IHostLifetime.StopAsync

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:

  • A given hook point, such as a IHostedLifecycleService.StartingAsync(), runs serially with other IHostedLifecycleService.StartingAsync() implementations in the order of registration until an await occurs, if any (or in general, an uncompleted Task is returned). At that point, all such async callbacks are run concurrently. This is to optimize performance since many hook point will return Task.CompletedTask if the implementation is a noop. This also allows the author more control over the ordering and semantics of additional async calls.
  • The StartAsync and\or StopAsync will change to the same optimized serial+concurrent model as explained above for the new callbacks**. Currently StartAsync\StopAsync do not have the "serial" portion and thus run concurrently even for synchronous methods. Changing this is feasible because the main reason for this new V8 feature was to avoid timeouts during a long shutdown, such as when the host needs to drain requests, and that logic should be async already.

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:

  • Async but running in series (instead of concurrent for callbacks that aren't all sync)
  • The first exception will prevent subsequent delegates from being called (instead of guaranteeing each callback is called for a given method)
  • Ordering is relative to other implementations of IHostedLifecycleService by the first call to add any delegate and not the individual order (a "builder" pattern would be useful to communicate that).

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

@steveharter steveharter added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-DependencyInjection labels May 19, 2023
@steveharter steveharter added this to the 8.0.0 milestone May 19, 2023
@steveharter steveharter self-assigned this May 19, 2023
@ghost
Copy link

ghost commented May 19, 2023

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:

A prototype is located here 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

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
Copy link
Member

ericstj commented May 19, 2023

@rafal-mz
Copy link

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
Copy link
Member Author

steveharter commented May 19, 2023

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
Copy link
Member

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

@ericstj
Copy link
Member

ericstj commented May 23, 2023

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 #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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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

@steveharter
Copy link
Member Author

steveharter commented May 25, 2023

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):

  • A cancellationToken is created with the timeout and linked to the token passed in.
  • All services that implement IServiceStartup.StartupAsync() are called. This respects the ServicesStartConcurrently option like IHostedService.StartAsync() does.
  • Wait until all IServiceStartup.StartupAsync() are finished.
  • All services that implement IHostedService.StartAsync() are called as they are today. The same cancellationToken instance is used, so the timeout encompasses both StartupAync() and StartAsync().

@steveharter steveharter changed the title [API Proposal]: Add IServiceStartup.StartupAsync() to support pre-startup scenarios [API Proposal]: Add IHostedServiceStartup.StartupAsync() to support pre-startup scenarios May 26, 2023
@eerhardt
Copy link
Member

eerhardt commented May 30, 2023

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
Copy link

ghost commented May 31, 2023

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:

A prototype is located here.

API Proposal

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

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<IServiceProvider, CancellationToken, Task> 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:

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:

IHostBuilder hostBuilder = CreateHostBuilder(services =>
{
    services.AddServiceStartup((provider, cancellationToken) =>
    {
        // <add logic here>
        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
Copy link
Member

ericstj commented May 31, 2023

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
Copy link
Member Author

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

@tekian
Copy link

tekian commented Jun 7, 2023

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

@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 api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 7, 2023
@steveharter
Copy link
Member Author

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
Copy link
Contributor

hwoodiwiss commented Jun 7, 2023

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 dotnet/aspnetcore#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
Copy link
Member

halter73 commented Jun 8, 2023

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
Copy link
Member

eerhardt commented Jun 8, 2023

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
Copy link
Member

Tratcher commented Jun 8, 2023

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
Copy link
Member

halter73 commented Jun 8, 2023

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
Copy link
Member

ericstj commented Jun 9, 2023

@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: #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
Copy link
Member

Tratcher commented Jun 9, 2023

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
Copy link
Member Author

steveharter commented Jun 12, 2023

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
Copy link

rafal-mz commented Jun 12, 2023

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:

  • 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

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
Copy link
Member

terrajobst commented Jun 13, 2023

Video

  • There concerns about the number of 'stages'. This adds one stage, do we end up in world where we need multiple stages.
    • The IOrderedHostedService is another option
  • What about a shutdown?
  • This needs more discussion
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);
    }
}

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 13, 2023
@Tratcher
Copy link
Member

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 steveharter changed the title [API Proposal]: Add IHostedServiceStartup.StartupAsync() to support pre-startup scenarios [API Proposal]: Add IHostedLifecycleService to support pre-startup scenarios Jun 21, 2023
@steveharter steveharter changed the title [API Proposal]: Add IHostedLifecycleService to support pre-startup scenarios [API Proposal]: Add IHostedLifecycleService to support additional callbacks Jun 21, 2023
@steveharter
Copy link
Member Author

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:

  • Collapse or manage multiple services in one service which would have the dependencies engrained.
  • Use DI constructor injection when implementing the new interface methods where app-controlled synchronization\dependencies\ordering can occur (i.e. the constructors of the new interface can have dependencies).
  • The application code that creates the service collection may understand these dependencies and can re-arrange the service collection order.

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.

@steveharter steveharter added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Jun 23, 2023
@terrajobst
Copy link
Member

terrajobst commented Jun 27, 2023

Video

  • Looks good as proposed
  • Should BackgroundService implement IHostedLifecycleService?
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; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 27, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 28, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 30, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 31, 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-Hosting blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.