Ensure that ILoggerFactory is re-captured for each context Instance

Otherwise the wrong (possibly disposed) logger factory can be used.

Fixes part of ASP.NET build break.

Author: ajcvickers

src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs

@@ -639,7 +639,7 @@ private void GenerateProperty(IProperty property, bool useDataAnnotations)
             }
 
             var dummyLogger = new DiagnosticsLogger<DbLoggerCategory.Model>(
-                new LoggerFactory(),
+                new ScopedLoggerFactory(new LoggerFactory(), dispose: true),
                 new LoggingOptions(),
                 new DiagnosticListener(""));
 

src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs

@@ -487,7 +487,7 @@ protected virtual KeyBuilder VisitPrimaryKey([NotNull] EntityTypeBuilder builder
                 if (property != null)
                 {
                     var dummyLogger = new DiagnosticsLogger<DbLoggerCategory.Model>(
-                        new LoggerFactory(),
+                        new ScopedLoggerFactory(new LoggerFactory(), dispose: true),
                         new LoggingOptions(),
                         new DiagnosticListener(""));
 

src/EFCore.Relational/Metadata/Conventions/Internal/RelationalConventionSetBuilderDependencies.cs

@@ -69,7 +69,10 @@ public RelationalConventionSetBuilderDependencies(
 
             TypeMappingSource = typeMappingSource;
             Logger = logger
-                     ?? new DiagnosticsLogger<DbLoggerCategory.Model>(new LoggerFactory(), new LoggingOptions(), new DiagnosticListener(""));
+                     ?? new DiagnosticsLogger<DbLoggerCategory.Model>(
+                         new ScopedLoggerFactory(new LoggerFactory(), dispose: true),
+                         new LoggingOptions(),
+                         new DiagnosticListener(""));
             Context = currentContext;
             SetFinder = setFinder;
         }

src/EFCore/Infrastructure/CoreOptionsExtension.cs

@@ -345,12 +345,6 @@ public virtual CoreOptionsExtension WithCacheServiceProvider(bool cacheServicePr
         /// <returns> False since no database provider is registered. </returns>
         public virtual bool ApplyServices(IServiceCollection services)
         {
-            var loggerFactory = GetLoggerFactory();
-            if (loggerFactory != null)
-            {
-                services.AddScoped<ILoggerFactory>(_ => new ExternalLoggerFactory(loggerFactory));
-            }
-
             var memoryCache = GetMemoryCache();
             if (memoryCache != null)
             {
@@ -363,9 +357,6 @@ public virtual bool ApplyServices(IServiceCollection services)
         private IMemoryCache GetMemoryCache()
             => MemoryCache;
 
-        private ILoggerFactory GetLoggerFactory()
-            => LoggerFactory ?? ApplicationServiceProvider?.GetService<ILoggerFactory>();
-
         /// <summary>
         ///     Returns a hash code created from any options that would cause a new <see cref="IServiceProvider" />
         ///     to be needed.

src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs

@@ -216,7 +216,7 @@ public virtual EntityFrameworkServicesBuilder TryAddCoreServices()
             TryAdd<ICoreConventionSetBuilder, CoreConventionSetBuilder>();
             TryAdd<IModelCustomizer, ModelCustomizer>();
             TryAdd<IModelCacheKeyFactory, ModelCacheKeyFactory>();
-            TryAdd<ILoggerFactory, LoggerFactory>();
+            TryAdd<ILoggerFactory>(p => ScopedLoggerFactory.Create(p, null));
             TryAdd<IModelSource, ModelSource>();
             TryAdd<IInternalEntityEntryFactory, InternalEntityEntryFactory>();
             TryAdd<IInternalEntityEntrySubscriber, InternalEntityEntrySubscriber>();

src/EFCore/Internal/ExternalLoggerFactory.cs

@@ -0,0 +1,86 @@
+// 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 System;
+using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Infrastructure;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+
+namespace Microsoft.EntityFrameworkCore.Internal
+{
+    public class ScopedLoggerFactory : ILoggerFactory
+    {
+        private readonly ILoggerFactory _underlyingFactory;
+        private readonly bool _dispose;
+
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public ScopedLoggerFactory(
+            [NotNull] ILoggerFactory loggerFactory,
+            bool dispose)
+        {
+            _underlyingFactory = loggerFactory;
+            _dispose = dispose;
+        }
+
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public static ScopedLoggerFactory Create(
+            [NotNull] IServiceProvider internalServiceProvider,
+            [CanBeNull] IDbContextOptions contextOptions)
+        {
+            var coreOptions
+                = (contextOptions ??
+                   internalServiceProvider.GetService<IDbContextOptions>())
+                ?.FindExtension<CoreOptionsExtension>();
+
+            if (coreOptions != null)
+            {
+                if (coreOptions.LoggerFactory != null)
+                {
+                    return new ScopedLoggerFactory(coreOptions.LoggerFactory, dispose: false);
+                }
+
+                var applicationServiceProvider = coreOptions.ApplicationServiceProvider;
+                if (applicationServiceProvider != null
+                    && applicationServiceProvider != internalServiceProvider)
+                {
+                    return new ScopedLoggerFactory(applicationServiceProvider.GetService<ILoggerFactory>(), dispose: false);
+                }
+            }
+
+            return new ScopedLoggerFactory(new LoggerFactory(), dispose: true);
+        }
+
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual void Dispose()
+        {
+            if (_dispose)
+            {
+                _underlyingFactory.Dispose();
+            }
+        }
+
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual ILogger CreateLogger(string categoryName)
+            => _underlyingFactory.CreateLogger(categoryName);
+
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual void AddProvider(ILoggerProvider provider)
+            => _underlyingFactory.AddProvider(provider);
+    }
+}

src/EFCore/Internal/ScopedLoggerFactory.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.Diagnostics;
@@ -100,8 +101,13 @@ public virtual IServiceProvider GetOrAdd([NotNull] IDbContextOptions options, bo
 
                 using (var scope = serviceProvider.CreateScope())
                 {
-                    var logger =
-                        scope.ServiceProvider.GetRequiredService<IDiagnosticsLogger<DbLoggerCategory.Infrastructure>>();
+                    var scopedProvider = scope.ServiceProvider;
+
+                    // Because IDbContextOptions cannot yet be resolved from the internal provider
+                    var logger = new DiagnosticsLogger<DbLoggerCategory.Infrastructure>(
+                        ScopedLoggerFactory.Create(scopedProvider, options),
+                        scopedProvider.GetService<ILoggingOptions>(),
+                        scopedProvider.GetService<DiagnosticSource>());
 
                     if (_configurations.Count == 0)
                     {

src/EFCore/Internal/ServiceProviderCache.cs

@@ -13,6 +13,7 @@ namespace Microsoft.EntityFrameworkCore.TestUtilities
     public class ListLoggerFactory : ILoggerFactory
     {
         private readonly Func<string, bool> _shouldLogCategory;
+        private bool _disposed;
 
         public ListLoggerFactory()
             : this(_ => true)
@@ -37,16 +38,30 @@ public void SetTestOutputHelper(ITestOutputHelper testOutputHelper)
         }
 
         public virtual ILogger CreateLogger(string name)
-            => !_shouldLogCategory(name)
+        {
+            CheckDisposed();
+
+            return !_shouldLogCategory(name)
                 ? (ILogger)NullLogger.Instance
                 : Logger;
+        }
+
+        private void CheckDisposed()
+        {
+            if (_disposed)
+            {
+                throw new ObjectDisposedException(nameof(ListLoggerFactory));
+            }
+        }
 
         public void AddProvider(ILoggerProvider provider)
         {
+            CheckDisposed();
         }
 
         public void Dispose()
         {
+            _disposed = true;
         }
 
         protected class ListLogger : ILogger

test/EFCore.Specification.Tests/TestUtilities/ListLoggerFactory.cs

@@ -119,10 +119,21 @@ protected internal override void OnConfiguring(DbContextOptionsBuilder optionsBu
 
         private class MyLoggerProvider : ILoggerProvider
         {
-            public ILogger CreateLogger(string categoryName) => new MyListLogger(Log);
+            private bool _disposed;
+
+            public ILogger CreateLogger(string categoryName)
+            {
+                if (_disposed)
+                {
+                    throw new ObjectDisposedException(nameof(MyLoggerProvider));
+                }
+
+                return new MyListLogger(Log);
+            }
 
             public void Dispose()
             {
+                _disposed = true;
             }
 
             private class MyListLogger : ILogger
@@ -164,6 +175,58 @@ public void Log<TState>(
             }
         }
 
+        [Theory]
+        [InlineData(ServiceLifetime.Scoped)]
+        [InlineData(ServiceLifetime.Singleton)]
+        public void Logger_factory_registered_on_application_service_provider_is_not_disposed(ServiceLifetime optionsLifetime)
+        {
+            for (var i = 0; i < 2; i++)
+            {
+                ILoggerFactory loggerFactory;
+
+                using (var appServiceProvider
+                    = new ServiceCollection()
+                        .AddScoped<Random>()
+                        .AddDbContext<ConstructorTestContext1A>(
+                            b => b.UseInMemoryDatabase("Scratch"), optionsLifetime: optionsLifetime)
+                        .BuildServiceProvider())
+                {
+                    loggerFactory = appServiceProvider.GetService<ILoggerFactory>();
+                    Random scopedExternalService;
+
+                    using (var scope = appServiceProvider.CreateScope())
+                    {
+                        var context = scope.ServiceProvider.GetRequiredService<ConstructorTestContext1A>();
+
+                        // Should not throw
+                        var _ = context.Model;
+
+                        Assert.Same(loggerFactory, scope.ServiceProvider.GetService<ILoggerFactory>());
+
+                        scopedExternalService = scope.ServiceProvider.GetService<Random>();
+                        Assert.NotNull(scopedExternalService);
+                    }
+
+                    using (var scope = appServiceProvider.CreateScope())
+                    {
+                        var context = scope.ServiceProvider.GetRequiredService<ConstructorTestContext1A>();
+
+                        // Should not throw
+                        var _ = context.Model;
+
+                        Assert.Same(loggerFactory, scope.ServiceProvider.GetService<ILoggerFactory>());
+
+                        Assert.NotSame(scopedExternalService, scope.ServiceProvider.GetService<Random>());
+                    }
+
+                    // Should not throw
+                    loggerFactory.CreateLogger("MyLogger");
+                }
+
+                Assert.Throws<ObjectDisposedException>(() => loggerFactory.CreateLogger("MyLogger"));
+            }
+        }
+
         [Fact]
         public void Can_use_GetInfrastructure_with_inferred_generic_to_get_service_provider()
         {
@@ -175,6 +238,28 @@ public void Can_use_GetInfrastructure_with_inferred_generic_to_get_service_provi
             }
         }
 
+        [Fact]
+        public void Logger_factory_registered_on_internal_service_provider_is_not_disposed()
+        {
+            var serviceProvider
+                = new ServiceCollection()
+                    .AddEntityFrameworkInMemoryDatabase()
+                    .BuildServiceProvider();
+
+            var appServiceProvider
+                = new ServiceCollection()
+                    .AddDbContext<ConstructorTestContext1A>(
+                        b => b.UseInMemoryDatabase("Scratch")
+                            .UseInternalServiceProvider(serviceProvider))
+                    .BuildServiceProvider();
+
+            var context = appServiceProvider.GetRequiredService<ConstructorTestContext1A>();
+            var _ = context.Model;
+
+            context = appServiceProvider.GetRequiredService<ConstructorTestContext1A>();
+            _ = context.Model;
+        }
+
         [Fact]
         public void Each_context_gets_new_scoped_services()
         {
@@ -306,7 +391,7 @@ public void Required_low_level_services_are_added_if_needed()
             new EntityFrameworkServicesBuilder(serviceCollection).TryAddCoreServices();
             var provider = serviceCollection.BuildServiceProvider();
 
-            Assert.IsType<LoggerFactory>(provider.GetRequiredService<ILoggerFactory>());
+            Assert.IsType<ScopedLoggerFactory>(provider.GetRequiredService<ILoggerFactory>());
         }
 
         [Fact]