Move BackgroundService implementation from StartAsync to the new StartedAsync

[steveharter]

As part of #87335, there were discussions that BackgroundService implementation should be moved to StartedAsync(). This will allow other IHostedService.StartAsync() implementations to always run before backgroundservice which prevents order-of-registration issues.

Since BackgroundService already has a virtual Start() implementing IHostedService.Start(), we would keep that and explicitly implement the new IHostedLifecycleService members:

API

namespace Microsoft.Extensions.Hosting
{
-   public abstract class BackgroundService : IDisposable, IHostedService

    // Note that IHostedLifecycleService implements IHostedService
+   public abstract class BackgroundService : IDisposable, IHostedLifecycleService

    // Keep this, but this method will no longer implement IHostedService.StartAsync
    public virtual Task StartAsync(CancellationToken cancellationToken);

+   Task IHostedLifecycleService.StartingAsync(CancellationToken cancellationToken); //noop
+   Task IHostedLifecycleService.StartAsync(CancellationToken cancellationToken); //noop
+   Task IHostedLifecycleService.StartedAsync(CancellationToken cancellationToken); //noop
+   Task IHostedLifecycleService.StoppingAsync(CancellationToken cancellationToken); //noop
    // We don't need to explictely implement StopAsync since the existing virtual is fine
+   Task IHostedLifecycleService.StoppedAsync(CancellationToken cancellationToken); //noop
}

Alternative

Note that by implementing the new callbacks, it prevents derived classes from also implementing them. Consider the alternative where the new callbacks are all added as virtuals, but this would be breaking since existing overrides of StartAsync will have pre- and post- behavior broken when calling base.StartAsync() since the base class implementation (BackgroundService) will no longer have any implementation:

namespace Microsoft.Extensions.Hosting
{
-   public abstract class BackgroundService : IDisposable, IHostedService
+   public abstract class BackgroundService : IDisposable, IHostedLifecycleService

+   public virtual Task StartingAsync(CancellationToken cancellationToken); //noop
    public virtual Task StartAsync(CancellationToken cancellationToken); //noop
+   public virtual Task StartedAsync(CancellationToken cancellationToken);
+   public virtual Task StoppingAsync(CancellationToken cancellationToken); //noop
    public virtual Task StopAsync(CancellationToken cancellationToken);
+   public virtual Task StoppedAsync(CancellationToken cancellationToken); //noop
}

[ghost]

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

Issue Details

---

As part of #87335, there were discussions that BackgroundService implementation should be moved to StartedAsync(). This will allow other IHostedService.StartAsync() implementations to always run before backgroundservice which prevents order-of-registration issues.

Since BackgroundService already has a virtual Start() implementing IHostedService.Start(), we would keep that and explicitly implement the new IHostedLifecycleService members:

API

namespace Microsoft.Extensions.Hosting
{
-   public abstract class BackgroundService : IDisposable, IHostedService

    // Note that IHostedLifecycleService implements IHostedService
+   public abstract class BackgroundService : IDisposable, IHostedLifecycleService

    // Keep this, but this method will no longer implement IHostedService.StartAsync
    public virtual Task StartAsync(CancellationToken cancellationToken);

+   Task IHostedLifecycleService.StartingAsync(CancellationToken cancellationToken); //noop
+   Task IHostedLifecycleService.StartAsync(CancellationToken cancellationToken); //noop
+   Task IHostedLifecycleService.StartedAsync(CancellationToken cancellationToken); //noop
+   Task IHostedLifecycleService.StoppingAsync(CancellationToken cancellationToken); //noop
    // We don't need to explictely implement StopAsync since the existing virtual is fine
+   Task IHostedLifecycleService.StoppedAsync(CancellationToken cancellationToken); //noop
}

Alternative

Note that by implementing the new callbacks, it prevents derived classes from also implementing them. Consider the alternative where the new callbacks are all added as virtuals, but this would be breaking since existing overrides of StartAsync will have pre- and post- behavior broken when calling base.StartAsync() since the base class implementation (BackgroundService) will no longer have any implementation:

namespace Microsoft.Extensions.Hosting
{
-   public abstract class BackgroundService : IDisposable, IHostedService
+   public abstract class BackgroundService : IDisposable, IHostedLifecycleService

+   public virtual Task StartingAsync(CancellationToken cancellationToken);
    public virtual Task StartAsync(CancellationToken cancellationToken);
+   public virtual Task StartingAsync(CancellationToken cancellationToken);
+   public virtual Task StoppingAsync(CancellationToken cancellationToken);
    public virtual Task StopAsync(CancellationToken cancellationToken);
+   public virtual Task StoppedAsync(CancellationToken cancellationToken);
}

Author:	steveharter
Assignees:	steveharter
Labels:	api-needs-work, area-Extensions-Hosting
Milestone:	-

[steveharter]

cc @halter73 @Tratcher @davidfowl @eerhardt @buyaa-n

[Tratcher]

This is a behavioral breaking change and makes a lot of assumptions about existing implementations. Please reiterate the rational here for discussion.

• These were started before the AspNetCore server; will they now be started after? That's problematic if my requests depend on these background services for anything.
• They were started in series; will they now be started in parallel? Introduces new concurrency and non-deterministic ordering.
• which prevents order-of-registration issues. Are there specific examples of these issues from customers?

[dersia]

FWIW, we use BackgroundService to preload data. this works well because this is executed before the server starts. So as @Tratcher said, this would break the behavior und I am not sure what this would do to our application.

[steveharter]

This is a behavioral breaking change and makes a lot of assumptions about existing implementations. Please reiterate the rational here for discussion.

Thanks for the detail - closing this issue.

However, based on internal discussion, ASP.NET should consider doing this for https://github.com/dotnet/aspnetcore/blob/main/src/Hosting/Hosting/src/GenericHost/GenericWebHostService.cs