From 4824f1365ae7d8a792e659d3b6c9d77fe0e7e83f Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 19 Aug 2022 10:03:25 -0700 Subject: [PATCH 1/4] 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 4ad8b1579a0290..4ebdec55f5946f 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -185,12 +185,17 @@ private List BeginDispose() // 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 3ce7a609ba9a565f786b0653928bf3216ba23b4c Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Fri, 19 Aug 2022 15:11:53 -0700 Subject: [PATCH 2/4] 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 4ebdec55f5946f..400b38c5d0833d 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -187,8 +187,11 @@ private List BeginDispose() } - 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 db1c1e72e93497..21a9b67c1511b9 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs @@ -85,6 +85,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(); From d1ca44b9871b84994071c3b52742f9fe5f12eb77 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 22 Aug 2022 11:19:36 -0700 Subject: [PATCH 3/4] Add ServicingVersion --- .../src/Microsoft.Extensions.DependencyInjection.csproj | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj index 7ee51306d042d5..2f840f92672d6b 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj @@ -10,13 +10,14 @@ Default implementation of dependency injection for Microsoft.Extensions.DependencyInjection. false + 1 - + True $(DefineConstants);IL_EMIT - $(DefineConstants);SAVE_ASSEMBLIES + $(DefineConstants);SAVE_ASSEMBLIES @@ -36,7 +37,7 @@ - + @@ -46,7 +47,7 @@ - + From 5d18af7ee63242e2a75c755a9f5bf5ea68fb52b2 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 22 Aug 2022 12:01:34 -0700 Subject: [PATCH 4/4] Add GeneratePackageOnBuild --- .../src/Microsoft.Extensions.DependencyInjection.csproj | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj index 2f840f92672d6b..afd62c462e6e70 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj @@ -10,6 +10,7 @@ Default implementation of dependency injection for Microsoft.Extensions.DependencyInjection. false + true 1