Review feedback for IApplicationLifetimeEvents

- Renamed the type to IHostedService and added Start and Stop.
- Split up the IHostedService execution and IApplicationLifetime to avoid
circular references
- Trigger IHostedService.Start after starting the server
- Trigger IHostedService.Stop before disposing the service provider

#895 #894

src/Microsoft.AspNetCore.Hosting.Abstractions/IHostedService.cs

@@ -4,28 +4,20 @@
 namespace Microsoft.AspNetCore.Hosting
 {
     /// <summary>
-    /// Allows consumers to perform cleanup during a graceful shutdown.
+    /// Defines methods for objects that are managed by the host.
     /// </summary>
-    public interface IApplicationLifetimeEvents
+    public interface IHostedService
     {
         /// <summary>
-        /// Triggered when the application host has fully started and is about to wait
+        /// Triggered when the application host has fully started and the server is waiting
-        /// for a graceful shutdown.
+        /// for requests.
         /// </summary>
-        void OnApplicationStarted();
+        void Start();
 
         /// <summary>
         /// Triggered when the application host is performing a graceful shutdown.
         /// Requests may still be in flight. Shutdown will block until this event completes.
         /// </summary>
-        void OnApplicationStopping();
+        void Stop();
-
-        /// <summary>
-        /// Triggered when the application host is performing a graceful shutdown.
-        /// All requests should be complete at this point. Shutdown will block
-        /// until this event completes.
-        /// </summary>
-        void OnApplicationStopped();
-
     }
 }

src/Microsoft.AspNetCore.Hosting/Internal/ApplicationLifetime.cs

@@ -17,13 +17,11 @@ namespace Microsoft.AspNetCore.Hosting.Internal
         private readonly CancellationTokenSource _startedSource = new CancellationTokenSource();
         private readonly CancellationTokenSource _stoppingSource = new CancellationTokenSource();
         private readonly CancellationTokenSource _stoppedSource = new CancellationTokenSource();
-        private readonly IEnumerable<IApplicationLifetimeEvents> _handlers = Enumerable.Empty<IApplicationLifetimeEvents>();
         private readonly ILogger<ApplicationLifetime> _logger;
 
-        public ApplicationLifetime(ILogger<ApplicationLifetime> logger, IEnumerable<IApplicationLifetimeEvents> handlers)
+        public ApplicationLifetime(ILogger<ApplicationLifetime> logger)
         {
             _logger = logger;
-            _handlers = handlers;
         }
 
         /// <summary>
@@ -57,7 +55,7 @@ namespace Microsoft.AspNetCore.Hosting.Internal
             {
                 try
                 {
-                    ExecuteHandlers(_stoppingSource, handler => handler.OnApplicationStopping());
+                    ExecuteHandlers(_stoppingSource);
                 }
                 catch (Exception ex)
                 {
@@ -75,7 +73,7 @@ namespace Microsoft.AspNetCore.Hosting.Internal
         {
             try
             {
-                ExecuteHandlers(_startedSource, handler => handler.OnApplicationStarted());
+                ExecuteHandlers(_startedSource);
             }
             catch (Exception ex)
             {
@@ -92,7 +90,7 @@ namespace Microsoft.AspNetCore.Hosting.Internal
         {
             try
             {
-                ExecuteHandlers(_stoppedSource, handler => handler.OnApplicationStopped());
+                ExecuteHandlers(_stoppedSource);
             }
             catch (Exception ex)
             {
@@ -102,7 +100,7 @@ namespace Microsoft.AspNetCore.Hosting.Internal
             }
         }
 
-        private void ExecuteHandlers(CancellationTokenSource cancel, Action<IApplicationLifetimeEvents> callback)
+        private void ExecuteHandlers(CancellationTokenSource cancel)
         {
             // Noop if this is already cancelled
             if (cancel.IsCancellationRequested)
@@ -127,24 +125,6 @@ namespace Microsoft.AspNetCore.Hosting.Internal
                 exceptions.Add(ex);
             }
 
-            // Run the handlers
-            foreach (var handler in _handlers)
-            {
-                try
-                {
-                    callback(handler);
-                }
-                catch (Exception ex)
-                {
-                    if (exceptions == null)
-                    {
-                        exceptions = new List<Exception>();
-                    }
-
-                    exceptions.Add(ex);
-                }
-            }
-
             // Throw an aggregate exception if there were any exceptions
             if (exceptions != null)
             {

src/Microsoft.AspNetCore.Hosting/Internal/HostedServiceExecutor.cs

@@ -0,0 +1,70 @@
+using System;
+using System.Collections.Generic;
+using Microsoft.Extensions.Logging;
+
+namespace Microsoft.AspNetCore.Hosting.Internal
+{
+    public class HostedServiceExecutor
+    {
+        private readonly IEnumerable<IHostedService> _services;
+        private readonly ILogger<HostedServiceExecutor> _logger;
+
+        public HostedServiceExecutor(ILogger<HostedServiceExecutor> logger, IEnumerable<IHostedService> services)
+        {
+            _logger = logger;
+            _services = services;
+        }
+
+        public void Start()
+        {
+            try
+            {
+                Execute(service => service.Start());
+            }
+            catch (Exception ex)
+            {
+                _logger.ApplicationError(LoggerEventIds.HostedServiceStartException, "An error occurred starting the application", ex);
+            }
+        }
+
+        public void Stop()
+        {
+            try
+            {
+                Execute(service => service.Stop());
+            }
+            catch (Exception ex)
+            {
+                _logger.ApplicationError(LoggerEventIds.HostedServiceStopException, "An error occurred stopping the application", ex);
+            }
+        }
+
+        private void Execute(Action<IHostedService> callback)
+        {
+            List<Exception> exceptions = null;
+
+            foreach (var service in _services)
+            {
+                try
+                {
+                    callback(service);
+                }
+                catch (Exception ex)
+                {
+                    if (exceptions == null)
+                    {
+                        exceptions = new List<Exception>();
+                    }
+
+                    exceptions.Add(ex);
+                }
+            }
+
+            // Throw an aggregate exception if there were any exceptions
+            if (exceptions != null)
+            {
+                throw new AggregateException(exceptions);
+            }
+        }
+    }
+}

src/Microsoft.AspNetCore.Hosting/Internal/LoggerEventIds.cs

@@ -13,5 +13,7 @@ namespace Microsoft.AspNetCore.Hosting.Internal
         public const int ApplicationStartupException = 6;
         public const int ApplicationStoppingException = 7;
         public const int ApplicationStoppedException = 8;
+        public const int HostedServiceStartException = 9;
+        public const int HostedServiceStopException = 10;
     }
 }

src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs

@@ -28,6 +28,7 @@ namespace Microsoft.AspNetCore.Hosting.Internal
         private readonly IServiceCollection _applicationServiceCollection;
         private IStartup _startup;
         private ApplicationLifetime _applicationLifetime;
+        private HostedServiceExecutor _hostedServiceExecutor;
 
         private readonly IServiceProvider _hostingServiceProvider;
         private readonly WebHostOptions _options;
@@ -68,6 +69,7 @@ namespace Microsoft.AspNetCore.Hosting.Internal
             _applicationServiceCollection = appServices;
             _hostingServiceProvider = hostingServiceProvider;
             _applicationServiceCollection.AddSingleton<IApplicationLifetime, ApplicationLifetime>();
+            _applicationServiceCollection.AddSingleton<HostedServiceExecutor>();
         }
 
         public IServiceProvider Services
@@ -104,11 +106,17 @@ namespace Microsoft.AspNetCore.Hosting.Internal
             Initialize();
 
             _applicationLifetime = _applicationServices.GetRequiredService<IApplicationLifetime>() as ApplicationLifetime;
+            _hostedServiceExecutor = _applicationServices.GetRequiredService<HostedServiceExecutor>();
             var diagnosticSource = _applicationServices.GetRequiredService<DiagnosticSource>();
             var httpContextFactory = _applicationServices.GetRequiredService<IHttpContextFactory>();
             Server.Start(new HostingApplication(_application, _logger, diagnosticSource, httpContextFactory));
 
+            // Fire IApplicationLifetime.Started
             _applicationLifetime?.NotifyStarted();
+
+            // Fire IHostedService.Start
+            _hostedServiceExecutor.Start();
+
             _logger.Started();
         }
 
@@ -243,9 +251,17 @@ namespace Microsoft.AspNetCore.Hosting.Internal
         public void Dispose()
         {
             _logger?.Shutdown();
+
+            // Fire IApplicationLifetime.Stopping
             _applicationLifetime?.StopApplication();
+
+            // Fire the IHostedService.Stop
+            _hostedServiceExecutor?.Stop();
+
             (_hostingServiceProvider as IDisposable)?.Dispose();
             (_applicationServices as IDisposable)?.Dispose();
+
+            // Fire IApplicationLifetime.Stopped
             _applicationLifetime?.NotifyStopped();
 
             HostingEventSource.Log.HostStop();

test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs

@@ -316,16 +316,13 @@ namespace Microsoft.AspNetCore.Hosting
                 host.Dispose();
                 Assert.True(events1[1]);
                 Assert.True(events2[1]);
-                Assert.True(events1[2]);
-                Assert.True(events2[2]);
             }
         }
 
         [Fact]
-        public void WebHostStopCallbacksDontMultipleTimes()
+        public void WebHostStopApplicationDoesNotFireStopOnHostedService()
         {
             var stoppingCalls = 0;
-            var stoppedCalls = 0;
 
             var host = CreateBuilder()
                 .UseServer(this)
@@ -340,31 +337,96 @@ namespace Microsoft.AspNetCore.Hosting
                         stoppingCalls++;
                     };
 
-                    Action stopped = () =>
+                    services.AddSingleton<IHostedService>(new DelegateHostedService(started, stopping));
-                    {
-                        stoppedCalls++;
-                    };
-
-                    services.AddSingleton<IApplicationLifetimeEvents>(new DelegateLifetimeEvents(started, stopping, stopped));
                 })
                 .Build();
             var lifetime = host.Services.GetRequiredService<IApplicationLifetime>();
             lifetime.StopApplication();
-            lifetime.StopApplication();
-            lifetime.StopApplication();
 
             using (host)
             {
                 host.Start();
-                host.Dispose();
 
-                Assert.Equal(1, stoppingCalls);
+                Assert.Equal(0, stoppingCalls);
-                Assert.Equal(1, stoppedCalls);
             }
         }
 
         [Fact]
-        public void WebHostNotifiesAllIApplicationLifetimeEventsCallbacksAndIApplicationLifetimeCallbacksEvenIfTheyThrow()
+        public void HostedServiceCanInjectApplicationLifetime()
+        {
+            var host = CreateBuilder()
+                   .UseServer(this)
+                   .ConfigureServices(services =>
+                   {
+                       services.AddSingleton<IHostedService, TestHostedService>();
+                   })
+                   .Build();
+            var lifetime = host.Services.GetRequiredService<IApplicationLifetime>();
+            lifetime.StopApplication();
+
+
+            host.Start();
+            var svc = (TestHostedService)host.Services.GetRequiredService<IHostedService>();
+            Assert.True(svc.StartCalled);
+            host.Dispose();
+            Assert.True(svc.StopCalled);
+        }
+
+        [Fact]
+        public void HostedServiceStartNotCalledIfWebHostNotStarted()
+        {
+            var host = CreateBuilder()
+                   .UseServer(this)
+                   .ConfigureServices(services =>
+                   {
+                       services.AddSingleton<IHostedService, TestHostedService>();
+                   })
+                   .Build();
+            var lifetime = host.Services.GetRequiredService<IApplicationLifetime>();
+            lifetime.StopApplication();
+
+            var svc = (TestHostedService)host.Services.GetRequiredService<IHostedService>();
+            Assert.False(svc.StartCalled);
+            host.Dispose();
+            Assert.False(svc.StopCalled);
+        }
+
+        [Fact]
+        public void WebHostDisposeApplicationFiresStopOnHostedService()
+        {
+            var stoppingCalls = 0;
+            var startedCalls = 0;
+
+            var host = CreateBuilder()
+                .UseServer(this)
+                .ConfigureServices(services =>
+                {
+                    Action started = () =>
+                    {
+                        startedCalls++;
+                    };
+
+                    Action stopping = () =>
+                    {
+                        stoppingCalls++;
+                    };
+
+                    services.AddSingleton<IHostedService>(new DelegateHostedService(started, stopping));
+                })
+                .Build();
+            var lifetime = host.Services.GetRequiredService<IApplicationLifetime>();
+            using (host)
+            {
+                host.Start();
+                host.Dispose();
+
+                Assert.Equal(1, startedCalls);
+                Assert.Equal(1, stoppingCalls);
+            }
+        }
+
+        [Fact]
+        public void WebHostNotifiesAllIApplicationLifetimeEventsCallbacksAndIHostedServicesEvenIfTheyThrow()
         {
             bool[] events1 = null;
             bool[] events2 = null;
@@ -381,7 +443,6 @@ namespace Microsoft.AspNetCore.Hosting
 
             var started = RegisterCallbacksThatThrow(applicationLifetime.ApplicationStarted);
             var stopping = RegisterCallbacksThatThrow(applicationLifetime.ApplicationStopping);
-            var stopped = RegisterCallbacksThatThrow(applicationLifetime.ApplicationStopped);
 
             using (host)
             {
@@ -393,9 +454,6 @@ namespace Microsoft.AspNetCore.Hosting
                 Assert.True(events1[1]);
                 Assert.True(events2[1]);
                 Assert.True(stopping.All(s => s));
-                Assert.True(events1[2]);
-                Assert.True(events2[2]);
-                Assert.True(stopped.All(s => s));
             }
         }
 
@@ -654,7 +712,7 @@ namespace Microsoft.AspNetCore.Hosting
 
         private static bool[] RegisterCallbacksThatThrow(IServiceCollection services)
         {
-            bool[] events = new bool[3];
+            bool[] events = new bool[2];
 
             Action started = () =>
             {
@@ -668,13 +726,7 @@ namespace Microsoft.AspNetCore.Hosting
                 throw new InvalidOperationException();
             };
 
-            Action stopped = () =>
+            services.AddSingleton<IHostedService>(new DelegateHostedService(started, stopping));
-            {
-                events[2] = true;
-                throw new InvalidOperationException();
-            };
-
-            services.AddSingleton<IApplicationLifetimeEvents>(new DelegateLifetimeEvents(started, stopping, stopped));
 
             return events;
         }
@@ -722,24 +774,43 @@ namespace Microsoft.AspNetCore.Hosting
             }
         }
 
-        private class DelegateLifetimeEvents : IApplicationLifetimeEvents
+        private class TestHostedService : IHostedService
+        {
+            private readonly IApplicationLifetime _lifetime;
+
+            public TestHostedService(IApplicationLifetime lifetime)
+            {
+                _lifetime = lifetime;
+            }
+
+            public bool StartCalled { get; set; }
+            public bool StopCalled { get; set; }
+
+            public void Start()
+            {
+                StartCalled = true;
+            }
+
+            public void Stop()
+            {
+                StopCalled = true;
+            }
+        }
+
+        private class DelegateHostedService : IHostedService
         {
             private readonly Action _started;
             private readonly Action _stopping;
-            private readonly Action _stopped;
 
-            public DelegateLifetimeEvents(Action started, Action stopping, Action stopped)
+            public DelegateHostedService(Action started, Action stopping)
             {
                 _started = started;
                 _stopping = stopping;
-                _stopped = stopped;
             }
 
-            public void OnApplicationStarted() => _started();
+            public void Start() => _started();
 
-            public void OnApplicationStopped() => _stopped();
+            public void Stop() => _stopping();
-
-            public void OnApplicationStopping() => _stopping();
         }
 
         private class StartInstance : IDisposable