diff --git a/src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs b/src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs index 1974723d88e..2af1cee6a20 100644 --- a/src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs +++ b/src/EFCore.Design/Scaffolding/Internal/CSharpDbContextGenerator.cs @@ -639,7 +639,7 @@ private void GenerateProperty(IProperty property, bool useDataAnnotations) } var dummyLogger = new DiagnosticsLogger( - new LoggerFactory(), + new ScopedLoggerFactory(new LoggerFactory(), dispose: true), new LoggingOptions(), new DiagnosticListener("")); diff --git a/src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs b/src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs index c2e8a1f08d3..b1bf221d986 100644 --- a/src/EFCore.Design/Scaffolding/Internal/RelationalScaffoldingModelFactory.cs +++ b/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( - new LoggerFactory(), + new ScopedLoggerFactory(new LoggerFactory(), dispose: true), new LoggingOptions(), new DiagnosticListener("")); diff --git a/src/EFCore.Relational/Metadata/Conventions/Internal/RelationalConventionSetBuilderDependencies.cs b/src/EFCore.Relational/Metadata/Conventions/Internal/RelationalConventionSetBuilderDependencies.cs index 45a9ebe4a76..bb201a0f4b9 100644 --- a/src/EFCore.Relational/Metadata/Conventions/Internal/RelationalConventionSetBuilderDependencies.cs +++ b/src/EFCore.Relational/Metadata/Conventions/Internal/RelationalConventionSetBuilderDependencies.cs @@ -69,7 +69,10 @@ public RelationalConventionSetBuilderDependencies( TypeMappingSource = typeMappingSource; Logger = logger - ?? new DiagnosticsLogger(new LoggerFactory(), new LoggingOptions(), new DiagnosticListener("")); + ?? new DiagnosticsLogger( + new ScopedLoggerFactory(new LoggerFactory(), dispose: true), + new LoggingOptions(), + new DiagnosticListener("")); Context = currentContext; SetFinder = setFinder; } diff --git a/src/EFCore/Infrastructure/CoreOptionsExtension.cs b/src/EFCore/Infrastructure/CoreOptionsExtension.cs index 25d6929d894..5e9d6bbfa5c 100644 --- a/src/EFCore/Infrastructure/CoreOptionsExtension.cs +++ b/src/EFCore/Infrastructure/CoreOptionsExtension.cs @@ -345,12 +345,6 @@ public virtual CoreOptionsExtension WithCacheServiceProvider(bool cacheServicePr /// False since no database provider is registered. public virtual bool ApplyServices(IServiceCollection services) { - var loggerFactory = GetLoggerFactory(); - if (loggerFactory != null) - { - services.AddScoped(_ => 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(); - /// /// Returns a hash code created from any options that would cause a new /// to be needed. diff --git a/src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs b/src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs index ac142708009..5373e94251a 100644 --- a/src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs +++ b/src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs @@ -216,7 +216,7 @@ public virtual EntityFrameworkServicesBuilder TryAddCoreServices() TryAdd(); TryAdd(); TryAdd(); - TryAdd(); + TryAdd(p => ScopedLoggerFactory.Create(p, null)); TryAdd(); TryAdd(); TryAdd(); diff --git a/src/EFCore/Internal/ExternalLoggerFactory.cs b/src/EFCore/Internal/ExternalLoggerFactory.cs deleted file mode 100644 index 020b9568757..00000000000 --- a/src/EFCore/Internal/ExternalLoggerFactory.cs +++ /dev/null @@ -1,50 +0,0 @@ -// 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 JetBrains.Annotations; -using Microsoft.Extensions.Logging; - -namespace Microsoft.EntityFrameworkCore.Internal -{ - /// - /// 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. - /// - public class ExternalLoggerFactory : ILoggerFactory - { - private readonly ILoggerFactory _underlyingFactory; - - /// - /// 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. - /// - public ExternalLoggerFactory([NotNull] ILoggerFactory underlyingFactory) - { - _underlyingFactory = underlyingFactory; - } - - /// - /// 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. - /// - public virtual void Dispose() - { - // Intentionally do nothing here so we don't dispose an external resource that - // we did not create. - } - - /// - /// 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. - /// - public virtual ILogger CreateLogger(string categoryName) - => _underlyingFactory.CreateLogger(categoryName); - - /// - /// 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. - /// - public virtual void AddProvider(ILoggerProvider provider) - => _underlyingFactory.AddProvider(provider); - } -} diff --git a/src/EFCore/Internal/ScopedLoggerFactory.cs b/src/EFCore/Internal/ScopedLoggerFactory.cs new file mode 100644 index 00000000000..b5179b3a734 --- /dev/null +++ b/src/EFCore/Internal/ScopedLoggerFactory.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; + + /// + /// 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. + /// + public ScopedLoggerFactory( + [NotNull] ILoggerFactory loggerFactory, + bool dispose) + { + _underlyingFactory = loggerFactory; + _dispose = dispose; + } + + /// + /// 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. + /// + public static ScopedLoggerFactory Create( + [NotNull] IServiceProvider internalServiceProvider, + [CanBeNull] IDbContextOptions contextOptions) + { + var coreOptions + = (contextOptions ?? + internalServiceProvider.GetService()) + ?.FindExtension(); + + 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(), dispose: false); + } + } + + return new ScopedLoggerFactory(new LoggerFactory(), dispose: true); + } + + /// + /// 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. + /// + public virtual void Dispose() + { + if (_dispose) + { + _underlyingFactory.Dispose(); + } + } + + /// + /// 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. + /// + public virtual ILogger CreateLogger(string categoryName) + => _underlyingFactory.CreateLogger(categoryName); + + /// + /// 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. + /// + public virtual void AddProvider(ILoggerProvider provider) + => _underlyingFactory.AddProvider(provider); + } +} diff --git a/src/EFCore/Internal/ServiceProviderCache.cs b/src/EFCore/Internal/ServiceProviderCache.cs index 44c72176fb4..0ad8600eca2 100644 --- a/src/EFCore/Internal/ServiceProviderCache.cs +++ b/src/EFCore/Internal/ServiceProviderCache.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>(); + var scopedProvider = scope.ServiceProvider; + + // Because IDbContextOptions cannot yet be resolved from the internal provider + var logger = new DiagnosticsLogger( + ScopedLoggerFactory.Create(scopedProvider, options), + scopedProvider.GetService(), + scopedProvider.GetService()); if (_configurations.Count == 0) { diff --git a/test/EFCore.Specification.Tests/TestUtilities/ListLoggerFactory.cs b/test/EFCore.Specification.Tests/TestUtilities/ListLoggerFactory.cs index 724a48415a4..b3194df56df 100644 --- a/test/EFCore.Specification.Tests/TestUtilities/ListLoggerFactory.cs +++ b/test/EFCore.Specification.Tests/TestUtilities/ListLoggerFactory.cs @@ -13,6 +13,7 @@ namespace Microsoft.EntityFrameworkCore.TestUtilities public class ListLoggerFactory : ILoggerFactory { private readonly Func _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 diff --git a/test/EFCore.Tests/DbContextServicesTest.cs b/test/EFCore.Tests/DbContextServicesTest.cs index 466453547d2..94d7419b48d 100644 --- a/test/EFCore.Tests/DbContextServicesTest.cs +++ b/test/EFCore.Tests/DbContextServicesTest.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( } } + [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() + .AddDbContext( + b => b.UseInMemoryDatabase("Scratch"), optionsLifetime: optionsLifetime) + .BuildServiceProvider()) + { + loggerFactory = appServiceProvider.GetService(); + Random scopedExternalService; + + using (var scope = appServiceProvider.CreateScope()) + { + var context = scope.ServiceProvider.GetRequiredService(); + + // Should not throw + var _ = context.Model; + + Assert.Same(loggerFactory, scope.ServiceProvider.GetService()); + + scopedExternalService = scope.ServiceProvider.GetService(); + Assert.NotNull(scopedExternalService); + } + + using (var scope = appServiceProvider.CreateScope()) + { + var context = scope.ServiceProvider.GetRequiredService(); + + // Should not throw + var _ = context.Model; + + Assert.Same(loggerFactory, scope.ServiceProvider.GetService()); + + Assert.NotSame(scopedExternalService, scope.ServiceProvider.GetService()); + } + + // Should not throw + loggerFactory.CreateLogger("MyLogger"); + } + + Assert.Throws(() => 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( + b => b.UseInMemoryDatabase("Scratch") + .UseInternalServiceProvider(serviceProvider)) + .BuildServiceProvider(); + + var context = appServiceProvider.GetRequiredService(); + var _ = context.Model; + + context = appServiceProvider.GetRequiredService(); + _ = 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(provider.GetRequiredService()); + Assert.IsType(provider.GetRequiredService()); } [Fact]