When awaiting an async method from ExecuteAsync which throws an unhandled exception, the service hangs

[hognevevle]

Describe the bug

In a .Net Core Worker Service, whenever an exception is thrown inside an async method, and you happened to not surround the method invocation in a try/catch, the application will simply hang when it hits the exception -- giving no sign that an exception has occured at all.

The same code applied within a simple Console Application does indeed work as expected, and you get the normal "unhandled exception" behaviour.

To Reproduce

Run sample worker service available at https://github.com/hognevevle/WorkerServicePlayground

This project was created using the Worker Service project template, and my only changes are done inside Worker.cs.

Expected behavior

The exception should be propagated up to the caller, and not simply disappear.

Screenshots

https://drive.google.com/file/d/1VCfrJY_a45gJ17uQXwGAq_krc8n-iteB/view?usp=drivesdk

Additional context

.NET Core SDK (reflecting any global.json):
 Version:   3.1.100
 Commit:    cd82f021f4

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18362
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\3.1.100\

Host (useful for support):
  Version: 3.1.0
  Commit:  65f04fb6db

.NET Core SDKs installed:
  3.0.101 [C:\Program Files\dotnet\sdk]
  3.1.100 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.14 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.14 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.0.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.14 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.0.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.0.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download```

[analogrelay]

the application will simply hang

A background service throwing an error during ExecuteAsync doesn't cause the app to shut down. That's intentional behavior. If you want that behavior, you'll have to try..catch and use the IApplicationLifetime.

giving no sign that an exception has occured at all.

We could consider logging the exception as it occurs (today it should be logged or manifested somehow when the host stops).

[Tratcher]

The exception should be propagated up to the caller, and not simply disappear.

Which caller? Program.Main?

[hognevevle]

@Tratcher Good question, for which I don't have the answer.

This screen recording might help clarify the issue at hand: https://drive.google.com/file/d/1gdMzXwQi2VzWP1OUivYgiY-yyDELpDGB/view

As you can see, any exception thrown synchronously will crash the service (which is what I'd expect). However, when thrown in combination with some asynchronous operation it will just silently hang.

I feel the behaviour should have been consistent in all of these cases.

[analogrelay]

As you can see, any exception thrown synchronously will crash the service

This is because StartAsync is set up to return the faulted task immediately if it's already faulted by the time it's returned by ExecuteAsync. It's likely this would be resolved by https://github.com/aspnet/Extensions/issues/2149

However, when thrown in combination with some asynchronous operation it will just silently hang.

This is behavior I'd expect. In general, BackgroundServices do not crash the process when they fail by design.

[rynowak]

We should really make sure that these exceptions are at least logged. I've been helping @stevejgordon with putting together some worker samples, and it's a painful lesson that everyone using worker seems to have to learn once.

In general if the framework is calling user-code we should make sure the exception is at least logged - I think there's a bigger discussion to be had about whether an unhandled exception from an IHostedService should be treated as a crash - or maybe that should be shown in docs. I say these things from the point of view of "we provide a template that gives you a single background service" - and if you crash out from that service you have no way of knowing.

[analogrelay]

This is a reasonable candidate for 5.0, but it'll be up to the runtime team to prioritize for sure :). The idea here would be to have BackgroundService.StartAsync set up the call to ExecuteAsync so that any exceptions thrown by it are logged and then rethrown.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

[eerhardt]

I investigated this issue and here is what I found:

There is a problem that there is no good location to do the logging. When the ExecutingTask is not finished in StartAsync, the BackgroundService class holds privately on to the returned ExecutingTask.

runtime/src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs

Lines 13 to 15 in a52f237

	public abstract class BackgroundService : IHostedService, IDisposable
	{
	private Task _executingTask;

And in StartAsync, it returns Task.CompletedTask saying that the service is done starting:

runtime/src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs

Lines 30 to 46 in a52f237

	public virtual Task StartAsync(CancellationToken cancellationToken)
	{
	// Create linked token to allow cancelling executing task from provided token
	_stoppingCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
	// Store the task we're executing
	_executingTask = ExecuteAsync(_stoppingCts.Token);
	// If the task is completed then return it, this will bubble cancellation and failure to the caller
	if (_executingTask.IsCompleted)
	{
	return _executingTask;
	}
	// Otherwise it's running
	return Task.CompletedTask;
	}

We could add a .ContinueWith on the _executingTask and check if an exception was thrown. However, there is nothing we can do in BackgroundService because it doesn't contain a Logger of any kind. so that's a dead end.

The more correct place for the logging to occur would be in the Host that starts the task. The Host already contains a logger, so it can simply log any exception to it:

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

Lines 51 to 55 in a52f237

	foreach (IHostedService hostedService in _hostedServices)
	{
	// Fire IHostedService.Start
	await hostedService.StartAsync(combinedCancellationToken).ConfigureAwait(false);
	}

However, the Host doesn't have access to the _executingTask, since BackgroundService doesn't expose it, and the Task returned from hostedService.StartAsync is the CompletedTask returned from BackgroundService.StartAsync above. So the Host has no way of knowing anything about the ExecutingTask.

We could do some hack to make this work in 5.0, but the real fix would be to expose the ExecutingTask on BackgroundService. This has come up before: #35991. So we will need to wait for that - which will need to go through API approval, which won't happen until 6.0.

[GSPP]

 if (_executingTask.IsCompleted) 
 { 
     return _executingTask; 
 } 

The _executingTask might fail immediately after this check. The check is probabilistic. It does not reliably address the case that the _executingTask failed. But it does add an additional case to the logic which alters behavior just sometimes. If this check is removed, nothing functional is lost but behavior becomes more uniform.