Extensions: Unhandled exception in IHostedService does not stop the host

[jdwoolcock]

Describe the bug

If an unhandled exception occurs inside an implementation of a Microsoft.Extensions.Hosting.BackgroundService I would expect it to exit the application.

Using the generic host builder in a .net core 2.1 console application, I register hosted services that override BackgroundService. Each service returns a task that performs a long running operation. If an unhandled exception is thrown inside the task the host is not exited but continues to run until a Ctrl^C is triggered.

To Reproduce

Steps to reproduce the behavior:

1. Using version '2.2.0' of package 'Microsoft.Extensions.Hosting'
2. Run this code

internal class Program
    {
        public static async Task Main(string[] args)
        {
            var host = new HostBuilder()
                .ConfigureServices(services => services.AddHostedService<TestService>())
                .Build();

            await host.RunAsync();
        }
    }

    public class TestService : BackgroundService
    {
        protected override async Task ExecuteAsync(CancellationToken stoppingToken)
        {
            try
            {
                await Task.Run(() =>
                    {
                        try
                        {
                            Thread.Sleep(5000);                        
                            throw new SystemException("Something went wrong!");
                        }
                        catch (Exception e)
                        {
                            Console.WriteLine(e);
                            throw;
                        }
                    }, stoppingToken);
            }
            catch (Exception e)
            {
                Console.WriteLine(e);
                throw;
            }
        }
    }

1. See error
   The exception is correctly handled and rethrown but is consumed by the host.

Application started. Press Ctrl+C to shut down.
System.SystemException: Something went wrong!
   at TestHostBuilder.TestService.<>c.<ExecuteAsync>b__0_0() in Program.cs:line 31
Hosting environment: Production
Content root path: TestHostBuilder\bin\Debug\netcoreapp2.1\
System.SystemException: Something went wrong!
   at TestHostBuilder.TestService.<>c.<ExecuteAsync>b__0_0() in Program.cs:line 31
   at System.Threading.Tasks.Task`1.InnerInvoke()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot)
--- End of stack trace from previous location where exception was thrown ---
   at TestHostBuilder.TestService.ExecuteAsync(CancellationToken stoppingToken) in Program.cs:line 27

Expected behavior

I expected the host to exit on receiving the unhandled exception.
See example below

Additional context

What I don't understand is why when I run the following hosted service
using "System.Reactive" Version="4.1.3" the exception does bubble up to the host and the application does exit.

public class TestService2 : BackgroundService
        {
            protected override async Task ExecuteAsync(CancellationToken stoppingToken)
            {
                try
                {
                    await Task.Run(() =>
                    {
                        try
                        {
                            Observable.Interval(TimeSpan.FromSeconds(1))
                                .Subscribe(x =>
                                {
                                    if (x > 4)
                                    {
                                        throw new SystemException("Something went wrong!");
                                    }
                                });
                        }
                        catch (Exception e)
                        {
                            Console.WriteLine(e);
                            throw;
                        }
                    }, stoppingToken);
                }
                catch (Exception e)
                {
                    Console.WriteLine(e);
                    throw;
                }
            }
        }

Output

Application started. Press Ctrl+C to shut down.
Hosting environment: Production
Content root path: TestHostBuilder\bin\Debug\netcoreapp2.1\

Unhandled Exception: System.SystemException: Something went wrong!
   at TestHostBuilder.TestService.TestService2.<>c.<ExecuteAsync>b__0_1(Int64 x) in Program.cs:line 66
   at System.Reactive.AnonymousSafeObserver`1.OnNext(T value) in D:\a\1\s\Rx.NET\Source\src\System.Reactive\AnonymousSafeObserver.cs:line 44
   at System.Reactive.Concurrency.Scheduler.<>c__67`1.<SchedulePeriodic>b__67_0(ValueTuple`2 t) in D:\a\1\s\Rx.NET\Source\src\System.Reactive\Concurrency\Scheduler.Services.Emulation.cs:line 79
   at System.Reactive.Concurrency.DefaultScheduler.PeriodicallyScheduledWorkItem`1.<>c.<Tick>b__5_0(PeriodicallyScheduledWorkItem`1 closureWorkItem) in D:\a\1\s\Rx.NET\Source\src\System.Reactive\Concurrency\DefaultScheduler.cs:line 127
   at System.Reactive.Concurrency.AsyncLock.Wait(Object state, Delegate delegate, Action`2 action) in D:\a\1\s\Rx.NET\Source\src\System.Reactive\Concurrency\AsyncLock.cs:line 93
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.TimerQueueTimer.CallCallback()
   at System.Threading.TimerQueueTimer.Fire()
   at System.Threading.TimerQueue.FireNextTimers()

C:\Program Files\dotnet\dotnet.exe (process 38600) exited with code 0.
To automatically close the console when debugging stops, enable Tools->Options->Debugging->Automatically close the console when debugging stops.
Press any key to close this window . . .

I believe the issue may be related to the implementation of BackgroundService

        /// <summary>
        /// Triggered when the application host is ready to start the service.
        /// </summary>
        /// <param name="cancellationToken">Indicates that the start process has been aborted.</param>
        public virtual Task StartAsync(CancellationToken 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;
        }

If I update the code to always return the executing task and not Task.CompletedTaskthen it works as expected using both TestService and TestService2.

// <summary>
        /// Triggered when the application host is ready to start the service.
        /// </summary>
        /// <param name="cancellationToken">Indicates that the start process has been aborted.</param>
        public virtual Task StartAsync(CancellationToken cancellationToken)
        {
            // Store the task we're executing
            _executingTask = ExecuteAsync(_stoppingCts.Token);

            return _executingTask;
        }

[davidfowl]

What I don't understand is why when I run the following hosted service
using "System.Reactive" Version="4.1.3" the exception does bubble up to the host and the application does exit.

The application exists because entire process is crashing. The host isn't gracefully shutting down, the process is just dying. This happens when using timers or the thread pool directly without a try catch.

If I update the code to always return the executing task and not Task.CompletedTaskthen it works as expected using both TestService and TestService2.

That's incorrect because it stops the host from starting. Other hosted services will fail to run. StartAsync is about starting or kicking off the service, not waiting on execution to complete. Nothing is tracking your background service loop per se.

I'm not sure we can do anything here by default other than optionally pass a service provider to the base background service base class so it can communicate with the host when ExecuteAsync fails.

You can work around this today by injecting an IApplicationLifetime and calling StopApplication at the appropriate time.

cc @glennc Any thoughts on this? It's not an unreasonable expectation.

[glennc]

Do you also want the application to exit when the work is done?

I think there is a category of app that we aren't really supporting very well yet, which is the "one and done" or more traditional console app that runs, does some work, and exists. I want to know if that's the case for this one.

If it is, then I think it's entirely reasonable that as we build out support for that scenario we take this kind of scenario into account and have an exception exit the process if it's unhandled.

[davidfowl]

I think this is more about being able to detect those failures. We don’t log by default either. I’m not sure if a single bad service should kill the process?

[jdwoolcock]

I'm running the application as a systemd service on Linux. There are many
hosted services some of which run and then complete and some that run
indefinitely or until the cancellation token is received.

If a fatal exception occurs that I can't recover from I was hoping to catch
it, log it and then re throw it causing the application to die and systemd
to then restart the process.

On Thu, 13 Jun 2019, 21:11 David Fowler, [e-mail] wrote:

I think this is more about being able to detect those failures. We don’t
log by default either. I’m not sure if a single bad service should kill the
process?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/aspnet/Extensions/issues/1836?email_source=notifications&email_token=AHGT2PBUHTCFP7CZJ7HQ33TP2KSXTA5CNFSM4HXXAEF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXU4TMI#issuecomment-501860785,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHGT2PCMJCQN7XDN7BPTCX3P2KSXTANCNFSM4HXXAEFQ
.

[glennc]

Wouldn't it be strictly better to handle the error and tell the app to shutdown gracefully instead of using unhandled exceptions to do it for you? For example, if you have a second hosted service in the same process shouldn't it get a chance to gracefully complete instead of being torn down? In which case injecting IApplicationLifetime and using it to stop the app seems like a better way to do this.

How are you implementing the applications that run and then complete? Are they using HostedServices? How do you shut down those?

@davidfowl I had a bit thing written about single hosted services taking down a process, but I didn't post it since I wasn't sure of the scenario :).

[andriysavin]

I think the confusion comes from how BackgroundService API looks like. The protected override async Task ExecuteAsync(CancellationToken stoppingToken) looks similar to many other application models, so one implies there is some code which awaits it and thus does all what is expected from awaiting (crash the app with unhandled exception in particular). The method name only assures in this, while from the host perspective it essentially behaves as StartAsync. It becomes obvious how things really work when you look at BackgroundService source code, and the explanation provided in this thread sounds reasonable. But now you have to think how to implement all that error handling yourself (using IApplicationLifetime etc.), and do that every time you need to implement a new service (ok, another base class can be created for code sharing), or new app or project (ok, a shared project/package can be created with the base class).

Ideally, I would expect an out of the box implementation of that in the form of, again, another base class derived from BackgroundService (or any other approach, because adding IApplicationLifetime to the base class constructor will make the API a bit more complicated).

[sitepodmatt]

Adding IApplicaitonLifetime to a base class constructor is still nasty. How do you get exit error code set to something other than 0?

[MV10]

@sitepodmatt Setting Environment.ExitCode works.

[sitepodmatt]

@MV10. Thanks. That does work although it does feel out-of-band hacky, it would be nice to signal to StopApplication that an error has occured and for that to render an appriorate error code depending on the hosting situation, I guess the lack of first class exit codes is pay back to early 2000s when people deployed on Windows and remember stuff like services.msc.

[davidfowl]

The problem is that stopping the host doesn't mean the process will exit. While that's the most common case, it isn't the only one. Having a way to signal failure is reasonable but would require a new API in a future version of .NET Core.

[sitepodmatt]

@davidfowl that's a good point I hadn't considered. Is this planned for 5.0 or is that feature locked already?

[davidfowl]

@davidfowl that's a good point I hadn't considered. Is this planned for 5.0 or is that feature locked already?

Nothing is planned but there are multiple feature requests:

• Add a new primitive that will cause the host to stop on failure.
• Add an API that allows an application to stop the host with an Exception (and expose it on the Host as well)

Out of scope:

• Exceptions that are thrown on background threads or unobserved tasks.
• Setting the exit code on host stop. This should be done by the application developer IMO.

[CNHume]

I was also wondering how one can request that the Host stop everything, clean up and exit. (I am using _Release 3.1.3_ of the .Net Core SDK.)

I had hoped that the OperationCanceledException could be caught outside the top-level Run():

  var host = CreateHostBuilder(args).Build();
  try {
    await host.RunAsync();
  }
  catch (OperationCanceledException) {
    // Expected after the worker performs:
    // StopAsync(cancellationToken);
    // cancellationToken.ThrowIfCancellationRequested();
  }

However, the Host quietly sinks all exceptions.

There are cases when one may want Host to continue, protecting sibling Tasks from each other; but there are also cases where one may reasonably want the Host to shut down.

I agree that one may just want a simple and direct path that the Host recognize when all of its workers have exited; and then exit itself.

Simply put: How do one or more of the workers

Press Ctrl+C to shut down?

Perhaps my answer lies somewhere in Hosting.Lifetime

[anurse]

Triage: circling back to the original issue, it's intended that BackgroundServices not bring down the app when they fail, so I'm closing this as by design. Do feel free to go to https://github.com/dotnet/runtime (the new home for these packages) to file bugs for other suggestions that have come up on this thread, but we don't plan to change the originally-discussed behavior.

[CNHume]

Hello Andrew,
This reaction comes as a big disappointment.
I am sorry to have to say this; but a"by design" response seems like a bureaucratic cop-out.
It is not an acceptable response in this case. 

Programmers making use of this ASP.Net Core hosted service capability need interfaces that will allow requesting the service to exit gracefully, under program control.
At a minimum, the service should be made smart enough to exit when there is no more work being done.

The implementation remains poorly designed and incomplete without some means to exit.
Furthermore, quietly eating exceptions is poor practice.
Please have someone take a closer look into this issue.
Thank you,Christopher Hume

On Wed, Apr 29, 2020 at 4:06 PM, Andrew [e-mail] wrote:

Closed #1836.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.