From 4e6fee3813a7069aebdb714a57abcf88c623d2b4 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 19 Aug 2022 10:03:25 -0700 Subject: [PATCH 1/2] Fix leaks caused by not disposing the parent scoped ServiceProvider --- .../src/ServiceLookup/ServiceProviderEngineScope.cs | 13 +++++++++---- .../DI.Tests/ServiceProviderEngineScopeTests.cs | 12 ++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 0cddf30f2a719c..fb70e4a6377e6e 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -187,12 +187,17 @@ static async ValueTask Await(int i, ValueTask vt, List toDispose) // No further changes to _state.Disposables, are allowed. _disposed = true; - // ResolvedServices is never cleared for singletons because there might be a compilation running in background - // trying to get a cached singleton service. If it doesn't find it - // it will try to create a new one which will result in an ObjectDisposedException. + } - return _disposables; + if (IsRootScope) + { + RootProvider.Dispose(); } + + // ResolvedServices is never cleared for singletons because there might be a compilation running in background + // trying to get a cached singleton service. If it doesn't find it + // it will try to create a new one which will result in an ObjectDisposedException. + return _disposables; } } } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs index d43752db21eba7..a6dbe1c92bfbf8 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using Microsoft.Extensions.DependencyInjection.Specification.Fakes; using Xunit; @@ -17,5 +18,16 @@ public void DoubleDisposeWorks() serviceProviderEngineScope.Dispose(); serviceProviderEngineScope.Dispose(); } + + [Fact] + public void RootDisposeTest() + { + var services = new ServiceCollection(); + ServiceProvider sp = services.BuildServiceProvider(); + var s = sp.GetRequiredService(); + ((IDisposable)s).Dispose(); + + Assert.Throws(() => sp.GetRequiredService()); + } } } From 9569a7b99a9ffcc1e18818a68f0e673a8fda5281 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 19 Aug 2022 15:11:53 -0700 Subject: [PATCH 2/2] Address the feedback --- .../src/ServiceLookup/ServiceProviderEngineScope.cs | 5 ++++- .../src/ServiceProvider.cs | 2 ++ .../tests/DI.Tests/ServiceProviderEngineScopeTests.cs | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index fb70e4a6377e6e..d1af024a3e38e1 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -189,8 +189,11 @@ static async ValueTask Await(int i, ValueTask vt, List toDispose) } - if (IsRootScope) + if (IsRootScope && !RootProvider.IsDisposed()) { + // If this ServiceProviderEngineScope instance is a root scope, disposing this instance will need to dispose the RootProvider too. + // Otherwise the RootProvider will never get disposed and will leak. + // Note, if the RootProvider get disposed first, it will automatically dispose all attached ServiceProviderEngineScope objects. RootProvider.Dispose(); } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs index f66f36b3cf6ed2..dd9b1af11a55a0 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs @@ -89,6 +89,8 @@ internal ServiceProvider(ICollection serviceDescriptors, Serv /// The service that was produced. public object? GetService(Type serviceType) => GetService(serviceType, Root); + internal bool IsDisposed() => _disposed; + /// public void Dispose() { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs index a6dbe1c92bfbf8..e801497236f0b9 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs @@ -20,7 +20,7 @@ public void DoubleDisposeWorks() } [Fact] - public void RootDisposeTest() + public void RootEngineScopeDisposeTest() { var services = new ServiceCollection(); ServiceProvider sp = services.BuildServiceProvider();