diff --git a/src/Microsoft.AspNetCore.Hosting.Abstractions/HostingAbstractionsWebHostBuilderExtensions.cs b/src/Microsoft.AspNetCore.Hosting.Abstractions/HostingAbstractionsWebHostBuilderExtensions.cs index 4b29b974d6..767ed9263f 100644 --- a/src/Microsoft.AspNetCore.Hosting.Abstractions/HostingAbstractionsWebHostBuilderExtensions.cs +++ b/src/Microsoft.AspNetCore.Hosting.Abstractions/HostingAbstractionsWebHostBuilderExtensions.cs @@ -77,8 +77,7 @@ namespace Microsoft.AspNetCore.Hosting { // It would be nicer if this was transient but we need to pass in the // factory instance directly - // Registering as factory so server gets disposed along with a WebHost - services.AddSingleton(provider => server); + services.AddSingleton(server); }); } diff --git a/src/Microsoft.AspNetCore.Hosting/Internal/ServiceCollectionExtensions.cs b/src/Microsoft.AspNetCore.Hosting/Internal/ServiceCollectionExtensions.cs new file mode 100644 index 0000000000..48b8758181 --- /dev/null +++ b/src/Microsoft.AspNetCore.Hosting/Internal/ServiceCollectionExtensions.cs @@ -0,0 +1,20 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.Extensions.DependencyInjection; + +namespace Microsoft.AspNetCore.Hosting.Internal +{ + internal static class ServiceCollectionExtensions + { + public static IServiceCollection Clone(this IServiceCollection serviceCollection) + { + IServiceCollection clone = new ServiceCollection(); + foreach (var service in serviceCollection) + { + clone.Add(service); + } + return clone; + } + } +} diff --git a/src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs b/src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs index 8098c5838f..1403858a40 100644 --- a/src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs +++ b/src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs @@ -245,23 +245,9 @@ namespace Microsoft.AspNetCore.Hosting.Internal { _logger?.Shutdown(); _applicationLifetime.StopApplication(); + (_hostingServiceProvider as IDisposable)?.Dispose(); (_applicationServices as IDisposable)?.Dispose(); _applicationLifetime.NotifyStopped(); } - - private class Disposable : IDisposable - { - private Action _dispose; - - public Disposable(Action dispose) - { - _dispose = dispose; - } - - public void Dispose() - { - Interlocked.Exchange(ref _dispose, () => { }).Invoke(); - } - } } } diff --git a/src/Microsoft.AspNetCore.Hosting/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Hosting/Properties/Resources.Designer.cs index 397c84eba2..088072729c 100644 --- a/src/Microsoft.AspNetCore.Hosting/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Hosting/Properties/Resources.Designer.cs @@ -58,6 +58,22 @@ namespace Microsoft.AspNetCore.Hosting return GetString("ErrorPageHtml_UnknownLocation"); } + /// + /// WebHostBuilder allows creation only of a single instance of WebHost + /// + internal static string WebHostBuilder_SingleInstance + { + get { return GetString("WebHostBuilder_SingleInstance"); } + } + + /// + /// WebHostBuilder allows creation only of a single instance of WebHost + /// + internal static string FormatWebHostBuilder_SingleInstance() + { + return GetString("WebHostBuilder_SingleInstance"); + } + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNetCore.Hosting/Resources.resx b/src/Microsoft.AspNetCore.Hosting/Resources.resx index 6af9630466..64d6fe523d 100644 --- a/src/Microsoft.AspNetCore.Hosting/Resources.resx +++ b/src/Microsoft.AspNetCore.Hosting/Resources.resx @@ -126,4 +126,7 @@ Unknown location + + WebHostBuilder allows creation only of a single instance of WebHost + \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Hosting/WebHostBuilder.cs b/src/Microsoft.AspNetCore.Hosting/WebHostBuilder.cs index 09c0983f59..a4b1e6dc3b 100644 --- a/src/Microsoft.AspNetCore.Hosting/WebHostBuilder.cs +++ b/src/Microsoft.AspNetCore.Hosting/WebHostBuilder.cs @@ -31,6 +31,7 @@ namespace Microsoft.AspNetCore.Hosting private IConfiguration _config; private ILoggerFactory _loggerFactory; private WebHostOptions _options; + private bool _webHostBuilt; /// /// Initializes a new instance of the class. @@ -48,7 +49,7 @@ namespace Microsoft.AspNetCore.Hosting if (string.IsNullOrEmpty(GetSetting(WebHostDefaults.EnvironmentKey))) { // Try adding legacy environment keys, never remove these. - UseSetting(WebHostDefaults.EnvironmentKey, Environment.GetEnvironmentVariable("Hosting:Environment") + UseSetting(WebHostDefaults.EnvironmentKey, Environment.GetEnvironmentVariable("Hosting:Environment") ?? Environment.GetEnvironmentVariable("ASPNET_ENV")); } @@ -135,6 +136,12 @@ namespace Microsoft.AspNetCore.Hosting /// public IWebHost Build() { + if (_webHostBuilt) + { + throw new InvalidOperationException(Resources.WebHostBuilder_SingleInstance); + } + _webHostBuilt = true; + // Warn about deprecated environment variables if (Environment.GetEnvironmentVariable("Hosting:Environment") != null) { @@ -151,17 +158,24 @@ namespace Microsoft.AspNetCore.Hosting Console.WriteLine("The environment variable 'ASPNETCORE_SERVER.URLS' is obsolete and has been replaced with 'ASPNETCORE_URLS'"); } - var hostingServices = BuildHostingServices(); - var hostingContainer = hostingServices.BuildServiceProvider(); + var hostingServices = BuildCommonServices(); + var applicationServices = hostingServices.Clone(); + var hostingServiceProvider = hostingServices.BuildServiceProvider(); - var host = new WebHost(hostingServices, hostingContainer, _options, _config); + AddApplicationServices(applicationServices, hostingServiceProvider); + + var host = new WebHost( + applicationServices, + hostingServiceProvider, + _options, + _config); host.Initialize(); return host; } - private IServiceCollection BuildHostingServices() + private IServiceCollection BuildCommonServices() { _options = new WebHostOptions(_config); @@ -175,9 +189,15 @@ namespace Microsoft.AspNetCore.Hosting var services = new ServiceCollection(); services.AddSingleton(_hostingEnvironment); + // The configured ILoggerFactory is added as a singleton here. AddLogging below will not add an additional one. if (_loggerFactory == null) { _loggerFactory = new LoggerFactory(); + services.AddSingleton(provider => _loggerFactory); + } + else + { + services.AddSingleton(_loggerFactory); } foreach (var configureLogging in _configureLoggingDelegates) @@ -185,20 +205,17 @@ namespace Microsoft.AspNetCore.Hosting configureLogging(_loggerFactory); } - //The configured ILoggerFactory is added as a singleton here. AddLogging below will not add an additional one. - services.AddSingleton(_loggerFactory); - //This is required to add ILogger of T. services.AddLogging(); + var listener = new DiagnosticListener("Microsoft.AspNetCore"); + services.AddSingleton(listener); + services.AddSingleton(listener); + services.AddTransient(); services.AddTransient(); services.AddOptions(); - var diagnosticSource = new DiagnosticListener("Microsoft.AspNetCore"); - services.AddSingleton(diagnosticSource); - services.AddSingleton(diagnosticSource); - // Conjure up a RequestServices services.AddTransient(); services.AddTransient, DefaultServiceProviderFactory>(); @@ -245,6 +262,20 @@ namespace Microsoft.AspNetCore.Hosting return services; } + private void AddApplicationServices(IServiceCollection services, IServiceProvider hostingServiceProvider) + { + // We are forwarding services from hosting contrainer so hosting container + // can still manage their lifetime (disposal) shared instances with application services. + // NOTE: This code overrides original services lifetime. Instances would always be singleton in + // application container. + var loggerFactory = hostingServiceProvider.GetService(); + services.Replace(ServiceDescriptor.Singleton(typeof(ILoggerFactory), loggerFactory)); + + var listener = hostingServiceProvider.GetService(); + services.Replace(ServiceDescriptor.Singleton(typeof(DiagnosticListener), listener)); + services.Replace(ServiceDescriptor.Singleton(typeof(DiagnosticSource), listener)); + } + private string ResolveContentRootPath(string contentRootPath, string basePath) { if (string.IsNullOrEmpty(contentRootPath)) diff --git a/src/Microsoft.AspNetCore.Hosting/WebHostBuilderExtensions.cs b/src/Microsoft.AspNetCore.Hosting/WebHostBuilderExtensions.cs index 7b33bebe4c..79598203f0 100644 --- a/src/Microsoft.AspNetCore.Hosting/WebHostBuilderExtensions.cs +++ b/src/Microsoft.AspNetCore.Hosting/WebHostBuilderExtensions.cs @@ -35,7 +35,7 @@ namespace Microsoft.AspNetCore.Hosting /// - /// Specify the startup type to be used by the web host. + /// Specify the startup type to be used by the web host. /// /// The to configure. /// The to be used. diff --git a/test/Microsoft.AspNetCore.Hosting.Tests/Fakes/StartupWithILoggerFactory.cs b/test/Microsoft.AspNetCore.Hosting.Tests/Fakes/StartupWithILoggerFactory.cs new file mode 100644 index 0000000000..ad596e2546 --- /dev/null +++ b/test/Microsoft.AspNetCore.Hosting.Tests/Fakes/StartupWithILoggerFactory.cs @@ -0,0 +1,31 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; + +namespace Microsoft.AspNetCore.Hosting.Fakes +{ + public class StartupWithILoggerFactory + { + public ILoggerFactory ConstructorLoggerFactory { get; set; } + + public ILoggerFactory ConfigureLoggerFactory { get; set; } + + public StartupWithILoggerFactory(ILoggerFactory constructorLoggerFactory) + { + ConstructorLoggerFactory = constructorLoggerFactory; + } + + public void ConfigureServices(IServiceCollection collection) + { + collection.AddSingleton(this); + } + + public void Configure(IApplicationBuilder builder, ILoggerFactory loggerFactory) + { + ConfigureLoggerFactory = loggerFactory; + } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Hosting.Tests/WebHostBuilderTests.cs b/test/Microsoft.AspNetCore.Hosting.Tests/WebHostBuilderTests.cs index c3e44259ed..1360d8ee42 100644 --- a/test/Microsoft.AspNetCore.Hosting.Tests/WebHostBuilderTests.cs +++ b/test/Microsoft.AspNetCore.Hosting.Tests/WebHostBuilderTests.cs @@ -15,6 +15,7 @@ using Microsoft.AspNetCore.Http.Features; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.ObjectPool; using Microsoft.Extensions.PlatformAbstractions; using Xunit; @@ -494,6 +495,84 @@ namespace Microsoft.AspNetCore.Hosting Assert.Equal("Microsoft.AspNetCore.Hosting.Tests", hostingEnv.ApplicationName); } + [Fact] + public void Build_DoesNotAllowBuildingMuiltipleTimes() + { + var builder = CreateWebHostBuilder(); + var server = new TestServer(); + builder.UseServer(server) + .UseStartup() + .Build(); + + var ex = Assert.Throws(() => builder.Build()); + + Assert.Equal("WebHostBuilder allows creation only of a single instance of WebHost", ex.Message); + } + + [Fact] + public void Build_PassesSameAutoCreatedILoggerFactoryEverywhere() + { + var builder = CreateWebHostBuilder(); + var server = new TestServer(); + var host = builder.UseServer(server) + .UseStartup() + .Build(); + + var startup = host.Services.GetService(); + + Assert.Equal(startup.ConfigureLoggerFactory, startup.ConstructorLoggerFactory); + } + + [Fact] + public void Build_PassesSamePassedILoggerFactoryEverywhere() + { + var factory = new LoggerFactory(); + var builder = CreateWebHostBuilder(); + var server = new TestServer(); + var host = builder.UseServer(server) + .UseLoggerFactory(factory) + .UseStartup() + .Build(); + + var startup = host.Services.GetService(); + + Assert.Equal(factory, startup.ConfigureLoggerFactory); + Assert.Equal(factory, startup.ConstructorLoggerFactory); + } + + [Fact] + public void Build_PassedILoggerFactoryNotDisposed() + { + var factory = new DisposableLoggerFactory(); + var builder = CreateWebHostBuilder(); + var server = new TestServer(); + + var host = builder.UseServer(server) + .UseLoggerFactory(factory) + .UseStartup() + .Build(); + + host.Dispose(); + + Assert.Equal(false, factory.Disposed); + } + + [Fact] + public void Build_DoesNotOverrideILoggerFactorySetByConfigureServices() + { + var factory = new DisposableLoggerFactory(); + var builder = CreateWebHostBuilder(); + var server = new TestServer(); + + var host = builder.UseServer(server) + .ConfigureServices(collection => collection.AddSingleton(factory)) + .UseStartup() + .Build(); + + var factoryFromHost = host.Services.GetService(); + Assert.Equal(factory, factoryFromHost); + } + private static void StaticConfigureMethod(IApplicationBuilder app) { } @@ -558,5 +637,24 @@ namespace Microsoft.AspNetCore.Hosting { } + + private class DisposableLoggerFactory : ILoggerFactory + { + public void Dispose() + { + Disposed = true; + } + + public bool Disposed { get; set; } + + public ILogger CreateLogger(string categoryName) + { + return NullLogger.Instance; + } + + public void AddProvider(ILoggerProvider provider) + { + } + } } } diff --git a/test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs b/test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs index 33b647eeeb..ab52089b8f 100644 --- a/test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs +++ b/test/Microsoft.AspNetCore.Hosting.Tests/WebHostTests.cs @@ -135,7 +135,7 @@ namespace Microsoft.AspNetCore.Hosting host.Dispose(); - Assert.Equal(1, _startInstances[0].DisposeCalls); + Assert.Equal(0, _startInstances[0].DisposeCalls); } [Fact] @@ -163,7 +163,7 @@ namespace Microsoft.AspNetCore.Hosting // Wait on the host to shutdown lifetime.ApplicationStopped.WaitHandle.WaitOne(); - Assert.Equal(1, _startInstances[0].DisposeCalls); + Assert.Equal(0, _startInstances[0].DisposeCalls); } [Fact]