From 5c5d369e428e1b642663d4d38bd0221b2c7f8fcd Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Mon, 20 Nov 2023 10:33:23 +0000 Subject: [PATCH] Make ILazyLoader not IDisposable (#32345) Fixes #32267 The problem here is that ILazyLoader is a transient IDisposable service, which means that the service scope will keep track of instances created in the scope. However, when using context pooling, the service scope is not disposed because it is instead re-used. This means that the scope keeps getting more and more instances added, and never clears them out. The fix is to make the service not IDisposable. Instead, we create instances from our own internal factory where we keep track of the instances created. These can then be disposed and freed when the context is places back in the pool, or when the scope is disposed thus disposing the factory. --- .../Infrastructure/ILazyLoader.cs | 7 +- .../ChangeTracking/Internal/StateManager.cs | 5 ++ .../EntityFrameworkServicesBuilder.cs | 5 +- .../Internal/ILazyLoaderFactory.cs | 21 +++++ .../Internal/LazyLoaderFactory.cs | 81 +++++++++++++++++++ .../EntityFrameworkServicesBuilderTest.cs | 4 +- 6 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 src/EFCore/Infrastructure/Internal/ILazyLoaderFactory.cs create mode 100644 src/EFCore/Infrastructure/Internal/LazyLoaderFactory.cs diff --git a/src/EFCore.Abstractions/Infrastructure/ILazyLoader.cs b/src/EFCore.Abstractions/Infrastructure/ILazyLoader.cs index ad0ec5aa5ca..6974d2c11ff 100644 --- a/src/EFCore.Abstractions/Infrastructure/ILazyLoader.cs +++ b/src/EFCore.Abstractions/Infrastructure/ILazyLoader.cs @@ -20,7 +20,7 @@ namespace Microsoft.EntityFrameworkCore.Infrastructure; /// See Lazy loading for more information and examples. /// /// -public interface ILazyLoader : IDisposable +public interface ILazyLoader { /// /// Sets the given navigation as known to be completely loaded or known to be @@ -66,4 +66,9 @@ Task LoadAsync( object entity, CancellationToken cancellationToken = default, [CallerMemberName] string navigationName = ""); + + /// + /// Disposes the loader. + /// + void Dispose(); } diff --git a/src/EFCore/ChangeTracking/Internal/StateManager.cs b/src/EFCore/ChangeTracking/Internal/StateManager.cs index b072ae32de6..eb42067b0af 100644 --- a/src/EFCore/ChangeTracking/Internal/StateManager.cs +++ b/src/EFCore/ChangeTracking/Internal/StateManager.cs @@ -695,6 +695,11 @@ public virtual void Unsubscribe(bool resetting) { disposable.Dispose(); } + else if (resetting + && service is ILazyLoader lazyLoader) + { + lazyLoader.Dispose(); + } else if (service is not IInjectableService detachable || detachable.Detaching(Context, entry.Entity)) { diff --git a/src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs b/src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs index ff4b7b79262..3311f7e1fbd 100644 --- a/src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs +++ b/src/EFCore/Infrastructure/EntityFrameworkServicesBuilder.cs @@ -126,6 +126,7 @@ public static readonly IDictionary CoreServices { typeof(IDbContextLogger), new ServiceCharacteristics(ServiceLifetime.Scoped) }, { typeof(IAdHocMapper), new ServiceCharacteristics(ServiceLifetime.Scoped) }, { typeof(ILazyLoader), new ServiceCharacteristics(ServiceLifetime.Transient) }, + { typeof(ILazyLoaderFactory), new ServiceCharacteristics(ServiceLifetime.Scoped) }, { typeof(IParameterBindingFactory), new ServiceCharacteristics(ServiceLifetime.Singleton, multipleRegistrations: true) }, { typeof(ITypeMappingSourcePlugin), new ServiceCharacteristics(ServiceLifetime.Singleton, multipleRegistrations: true) }, { @@ -285,12 +286,14 @@ public virtual EntityFrameworkServicesBuilder TryAddCoreServices() TryAdd(p => new DesignTimeModel(GetContextServices(p))); TryAdd(p => GetContextServices(p).CurrentContext); TryAdd(p => GetContextServices(p).ContextOptions); + TryAdd(p => p.GetRequiredService()); TryAdd(p => p.GetRequiredService()); TryAdd(p => p.GetRequiredService()); TryAdd(); TryAdd(); TryAdd(); - TryAdd(); + TryAdd(); + TryAdd(p => p.GetRequiredService().Create()); TryAdd(); TryAdd(); TryAdd(); diff --git a/src/EFCore/Infrastructure/Internal/ILazyLoaderFactory.cs b/src/EFCore/Infrastructure/Internal/ILazyLoaderFactory.cs new file mode 100644 index 00000000000..5dc41dbb17a --- /dev/null +++ b/src/EFCore/Infrastructure/Internal/ILazyLoaderFactory.cs @@ -0,0 +1,21 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.EntityFrameworkCore.Infrastructure.Internal; + +/// +/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to +/// the same compatibility standards as public APIs. It may be changed or removed without notice in +/// any release. You should only use it directly in your code with extreme caution and knowing that +/// doing so can result in application failures when updating to a new Entity Framework Core release. +/// +public interface ILazyLoaderFactory : IDisposable, IResettableService +{ + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + ILazyLoader Create(); +} diff --git a/src/EFCore/Infrastructure/Internal/LazyLoaderFactory.cs b/src/EFCore/Infrastructure/Internal/LazyLoaderFactory.cs new file mode 100644 index 00000000000..86f51ce973d --- /dev/null +++ b/src/EFCore/Infrastructure/Internal/LazyLoaderFactory.cs @@ -0,0 +1,81 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.EntityFrameworkCore.Infrastructure.Internal; + +/// +/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to +/// the same compatibility standards as public APIs. It may be changed or removed without notice in +/// any release. You should only use it directly in your code with extreme caution and knowing that +/// doing so can result in application failures when updating to a new Entity Framework Core release. +/// +public class LazyLoaderFactory : ILazyLoaderFactory +{ + private readonly ICurrentDbContext _currentContext; + private readonly IDiagnosticsLogger _logger; + private readonly List _loaders = new(); + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public LazyLoaderFactory( + ICurrentDbContext currentContext, + IDiagnosticsLogger logger) + { + _currentContext = currentContext; + _logger = logger; + } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public virtual ILazyLoader Create() + { + var loader = new LazyLoader(_currentContext, _logger); + _loaders.Add(loader); + return loader; + } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public void Dispose() + { + foreach (var loader in _loaders) + { + loader.Dispose(); + } + _loaders.Clear(); + } + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public void ResetState() + => Dispose(); + + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public Task ResetStateAsync(CancellationToken cancellationToken = default) + { + Dispose(); + + return Task.CompletedTask; + } +} diff --git a/test/EFCore.Tests/Infrastructure/EntityFrameworkServicesBuilderTest.cs b/test/EFCore.Tests/Infrastructure/EntityFrameworkServicesBuilderTest.cs index 9e961fe0aff..d7435418f1f 100644 --- a/test/EFCore.Tests/Infrastructure/EntityFrameworkServicesBuilderTest.cs +++ b/test/EFCore.Tests/Infrastructure/EntityFrameworkServicesBuilderTest.cs @@ -266,7 +266,7 @@ private static void TestMultipleScoped(Action tr { services = context.GetService>().ToList(); - Assert.Equal(3, services.Count); + Assert.Equal(4, services.Count); Assert.Contains(typeof(FakeResetableService), services.Select(s => s.GetType())); Assert.Contains(typeof(StateManager), services.Select(s => s.GetType())); Assert.Contains(typeof(InMemoryTransactionManager), services.Select(s => s.GetType())); @@ -281,7 +281,7 @@ private static void TestMultipleScoped(Action tr { var newServices = context.GetService>().ToList(); - Assert.Equal(3, newServices.Count); + Assert.Equal(4, newServices.Count); foreach (var service in newServices) {