Aspnetcore: Don't swallow IHostedService.StartAsync exceptions

[cwe1ss]

As discussed briefly in https://github.com/aspnet/Hosting/issues/1085#issuecomment-305717425 I propose that the HostedServiceExecutor should not catch exceptions in StartAsync.

By swallowing exceptions, the hosting-layer forces all services to be optional which is undesired IMO. By not swallowing exceptions we give services the option to decide this on its own. If a service is truly optional, it can swallow exceptions in its own StartAsync code.

I also think that StartAsync-exceptions should not be combined into an AggregateException. Instead it should just stop on the first exception.

One thing to consider is that right now this code is executed AFTER the server has been started. Not sure what effects this has when exceptions are no longer caught?!

I still hold the opinion that hosted services should be started before the server is started (a service could always decide to kick off another task in its StartAsync if it wants to be delayed) but this has also been discussed briefly in #1085 and declined.

[muratg]

@cwe1ss Would you like to send a PR? If so, we should discuss the approach here. cc @Tratcher

[muratg]

This would be a breaking change in 2.x. Our new feature, GenericHost does this already. We expect that WebHost implementation would be build on top of GenericHost by 3.0.0.

[sirajmansour]

@muratg could you please elaborate more on how is this a breaking change ?

[muratg]

@sirajmansour If we take this in, some application that were functional before could stop being so due to a new exception raised.

[sirajmansour]

Hmm, i thought this kind of change could be accepted since 2.1 is in preview, fair enough.

[sirajmansour]

Maybe allow to configure options for HostedServiceExecutor to allow to propagate exceptions, which would override the current behaviour that would remain as default ? That would be non-breaking and not so complex.

[muratg]

@sirajmansour We do not make breaking changes in minor releases (with some very rare exceptions).

We are not planning to add more features (such as configuration for this) to 2.1. I think in 3.0.0 we'll likely make this default though.

Do you have a really compelling scenario that you'd like to enable with this? Perhaps it can be achieved in some other way?

[sirajmansour]

Not really, it just seemed odd that it is swallowed. 3.0 it then 👍

[matteocontrini]

In my case I use an hosted service for doing some initialization of the application (like loading some resources from other services), so actually the fact that an exception allows the application to keep going is not ok. Could you confirm there's nothing planned before 3.0? Thanks

[Tratcher]

Correct.

[dotnetchris]

This is unfortunate to see. As a work around to this i'm going to do something like

```c#
public abstract class CriticalService : IHostedService
{
protected IServiceProvider Provider { get; }

protected CriticalProcess(IServiceProvider provider)
{
    Provider = provider;
}
public virtual LogDescription FatalEvent => LogDescription.FailedToStartWorker;
public virtual string FatalMessage => $"failed to construct {GetType()}";

public async Task StartAsync(CancellationToken cancellationToken)
{
    try
    {
       await StartAsync(cancellationToken, FatalEvent);
    }
    catch (Exception e)
    {
        FatalExit(e);
        throw;
    }
}

public abstract Task StartAsync(CancellationToken cancellationToken, LogDescription? @event);

public abstract Task StopAsync(CancellationToken cancellationToken);

protected void FatalExit(Exception ex)
{
    //TODO SMTP out a message or something
    Console.WriteLine($"Fatal, {FatalMessage}");
    Console.WriteLine(ex);

    int exitCode = (int)FatalEvent;

    Console.WriteLine($"Process exiting {-exitCode}, {FatalEvent}");

    Environment.ExitCode = -exitCode;
    //Environment.Exit(-exitCode);
    ((IApplicationLifetime) Provider.GetService(typeof(IApplicationLifetime)))
                                    .StopApplication();         
}

}

```

[smbecker]

Wouldn't it be better to request shutdown with IApplicationLifetime.StopApplication?

[dotnetchris]

@smbecker that would probably be more friendly, and maybe allow more time to collection additional error information (assuming if config is wrong to cause a critical failure, there's probably more config missing/wrong for other places too)

And i can still set the ExitCode to be negative

[muratg]

@davidfowl would you be OK getting a breaking change here in 3.0?

Folks, we may accept PRs for this. If you're interested please let me know!

[davidfowl]

I'll take this one for 3.0

[cwe1ss]

Thank you very much, David!

[srollinet]

Workaround for the 2.x version

    public static class ServiceCollectionUtils
    {
        public static void AddCriticalHostedService<TService, TImplementation>(this IServiceCollection services)
            where TService : class
            where TImplementation : class, IHostedService, TService
        {
            services.AddSingleton<TService, TImplementation>();
            services.AddHostedService<CriticalHostedServiceWrapper<TService>>();
        }

        private class CriticalHostedServiceWrapper<TService> : IHostedService
        {
            private readonly IHostedService _hostedService;
            private readonly ILogger _logger;

            public CriticalHostedServiceWrapper(TService hostedService, ILoggerFactory loggerFactory)
            {
                _hostedService = (IHostedService)hostedService;
                _logger = loggerFactory.CreateLogger(_hostedService.GetType());
            }

            public async Task StartAsync(CancellationToken cancellationToken)
            {
                try
                {
                    await _hostedService.StartAsync(cancellationToken);
                }
                catch (Exception e)
                {
                    _logger.LogCritical(e, "Cannot start critical service. The application will exit.");
                    Environment.Exit(1);
                }
            }

            public Task StopAsync(CancellationToken cancellationToken)
            {
                return _hostedService.StopAsync(cancellationToken);
            }
        }
    }

[AsValeO]

Looks like behavior still the same in 3.0.

[davidfowl]

Update to use the generic host?

[AsValeO]

It works with generic host. Damn, I'm so far behind..