Add WaitAsync functionality to Background Service

[Nick-Stanton]

Resolves problem in #36380 where Microsoft.Extensions.Hosting.Abstractions\src\BackgroundService.cs can create Tasks that appear likely to never complete. New functionality in .NET 6.0 allowed this to be cleaned up with Task.WaitAsync. Since this API does not exist in all configurations that the Extensions libraries target, the new desired behavior is simulated by a fix proposed by @Zenexer in versions without it.

The nested ConfigureAwait() calls don't look pretty, but they were required in order to be compliant with CA2007.

[ghost]

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

Issue Details

---

Resolves problem in #36380 where Microsoft.Extensions.Hosting.Abstractions\src\BackgroundService.cs can create Tasks that appear likely to never complete. New functionality in .NET 6.0 allowed this to be cleaned up with Task.WaitAsync. Since this API does not exist in all configurations that the Extensions libraries target, the new desired behavior is simulated by a fix proposed by @Zenexer in versions without it.

The nested ConfigureAwait() calls don't look pretty, but they were required in order to be compliant with CA2007.

Author:	Nick-Stanton
Assignees:	-
Labels:	area-Extensions-Hosting
Milestone:	-

[tarekgh]

Do we have a test exercise the affected functionality (especially on non net core)?

CC @stephentoub if interested to take a quick look at the change.

[eerhardt]

Like @tarekgh mentioned, it would be good to ensure we have a test that does something like:

1. Run a BackgroundService
2. Calls StopAsync on the service
3. Stopping the service will cancel the ExecuteAsync method, which should be either ignored or continue processing for longer than the shutdown timeout
4. The shutdown timeout should kick in and the processing in StopAsync here (that you are changing) should be canceled and the host should continue shutting down.

[Nick-Stanton]

Test are failing off of line 78. Here's a stack trace of some of the fails.

[eerhardt]

We were never actually awaiting the _executeTask before. Before we were just awaiting Task.WhenAny which successfully returns when either of those tasks completes (if that underlying Task is successful or not).

Basically we need to ignore that the task is getting cancelled. The first way I can think of to do that is to:

try
{
    await _executeTask.WaitAsync(cancellationToken).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
    // ignore cancellation exceptions since we are stopping the service and the cancellation is expected
}

But maybe there is a cleaner way to have have exception processing in the "happy path".

Another option is to not use the new API and instead unconditionally use the TaskCompletionSource approach even on NETCOREAPP.

[Zenexer]

We were never actually awaiting the _executeTask before.

The intention was to await _executeTask, though; the original code had a bug (#36380).

Edit: Looking at the source for WaitAsync, I'm fairly certain the NETCOREAPP and !NETCOREAPP paths are equivalent, other than the fact that the code for WaitAsync seems suboptimal (bool type param instead of object, inconsistent formatting--and why is it wrapped in a Timer? Is that important?)

[eerhardt]

The intention was to await _executeTask, though

Not really. The intention is to signal to the _executeTask that it should be stopped and wait for it to be stopped (or to timeout). If an exception is thrown by the _executeTask, we don't care about throwing it here - which is what await does. Exceptions coming from the ExecuteTask are handled here (which also ignores Cancellation exceptions):

runtime/src/libraries/Microsoft.Extensions.Hosting/src/Internal/Host.cs

Lines 85 to 111 in b13e6ef

	private async Task TryExecuteBackgroundServiceAsync(BackgroundService backgroundService)
	{
	// backgroundService.ExecuteTask may not be set (e.g. if the derived class doesn't call base.StartAsync)
	Task? backgroundTask = backgroundService.ExecuteTask;
	if (backgroundTask == null)
	{
	return;
	}
	try
	{
	await backgroundTask.ConfigureAwait(false);
	}
	catch (Exception ex)
	{
	// When the host is being stopped, it cancels the background services.
	// This isn't an error condition, so don't log it as an error.
	if (_stopCalled && backgroundTask.IsCanceled && ex is OperationCanceledException)
	{
	return;
	}
	_logger.BackgroundServiceFaulted(ex);
	if (_options.BackgroundServiceExceptionBehavior == BackgroundServiceExceptionBehavior.StopHost)
	{
	_logger.BackgroundServiceStoppingHost(ex);
	_applicationLifetime.StopApplication();

[Zenexer]

It looks like the behavior of BackgroundService handling has changed significantly from what it was when I opened #36380. @Nick-Stanton, you should keep in mind that my original suggestion was for .NET Core 3.0, and the behavior has changed since then. My suggestion is no longer valid with the current behavior. Line 82 in your current version is also erroneous with the current behavior: it shouldn't await twice; that was only valid when BackgroundService didn't expose _executeTask (the ExecuteTask property didn't exist).

[Zenexer]

Looking at the !NETCOREAPP behavior, this is actually a bit of a mess: if _executeTask is returned, it must not be awaited, but if tcs.Task is returned, it must be awaited (otherwise, the memory leak in #36380 returns). See line 82 below.

[eerhardt]

Maybe add a quick comment here explaining why we are ignoring this exception.

[eerhardt]

Suggested change

	await Task.WhenAny(_executeTask, tcs.Task).ConfigureAwait(false);
	await (await Task.WhenAny(_executeTask, tcs.Task).ConfigureAwait(false)).ConfigureAwait(false).;

I think since we are catching OperationCanceledException, we should also be awaiting the returned Task from awaiting Task.WhenAny here.

[eerhardt]

@stephentoub - do you have any opinion on this approach? The options I see are:

1. The current code.
2. Explicitly not awaiting the Task returned from awaiting Task.WhenAny, which will ignore the OperationCanceledException - because we never await the Task that got canceled.

var tcs = new TaskCompletionSource<object>();
using CancellationTokenRegistration registration = cancellationToken.Register(s => ((TaskCompletionSource<object>)s).SetCanceled(), tcs);
await Task.WhenAny(_executeTask, tcs.Task).ConfigureAwait(false);

I'm not 100% sure which is the best option. I thought the current proposed approach was more explicit as to what is happening: An OperationCanceledException is being thrown, and we are explicitly ignoring it.

Performance here isn't a major concern because these BackgroundServices don't get Start/Stopped very often (typically just at the beginning and end of the process), so I didn't think the exception handling would make a perf difference.

[eerhardt]

I spoke with @stephentoub offline. His recommendation was to keep it simple, and just use option (2) above on all TFMs. I also recommend adding a comment that we explicitly aren't awaiting the _executeTask here, just the Task.WhenAny because cancelling the _executeTask will always throw an OperationCanceledException and we are just ignoring that exception.

Futher, @stephentoub showed me that in "normal" hosting scenarios, we don't actually have a leak with the current code. The Task created from Task.Delay(Timeout.Infinite, cancellationToken) will be available for GC once the cancellationToken passed in goes away. This is because Timeout.Infinite is special cased to not create a Timer:

runtime/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs

Lines 5619 to 5629 in 6294858

	if (millisecondsDelay != Timeout.UnsignedInfinite) // no need to create the timer if it's an infinite timeout
	{
	_timer = new TimerQueueTimer(s_timerCallback, this, millisecondsDelay, Timeout.UnsignedInfinite, flowExecutionContext: false);
	if (IsCompleted)
	{
	// Handle rare race condition where the timer fires prior to our having stored it into the field, in which case
	// the timer won't have been cleaned up appropriately. This call to close might race with the Cleanup call to Close,
	// but Close is thread-safe and will be a nop if it's already been closed.
	_timer.Close();
	}
	}

Instead, the CancellationToken and the Task from Task.WhenAny are the things holding onto the Task. The latter will be available for GC once this method completes. So the only question is the lifetime of the CancellationToken.

In normal hosting scenarios, StopAsync gets called with a CancellationToken from a CancellationTokenSource that is disposed of after all the hosted services are stopped:

runtime/src/libraries/Microsoft.Extensions.Hosting/src/Internal/Host.cs

Lines 122 to 135 in 6294858

	using (var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cts.Token, cancellationToken))
	{
	CancellationToken token = linkedCts.Token;
	// Trigger IHostApplicationLifetime.ApplicationStopping
	_applicationLifetime.StopApplication();
	IList<Exception> exceptions = new List<Exception>();
	if (_hostedServices != null) // Started?
	{
	foreach (IHostedService hostedService in _hostedServices.Reverse())
	{
	try
	{
	await hostedService.StopAsync(token).ConfigureAwait(false);

In that case, there won't be a leak.

However, since this API is public, taking a fix here is the right thing to do since anyone can call this method with any CancellationToken - for example one that is long-lived.

[eerhardt]

LGTM. Thanks for fixing this!

[stephentoub]

Nit: we usually spell canceling (and canceled and canceller) with 1 L rather than 2. Cancellation is the only variant for which we use 2.

[Nick-Stanton]

Failures are tracked by #74952 and are unrelated to this change. Merging