From fe9075d36003691f151d502dc23eada915cfc76f Mon Sep 17 00:00:00 2001 From: David Fowler Date: Mon, 5 Dec 2016 23:13:50 -0800 Subject: [PATCH] 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 --- ...ionLifetimeEvents.cs => IHostedService.cs} | 20 +-- .../Internal/ApplicationLifetime.cs | 30 +--- .../Internal/HostedServiceExecutor.cs | 70 +++++++++ .../Internal/LoggerEventIds.cs | 2 + .../Internal/WebHost.cs | 16 +++ .../WebHostTests.cs | 135 +++++++++++++----- 6 files changed, 202 insertions(+), 71 deletions(-) rename src/Microsoft.AspNetCore.Hosting.Abstractions/{IApplicationLifetimeEvents.cs => IHostedService.cs} (52%) create mode 100644 src/Microsoft.AspNetCore.Hosting/Internal/HostedServiceExecutor.cs diff --git a/src/Microsoft.AspNetCore.Hosting.Abstractions/IApplicationLifetimeEvents.cs b/src/Microsoft.AspNetCore.Hosting.Abstractions/IHostedService.cs similarity index 52% rename from src/Microsoft.AspNetCore.Hosting.Abstractions/IApplicationLifetimeEvents.cs rename to src/Microsoft.AspNetCore.Hosting.Abstractions/IHostedService.cs index e3f5cb6b..7573d36b 100644 --- a/src/Microsoft.AspNetCore.Hosting.Abstractions/IApplicationLifetimeEvents.cs +++ b/src/Microsoft.AspNetCore.Hosting.Abstractions/IHostedService.cs @@ -4,28 +4,20 @@ namespace Microsoft.AspNetCore.Hosting { /// - /// Allows consumers to perform cleanup during a graceful shutdown. + /// Defines methods for objects that are managed by the host. /// - public interface IApplicationLifetimeEvents + public interface IHostedService { /// - /// Triggered when the application host has fully started and is about to wait - /// for a graceful shutdown. + /// Triggered when the application host has fully started and the server is waiting + /// for requests. /// - void OnApplicationStarted(); + void Start(); /// /// Triggered when the application host is performing a graceful shutdown. /// Requests may still be in flight. Shutdown will block until this event completes. /// - void OnApplicationStopping(); - - /// - /// 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. - /// - void OnApplicationStopped(); - + void Stop(); } } diff --git a/src/Microsoft.AspNetCore.Hosting/Internal/ApplicationLifetime.cs b/src/Microsoft.AspNetCore.Hosting/Internal/ApplicationLifetime.cs index 0517d46f..b370d37d 100644 --- a/src/Microsoft.AspNetCore.Hosting/Internal/ApplicationLifetime.cs +++ b/src/Microsoft.AspNetCore.Hosting/Internal/ApplicationLifetime.cs @@ -17,13 +17,11 @@ public class ApplicationLifetime : IApplicationLifetime private readonly CancellationTokenSource _startedSource = new CancellationTokenSource(); private readonly CancellationTokenSource _stoppingSource = new CancellationTokenSource(); private readonly CancellationTokenSource _stoppedSource = new CancellationTokenSource(); - private readonly IEnumerable _handlers = Enumerable.Empty(); private readonly ILogger _logger; - public ApplicationLifetime(ILogger logger, IEnumerable handlers) + public ApplicationLifetime(ILogger logger) { _logger = logger; - _handlers = handlers; } /// @@ -57,7 +55,7 @@ public void StopApplication() { try { - ExecuteHandlers(_stoppingSource, handler => handler.OnApplicationStopping()); + ExecuteHandlers(_stoppingSource); } catch (Exception ex) { @@ -75,7 +73,7 @@ public void NotifyStarted() { try { - ExecuteHandlers(_startedSource, handler => handler.OnApplicationStarted()); + ExecuteHandlers(_startedSource); } catch (Exception ex) { @@ -92,7 +90,7 @@ public void NotifyStopped() { try { - ExecuteHandlers(_stoppedSource, handler => handler.OnApplicationStopped()); + ExecuteHandlers(_stoppedSource); } catch (Exception ex) { @@ -102,7 +100,7 @@ public void NotifyStopped() } } - private void ExecuteHandlers(CancellationTokenSource cancel, Action callback) + private void ExecuteHandlers(CancellationTokenSource cancel) { // Noop if this is already cancelled if (cancel.IsCancellationRequested) @@ -127,24 +125,6 @@ private void ExecuteHandlers(CancellationTokenSource cancel, Action(); - } - - exceptions.Add(ex); - } - } - // Throw an aggregate exception if there were any exceptions if (exceptions != null) { diff --git a/src/Microsoft.AspNetCore.Hosting/Internal/HostedServiceExecutor.cs b/src/Microsoft.AspNetCore.Hosting/Internal/HostedServiceExecutor.cs new file mode 100644 index 00000000..ac8afb41 --- /dev/null +++ b/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 _services; + private readonly ILogger _logger; + + public HostedServiceExecutor(ILogger logger, IEnumerable 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 callback) + { + List exceptions = null; + + foreach (var service in _services) + { + try + { + callback(service); + } + catch (Exception ex) + { + if (exceptions == null) + { + exceptions = new List(); + } + + exceptions.Add(ex); + } + } + + // Throw an aggregate exception if there were any exceptions + if (exceptions != null) + { + throw new AggregateException(exceptions); + } + } + } +} diff --git a/src/Microsoft.AspNetCore.Hosting/Internal/LoggerEventIds.cs b/src/Microsoft.AspNetCore.Hosting/Internal/LoggerEventIds.cs index 94764ec4..3266a6be 100644 --- a/src/Microsoft.AspNetCore.Hosting/Internal/LoggerEventIds.cs +++ b/src/Microsoft.AspNetCore.Hosting/Internal/LoggerEventIds.cs @@ -13,5 +13,7 @@ internal static class LoggerEventIds 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; } } diff --git a/src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs b/src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs index 8859fb6f..297f819b 100644 --- a/src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs +++ b/src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs @@ -28,6 +28,7 @@ public class WebHost : IWebHost 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 @@ public WebHost( _applicationServiceCollection = appServices; _hostingServiceProvider = hostingServiceProvider; _applicationServiceCollection.AddSingleton(); + _applicationServiceCollection.AddSingleton(); } public IServiceProvider Services @@ -104,11 +106,17 @@ public virtual void Start() Initialize(); _applicationLifetime = _applicationServices.GetRequiredService() as ApplicationLifetime; + _hostedServiceExecutor = _applicationServices.GetRequiredService(); var diagnosticSource = _applicationServices.GetRequiredService(); var httpContextFactory = _applicationServices.GetRequiredService(); Server.Start(new HostingApplication(_application, _logger, diagnosticSource, httpContextFactory)); + // Fire IApplicationLifetime.Started _applicationLifetime?.NotifyStarted(); + + // Fire IHostedService.Start + _hostedServiceExecutor.Start(); + _logger.Started(); } @@ -243,9 +251,17 @@ private void EnsureServer() 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(); diff --git a/test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs b/test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs index 44d0a720..6f8c08c9 100644 --- a/test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs +++ b/test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs @@ -316,16 +316,13 @@ public void WebHostNotifiesAllIApplicationLifetimeEventsCallbacksEvenIfTheyThrow 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 @@ public void WebHostStopCallbacksDontMultipleTimes() stoppingCalls++; }; - Action stopped = () => - { - stoppedCalls++; - }; - - services.AddSingleton(new DelegateLifetimeEvents(started, stopping, stopped)); + services.AddSingleton(new DelegateHostedService(started, stopping)); }) .Build(); var lifetime = host.Services.GetRequiredService(); lifetime.StopApplication(); + + using (host) + { + host.Start(); + + Assert.Equal(0, stoppingCalls); + } + } + + [Fact] + public void HostedServiceCanInjectApplicationLifetime() + { + var host = CreateBuilder() + .UseServer(this) + .ConfigureServices(services => + { + services.AddSingleton(); + }) + .Build(); + var lifetime = host.Services.GetRequiredService(); lifetime.StopApplication(); + + + host.Start(); + var svc = (TestHostedService)host.Services.GetRequiredService(); + Assert.True(svc.StartCalled); + host.Dispose(); + Assert.True(svc.StopCalled); + } + + [Fact] + public void HostedServiceStartNotCalledIfWebHostNotStarted() + { + var host = CreateBuilder() + .UseServer(this) + .ConfigureServices(services => + { + services.AddSingleton(); + }) + .Build(); + var lifetime = host.Services.GetRequiredService(); lifetime.StopApplication(); + var svc = (TestHostedService)host.Services.GetRequiredService(); + 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(new DelegateHostedService(started, stopping)); + }) + .Build(); + var lifetime = host.Services.GetRequiredService(); using (host) { host.Start(); host.Dispose(); + Assert.Equal(1, startedCalls); Assert.Equal(1, stoppingCalls); - Assert.Equal(1, stoppedCalls); } } [Fact] - public void WebHostNotifiesAllIApplicationLifetimeEventsCallbacksAndIApplicationLifetimeCallbacksEvenIfTheyThrow() + public void WebHostNotifiesAllIApplicationLifetimeEventsCallbacksAndIHostedServicesEvenIfTheyThrow() { bool[] events1 = null; bool[] events2 = null; @@ -381,7 +443,6 @@ public void WebHostNotifiesAllIApplicationLifetimeEventsCallbacksAndIApplication var started = RegisterCallbacksThatThrow(applicationLifetime.ApplicationStarted); var stopping = RegisterCallbacksThatThrow(applicationLifetime.ApplicationStopping); - var stopped = RegisterCallbacksThatThrow(applicationLifetime.ApplicationStopped); using (host) { @@ -393,9 +454,6 @@ public void WebHostNotifiesAllIApplicationLifetimeEventsCallbacksAndIApplication 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 @@ private IWebHostBuilder CreateBuilder(IConfiguration config = null) private static bool[] RegisterCallbacksThatThrow(IServiceCollection services) { - bool[] events = new bool[3]; + bool[] events = new bool[2]; Action started = () => { @@ -668,13 +726,7 @@ private static bool[] RegisterCallbacksThatThrow(IServiceCollection services) throw new InvalidOperationException(); }; - Action stopped = () => - { - events[2] = true; - throw new InvalidOperationException(); - }; - - services.AddSingleton(new DelegateLifetimeEvents(started, stopping, stopped)); + services.AddSingleton(new DelegateHostedService(started, stopping)); return events; } @@ -722,24 +774,43 @@ public void Dispose() } } - 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 OnApplicationStopped() => _stopped(); + public void Start() => _started(); - public void OnApplicationStopping() => _stopping(); + public void Stop() => _stopping(); } private class StartInstance : IDisposable