From ba6cc3b761fe3300038d617e6408c24f629abe0c Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Mon, 23 Nov 2020 09:21:35 -0800 Subject: [PATCH 01/18] Breaking change: Throw when resolving on a disposed service provider - Resolving Services after ServiceProvider is Disposed should throw with ObjectDisposedException --- .../ServiceProviderEngineScope.cs | 16 +++-- .../DI.Tests/ServiceProviderContainerTests.cs | 66 +++++++++++++++++++ 2 files changed, 78 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 b8a330642cf40..d3fa93e7f236c 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -17,6 +17,7 @@ internal class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IAs private List _disposables; private bool _disposed; + private static object s_locker = new object(); public ServiceProviderEngineScope(ServiceProviderEngine engine) { @@ -41,8 +42,6 @@ public object GetService(Type serviceType) internal object CaptureDisposable(object service) { - Debug.Assert(!_disposed); - _captureDisposableCallback?.Invoke(service); if (ReferenceEquals(this, service) || !(service is IDisposable || service is IAsyncDisposable)) @@ -50,8 +49,17 @@ internal object CaptureDisposable(object service) return service; } - lock (ResolvedServices) + lock (s_locker) { + if (_disposed) + { + if (_disposables != null) + { + // cleanup disposable just in case still has items before throwing + _disposables = null; + } + ThrowHelper.ThrowObjectDisposedException(); + } if (_disposables == null) { _disposables = new List(); @@ -144,7 +152,7 @@ static async ValueTask Await(int i, ValueTask vt, List toDispose) private List BeginDispose() { List toDispose; - lock (ResolvedServices) + lock (s_locker) { if (_disposed) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 0ede07896f5b5..fae884bd4d65d 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -4,7 +4,9 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Fakes; using Microsoft.Extensions.DependencyInjection.Specification; using Microsoft.Extensions.DependencyInjection.Specification.Fakes; @@ -264,6 +266,70 @@ public void ScopeDispose_PreventsServiceResolution() //Check that resolution from root works Assert.NotNull(provider.CreateScope()); } + + [ThreadStatic] + public static int ThreadId; + + [Fact] + public async Task ServiceProviderDispose_ContinueResolvingServices_Throws() + { + var services = new ServiceCollection(); + ServiceProvider sp = null; + + var lazy = new Lazy(() => + { + // Tries to take the singleton lock (global) + return new Thing1(sp); + }); + + services.AddTransient(sp => + { + if (ThreadId == 1) + { + Thread.Sleep(1000); + } + else + { + // Let Thread 1 over take Thread 2 + Thread.Sleep(3000); + } + + return lazy.Value; + }); + services.AddSingleton(); + + sp = services.BuildServiceProvider(); + + var t1 = Task.Run(() => + { + ThreadId = 1; + using var scope1 = sp.CreateScope(); + scope1.ServiceProvider.GetRequiredService(); + }); + + var t2 = Task.Run(() => + { + ThreadId = 2; + using var scope2 = sp.CreateScope(); + scope2.ServiceProvider.GetRequiredService(); + }); + + await Assert.ThrowsAsync(async () => {await t1; await t2;}); + } + + public class Thing2 : IDisposable + { + public Thing2(Thing1 thing1) { } + public void Dispose() { } + } + + public class Thing1 + { + public Thing1(ServiceProvider sp) + { + sp.Dispose(); + } + } [ActiveIssue("https://github.com/dotnet/runtime/issues/42160")] // We don't support value task services currently [Theory] From f5fae4c48cbb156911a1b030f2724cec838acb1c Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Mon, 23 Nov 2020 09:45:09 -0800 Subject: [PATCH 02/18] Update src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs --- .../tests/DI.Tests/ServiceProviderContainerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index fae884bd4d65d..1080dd7eaaeb9 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -314,7 +314,7 @@ public async Task ServiceProviderDispose_ContinueResolvingServices_Throws() scope2.ServiceProvider.GetRequiredService(); }); - await Assert.ThrowsAsync(async () => {await t1; await t2;}); + await Assert.ThrowsAsync(async () => { await t1; await t2; }); } public class Thing2 : IDisposable From 91d6053d17a78ffb0305516c0070c68fee6b806a Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Mon, 23 Nov 2020 15:26:42 -0800 Subject: [PATCH 03/18] PR feedback + Add simpler test --- .../ServiceProviderEngineScope.cs | 6 +- .../DI.Tests/ServiceProviderContainerTests.cs | 82 ++++++++++--------- 2 files changed, 47 insertions(+), 41 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index d3fa93e7f236c..ef4eed8ba1aef 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -17,7 +17,7 @@ internal class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IAs private List _disposables; private bool _disposed; - private static object s_locker = new object(); + private object _locker = new object(); public ServiceProviderEngineScope(ServiceProviderEngine engine) { @@ -49,7 +49,7 @@ internal object CaptureDisposable(object service) return service; } - lock (s_locker) + lock (_locker) { if (_disposed) { @@ -152,7 +152,7 @@ static async ValueTask Await(int i, ValueTask vt, List toDispose) private List BeginDispose() { List toDispose; - lock (s_locker) + lock (_locker) { if (_disposed) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 1080dd7eaaeb9..a98e6b504c6a6 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -6,7 +6,6 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; -using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Fakes; using Microsoft.Extensions.DependencyInjection.Specification; using Microsoft.Extensions.DependencyInjection.Specification.Fakes; @@ -266,55 +265,62 @@ public void ScopeDispose_PreventsServiceResolution() //Check that resolution from root works Assert.NotNull(provider.CreateScope()); } - - [ThreadStatic] - public static int ThreadId; [Fact] - public async Task ServiceProviderDispose_ContinueResolvingServices_Throws() + private void GetService_ThenDisposeOnDifferentThread_ServiceDisposed() { var services = new ServiceCollection(); - ServiceProvider sp = null; + services.AddSingleton(); + var sp = services.BuildServiceProvider(); + var foo = sp.GetRequiredService(); + Task.Run(() => sp.Dispose()).Wait(); + + Assert.Equal( + Foo.TimesConstructorCalled, + Foo.TimesDisposeCalled); + } - var lazy = new Lazy(() => + [Fact] + public void GetService_DisposeOnSameThread_Throws() + { + var services = new ServiceCollection(); + services.AddSingleton(); + var sp = services.BuildServiceProvider(); + Assert.Throws(() => { - // Tries to take the singleton lock (global) - return new Thing1(sp); + // foo ctor disposes ServiceProvider + var foo = sp.GetRequiredService(); }); + } - services.AddTransient(sp => + public class Foo : IDisposable + { + public static int TimesConstructorCalled = 0; + public static int TimesDisposeCalled = 0; + public Foo() { - if (ThreadId == 1) - { - Thread.Sleep(1000); - } - else - { - // Let Thread 1 over take Thread 2 - Thread.Sleep(3000); - } - - return lazy.Value; - }); - services.AddSingleton(); - - sp = services.BuildServiceProvider(); - - var t1 = Task.Run(() => + Interlocked.Increment(ref TimesConstructorCalled); + Thread.Sleep(5000); + } + public void Dispose() { - ThreadId = 1; - using var scope1 = sp.CreateScope(); - scope1.ServiceProvider.GetRequiredService(); - }); + Interlocked.Increment(ref TimesDisposeCalled); + } + } - var t2 = Task.Run(() => + public class DisposableFoo : IDisposable + { + public static int TimesConstructorCalled = 0; + public static int TimesDisposeCalled = 0; + public DisposableFoo(IServiceProvider sp) { - ThreadId = 2; - using var scope2 = sp.CreateScope(); - scope2.ServiceProvider.GetRequiredService(); - }); - - await Assert.ThrowsAsync(async () => { await t1; await t2; }); + Interlocked.Increment(ref TimesConstructorCalled); + (sp as IDisposable).Dispose(); + } + public void Dispose() + { + Interlocked.Increment(ref TimesDisposeCalled); + } } public class Thing2 : IDisposable From 6d3f4fa7281127ab5b4165cb4de4eeb0cf6c047d Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Mon, 23 Nov 2020 16:06:18 -0800 Subject: [PATCH 04/18] Dispose missed disposables before throwing + Cleanup --- .../ServiceProviderEngineScope.cs | 10 ++-- .../DI.Tests/ServiceProviderContainerTests.cs | 49 +++++-------------- 2 files changed, 20 insertions(+), 39 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index ef4eed8ba1aef..8c045e9408ebb 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -17,7 +17,7 @@ internal class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IAs private List _disposables; private bool _disposed; - private object _locker = new object(); + private object _lock = new object(); public ServiceProviderEngineScope(ServiceProviderEngine engine) { @@ -49,13 +49,17 @@ internal object CaptureDisposable(object service) return service; } - lock (_locker) + lock (_lock) { if (_disposed) { if (_disposables != null) { // cleanup disposable just in case still has items before throwing + foreach (IDisposable disposable in _disposables) + { + disposable.Dispose(); + } _disposables = null; } ThrowHelper.ThrowObjectDisposedException(); @@ -152,7 +156,7 @@ static async ValueTask Await(int i, ValueTask vt, List toDispose) private List BeginDispose() { List toDispose; - lock (_locker) + lock (_lock) { if (_disposed) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index a98e6b504c6a6..c4d0f0719db5b 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -270,73 +270,50 @@ public void ScopeDispose_PreventsServiceResolution() private void GetService_ThenDisposeOnDifferentThread_ServiceDisposed() { var services = new ServiceCollection(); - services.AddSingleton(); + services.AddSingleton(); var sp = services.BuildServiceProvider(); - var foo = sp.GetRequiredService(); + var foo = sp.GetRequiredService(); Task.Run(() => sp.Dispose()).Wait(); - Assert.Equal( - Foo.TimesConstructorCalled, - Foo.TimesDisposeCalled); + Assert.True(foo.IsDisposed); } [Fact] public void GetService_DisposeOnSameThread_Throws() { var services = new ServiceCollection(); - services.AddSingleton(); + services.AddSingleton(); var sp = services.BuildServiceProvider(); Assert.Throws(() => { - // foo ctor disposes ServiceProvider - var foo = sp.GetRequiredService(); + // ctor disposes ServiceProvider + var foo = sp.GetRequiredService(); }); } - public class Foo : IDisposable + public class Foo1 : IDisposable { - public static int TimesConstructorCalled = 0; - public static int TimesDisposeCalled = 0; - public Foo() + public Foo1() { - Interlocked.Increment(ref TimesConstructorCalled); Thread.Sleep(5000); } + public bool IsDisposed { get; private set; } + public void Dispose() { - Interlocked.Increment(ref TimesDisposeCalled); + IsDisposed = true; } } - public class DisposableFoo : IDisposable + public class Foo2 : IDisposable { - public static int TimesConstructorCalled = 0; - public static int TimesDisposeCalled = 0; - public DisposableFoo(IServiceProvider sp) + public Foo2(IServiceProvider sp) { - Interlocked.Increment(ref TimesConstructorCalled); (sp as IDisposable).Dispose(); } - public void Dispose() - { - Interlocked.Increment(ref TimesDisposeCalled); - } - } - - public class Thing2 : IDisposable - { - public Thing2(Thing1 thing1) { } public void Dispose() { } } - public class Thing1 - { - public Thing1(ServiceProvider sp) - { - sp.Dispose(); - } - } - [ActiveIssue("https://github.com/dotnet/runtime/issues/42160")] // We don't support value task services currently [Theory] [InlineData(ServiceLifetime.Transient)] From 9d9c9ae86a7516ad41fd9df4c81eaeac70c262c5 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Mon, 23 Nov 2020 16:14:32 -0800 Subject: [PATCH 05/18] Remove unnecessary line --- .../src/ServiceLookup/ServiceProviderEngineScope.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 8c045e9408ebb..891110fa0a870 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -60,7 +60,6 @@ internal object CaptureDisposable(object service) { disposable.Dispose(); } - _disposables = null; } ThrowHelper.ThrowObjectDisposedException(); } From 3931f618b96a1c18f1b481ddb49cf7b72c01793a Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Tue, 24 Nov 2020 08:36:30 -0800 Subject: [PATCH 06/18] dispose service before throw --- .../src/ServiceLookup/ServiceProviderEngineScope.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 891110fa0a870..23073928252b7 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -53,13 +53,13 @@ internal object CaptureDisposable(object service) { if (_disposed) { - if (_disposables != null) + if (service is IDisposable disposable) { - // cleanup disposable just in case still has items before throwing - foreach (IDisposable disposable in _disposables) - { - disposable.Dispose(); - } + disposable.Dispose(); + } + else + { + ((IAsyncDisposable)service).DisposeAsync().AsTask().Wait(); } ThrowHelper.ThrowObjectDisposedException(); } From 5a1270495dc39c81f6d40f4a779fd5d832e69139 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Tue, 24 Nov 2020 08:45:21 -0800 Subject: [PATCH 07/18] Add test code coverage for IAsyncDispoable service --- .../DI.Tests/ServiceProviderContainerTests.cs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index c4d0f0719db5b..37c88e34a4b11 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -291,6 +291,19 @@ public void GetService_DisposeOnSameThread_Throws() }); } + [Fact] + public void GetAsyncService_DisposeAsyncOnSameThread_Throws() + { + var services = new ServiceCollection(); + services.AddSingleton(); + var sp = services.BuildServiceProvider(); + Assert.Throws(() => + { + // ctor disposes ServiceProvider + var foo = sp.GetRequiredService(); + }); + } + public class Foo1 : IDisposable { public Foo1() @@ -314,6 +327,18 @@ public Foo2(IServiceProvider sp) public void Dispose() { } } + private class FooAsyncDisposable : IFakeService, IAsyncDisposable + { + public FooAsyncDisposable(IServiceProvider sp) + { + (sp as IDisposable).Dispose(); + } + public ValueTask DisposeAsync() + { + return new ValueTask(Task.CompletedTask); + } + } + [ActiveIssue("https://github.com/dotnet/runtime/issues/42160")] // We don't support value task services currently [Theory] [InlineData(ServiceLifetime.Transient)] From eddbd6f621b1e70f19287f030ab55ba2676dcfec Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Tue, 24 Nov 2020 09:30:51 -0800 Subject: [PATCH 08/18] correction --- .../src/ServiceLookup/ServiceProviderEngineScope.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 23073928252b7..842a1fccc1887 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -59,7 +59,7 @@ internal object CaptureDisposable(object service) } else { - ((IAsyncDisposable)service).DisposeAsync().AsTask().Wait(); + ((IAsyncDisposable)service).DisposeAsync().AsTask().GetAwaiter().GetResult(); } ThrowHelper.ThrowObjectDisposedException(); } From 4f3690e8f54a5e82b9f6a4e61150289477e10d69 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Tue, 24 Nov 2020 11:08:02 -0800 Subject: [PATCH 09/18] cleanup --- .../tests/DI.Tests/ServiceProviderContainerTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 37c88e34a4b11..f1e279f7120e7 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -267,7 +267,7 @@ public void ScopeDispose_PreventsServiceResolution() } [Fact] - private void GetService_ThenDisposeOnDifferentThread_ServiceDisposed() + public void GetService_ThenDisposeOnDifferentThread_ServiceDisposed() { var services = new ServiceCollection(); services.AddSingleton(); @@ -304,7 +304,7 @@ public void GetAsyncService_DisposeAsyncOnSameThread_Throws() }); } - public class Foo1 : IDisposable + private class Foo1 : IDisposable { public Foo1() { @@ -318,7 +318,7 @@ public void Dispose() } } - public class Foo2 : IDisposable + private class Foo2 : IDisposable { public Foo2(IServiceProvider sp) { From fa01467fac7b4378ec6e9ba93bc15617481dc6ae Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Wed, 25 Nov 2020 07:24:18 -0800 Subject: [PATCH 10/18] Add code coverage for DisposeAsync --- .../DI.Tests/ServiceProviderContainerTests.cs | 38 +++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index f1e279f7120e7..12b4900120865 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -271,9 +271,9 @@ public void GetService_ThenDisposeOnDifferentThread_ServiceDisposed() { var services = new ServiceCollection(); services.AddSingleton(); - var sp = services.BuildServiceProvider(); + IServiceProvider sp = services.BuildServiceProvider(); var foo = sp.GetRequiredService(); - Task.Run(() => sp.Dispose()).Wait(); + Task.Run(() => (sp as IDisposable).Dispose()).Wait(); Assert.True(foo.IsDisposed); } @@ -283,7 +283,7 @@ public void GetService_DisposeOnSameThread_Throws() { var services = new ServiceCollection(); services.AddSingleton(); - var sp = services.BuildServiceProvider(); + IServiceProvider sp = services.BuildServiceProvider(); Assert.Throws(() => { // ctor disposes ServiceProvider @@ -327,6 +327,38 @@ public Foo2(IServiceProvider sp) public void Dispose() { } } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task AddDisposablesAndAsyncDisposables_DisposeAsync_AllDisposed(bool includeDelayedAsyncDisposable) + { + var services = new ServiceCollection(); + services.AddSingleton(); + services.AddSingleton(); + if (includeDelayedAsyncDisposable) + { + //forces Dispose ValueTask to be asynchronous and not be immediately completed + services.AddSingleton(); + } + IServiceProvider sp = services.BuildServiceProvider(); + var disposable = sp.GetRequiredService(); + var asyncDisposable = sp.GetRequiredService(); + DelayedAsyncDisposableService delayedAsyncDisposableService = null; + if (includeDelayedAsyncDisposable) + { + delayedAsyncDisposableService = sp.GetRequiredService(); + } + + await (sp as IAsyncDisposable).DisposeAsync(); + + Assert.True(disposable.Disposed); + Assert.True(asyncDisposable.DisposeAsyncCalled); + if (includeDelayedAsyncDisposable) + { + Assert.Equal(1, delayedAsyncDisposableService.DisposeCount); + } + } + private class FooAsyncDisposable : IFakeService, IAsyncDisposable { public FooAsyncDisposable(IServiceProvider sp) From bf3435e16a0a9c8214a2b4a3b4ca36269667bc4b Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Thu, 26 Nov 2020 14:07:33 -0800 Subject: [PATCH 11/18] include SingleThreadSynchronizationContext for DI.Tests" --- .../Microsoft.Extensions.DependencyInjection.Tests.csproj | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/Microsoft.Extensions.DependencyInjection.Tests.csproj b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/Microsoft.Extensions.DependencyInjection.Tests.csproj index 762664f48f733..bb3a008f3684d 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/Microsoft.Extensions.DependencyInjection.Tests.csproj +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/Microsoft.Extensions.DependencyInjection.Tests.csproj @@ -7,6 +7,8 @@ + @@ -15,4 +17,8 @@ + + + + From e3317ffce31754015e08cac4b5624b2c0806313f Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Thu, 26 Nov 2020 15:26:04 -0800 Subject: [PATCH 12/18] Corrections - fire-and-forget and add test --- .../ServiceProviderEngineScope.cs | 2 +- .../DI.Tests/ServiceProviderContainerTests.cs | 32 +++++++++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 842a1fccc1887..62ac7fca42e3d 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -59,7 +59,7 @@ internal object CaptureDisposable(object service) } else { - ((IAsyncDisposable)service).DisposeAsync().AsTask().GetAwaiter().GetResult(); + Task.Run(() => ((IAsyncDisposable)service).DisposeAsync().AsTask().GetAwaiter().GetResult()); } ThrowHelper.ThrowObjectDisposedException(); } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 12b4900120865..3937aec1ffd5c 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -10,6 +10,7 @@ using Microsoft.Extensions.DependencyInjection.Specification; using Microsoft.Extensions.DependencyInjection.Specification.Fakes; using Microsoft.Extensions.DependencyInjection.Tests.Fakes; +using Microsoft.Extensions.Internal; using Xunit; namespace Microsoft.Extensions.DependencyInjection.Tests @@ -292,16 +293,33 @@ public void GetService_DisposeOnSameThread_Throws() } [Fact] - public void GetAsyncService_DisposeAsyncOnSameThread_Throws() + public void GetAsyncService_DisposeAsyncOnSameThread_ThrowsAndDoesNotHang() { + // Arrange var services = new ServiceCollection(); services.AddSingleton(); var sp = services.BuildServiceProvider(); - Assert.Throws(() => + bool success = Task.Run(() => { - // ctor disposes ServiceProvider - var foo = sp.GetRequiredService(); - }); + SingleThreadedSynchronizationContext.Run(() => + { + Task.Factory.StartNew(() => + { + // Act + Assert.Throws(() => + { + // ctor disposes ServiceProvider + var foo = sp.GetRequiredService(); + }); + }, + CancellationToken.None, + TaskCreationOptions.None, + TaskScheduler.FromCurrentSynchronizationContext() + ).Wait(); + }); + }).Wait(TimeSpan.FromSeconds(10)); + + Assert.True(success); } private class Foo1 : IDisposable @@ -365,9 +383,9 @@ public FooAsyncDisposable(IServiceProvider sp) { (sp as IDisposable).Dispose(); } - public ValueTask DisposeAsync() + public async ValueTask DisposeAsync() { - return new ValueTask(Task.CompletedTask); + await Task.Yield(); } } From 5fd7abddcfbaa923305bc4bdafdb157374267e41 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Thu, 26 Nov 2020 15:53:25 -0800 Subject: [PATCH 13/18] PR feedback --- .../tests/DI.Tests/ServiceProviderContainerTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 3937aec1ffd5c..7c789bd5d7ea3 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -358,7 +358,7 @@ public async Task AddDisposablesAndAsyncDisposables_DisposeAsync_AllDisposed(boo //forces Dispose ValueTask to be asynchronous and not be immediately completed services.AddSingleton(); } - IServiceProvider sp = services.BuildServiceProvider(); + ServiceProvider sp = services.BuildServiceProvider(); var disposable = sp.GetRequiredService(); var asyncDisposable = sp.GetRequiredService(); DelayedAsyncDisposableService delayedAsyncDisposableService = null; @@ -367,7 +367,7 @@ public async Task AddDisposablesAndAsyncDisposables_DisposeAsync_AllDisposed(boo delayedAsyncDisposableService = sp.GetRequiredService(); } - await (sp as IAsyncDisposable).DisposeAsync(); + await sp.DisposeAsync(); Assert.True(disposable.Disposed); Assert.True(asyncDisposable.DisposeAsyncCalled); From 074800a582d1f895225e0283fcc8183a472a8c39 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Thu, 26 Nov 2020 22:18:04 -0800 Subject: [PATCH 14/18] Add comment --- .../src/ServiceLookup/ServiceProviderEngineScope.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 62ac7fca42e3d..a42b47e980268 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -17,7 +17,7 @@ internal class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IAs private List _disposables; private bool _disposed; - private object _lock = new object(); + private readonly object _lock = new object(); public ServiceProviderEngineScope(ServiceProviderEngine engine) { @@ -59,6 +59,9 @@ internal object CaptureDisposable(object service) } else { + // this synchronous code path could cause a deadlock if we were to block on DisposeAsync + // instead, a fire-and-forget call may cause race conditions while disposing borrowed resources. + // since this is a rare error case, we just take the fire-and-forget approach here Task.Run(() => ((IAsyncDisposable)service).DisposeAsync().AsTask().GetAwaiter().GetResult()); } ThrowHelper.ThrowObjectDisposedException(); From c9e1c54612c3e9f5ee9c6932fb1b0ccf26ac4cf0 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Fri, 27 Nov 2020 08:04:11 -0800 Subject: [PATCH 15/18] use sync over async and update comment --- .../src/ServiceLookup/ServiceProviderEngineScope.cs | 6 ++---- 1 file changed, 2 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 a42b47e980268..79c8b41d2c109 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -59,10 +59,8 @@ internal object CaptureDisposable(object service) } else { - // this synchronous code path could cause a deadlock if we were to block on DisposeAsync - // instead, a fire-and-forget call may cause race conditions while disposing borrowed resources. - // since this is a rare error case, we just take the fire-and-forget approach here - Task.Run(() => ((IAsyncDisposable)service).DisposeAsync().AsTask().GetAwaiter().GetResult()); + // sync over async, for the rare case that an object only implements IAsyncDisposable and may end up starving the thread pool. + Task.Run(() => ((IAsyncDisposable)service).DisposeAsync().AsTask()).GetAwaiter().GetResult(); } ThrowHelper.ThrowObjectDisposedException(); } From 90a2a987e18fedc62720c7ac3361043fa8714e84 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Tue, 1 Dec 2020 10:43:55 -0800 Subject: [PATCH 16/18] Test improvements + nit feedback --- .../ServiceProviderEngineScope.cs | 7 +-- .../DI.Tests/ServiceProviderContainerTests.cs | 45 ++++++++----------- 2 files changed, 23 insertions(+), 29 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 79c8b41d2c109..48335806c9e06 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -17,7 +17,7 @@ internal class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IAs private List _disposables; private bool _disposed; - private readonly object _lock = new object(); + private readonly object _disposelock = new object(); public ServiceProviderEngineScope(ServiceProviderEngine engine) { @@ -49,7 +49,7 @@ internal object CaptureDisposable(object service) return service; } - lock (_lock) + lock (_disposelock) { if (_disposed) { @@ -64,6 +64,7 @@ internal object CaptureDisposable(object service) } ThrowHelper.ThrowObjectDisposedException(); } + if (_disposables == null) { _disposables = new List(); @@ -156,7 +157,7 @@ static async ValueTask Await(int i, ValueTask vt, List toDispose) private List BeginDispose() { List toDispose; - lock (_lock) + lock (_disposelock) { if (_disposed) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 7c789bd5d7ea3..b9633d2576ff4 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -268,27 +268,27 @@ public void ScopeDispose_PreventsServiceResolution() } [Fact] - public void GetService_ThenDisposeOnDifferentThread_ServiceDisposed() + public async Task GetService_ThenDisposeOnDifferentThread_ServiceDisposed() { var services = new ServiceCollection(); - services.AddSingleton(); + services.AddSingleton(); IServiceProvider sp = services.BuildServiceProvider(); - var foo = sp.GetRequiredService(); - Task.Run(() => (sp as IDisposable).Dispose()).Wait(); + var service = sp.GetRequiredService(); + await Task.Run(() => (sp as IDisposable).Dispose()); - Assert.True(foo.IsDisposed); + Assert.True(service.IsDisposed); } [Fact] public void GetService_DisposeOnSameThread_Throws() { var services = new ServiceCollection(); - services.AddSingleton(); + services.AddSingleton(); IServiceProvider sp = services.BuildServiceProvider(); Assert.Throws(() => { // ctor disposes ServiceProvider - var foo = sp.GetRequiredService(); + var service = sp.GetRequiredService(); }); } @@ -297,34 +297,27 @@ public void GetAsyncService_DisposeAsyncOnSameThread_ThrowsAndDoesNotHang() { // Arrange var services = new ServiceCollection(); - services.AddSingleton(); + services.AddSingleton(); var sp = services.BuildServiceProvider(); bool success = Task.Run(() => { SingleThreadedSynchronizationContext.Run(() => { - Task.Factory.StartNew(() => + // Act + Assert.Throws(() => { - // Act - Assert.Throws(() => - { - // ctor disposes ServiceProvider - var foo = sp.GetRequiredService(); - }); - }, - CancellationToken.None, - TaskCreationOptions.None, - TaskScheduler.FromCurrentSynchronizationContext() - ).Wait(); + // ctor disposes ServiceProvider + var service = sp.GetRequiredService(); + }); }); }).Wait(TimeSpan.FromSeconds(10)); Assert.True(success); } - private class Foo1 : IDisposable + private class DisposableSleepInCtor : IDisposable { - public Foo1() + public DisposableSleepInCtor() { Thread.Sleep(5000); } @@ -336,9 +329,9 @@ public void Dispose() } } - private class Foo2 : IDisposable + private class DisposeServiceProviderInCtor : IDisposable { - public Foo2(IServiceProvider sp) + public DisposeServiceProviderInCtor(IServiceProvider sp) { (sp as IDisposable).Dispose(); } @@ -377,9 +370,9 @@ public async Task AddDisposablesAndAsyncDisposables_DisposeAsync_AllDisposed(boo } } - private class FooAsyncDisposable : IFakeService, IAsyncDisposable + private class DisposeServiceProviderInCtorAsyncDisposable : IFakeService, IAsyncDisposable { - public FooAsyncDisposable(IServiceProvider sp) + public DisposeServiceProviderInCtorAsyncDisposable(IServiceProvider sp) { (sp as IDisposable).Dispose(); } From 53277891f7980eb4d4348ef33598b502cc5728a5 Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Tue, 1 Dec 2020 14:26:30 -0800 Subject: [PATCH 17/18] Remove useless test --- .../DI.Tests/ServiceProviderContainerTests.cs | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index b9633d2576ff4..0bc32103a08fc 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -267,18 +267,6 @@ public void ScopeDispose_PreventsServiceResolution() Assert.NotNull(provider.CreateScope()); } - [Fact] - public async Task GetService_ThenDisposeOnDifferentThread_ServiceDisposed() - { - var services = new ServiceCollection(); - services.AddSingleton(); - IServiceProvider sp = services.BuildServiceProvider(); - var service = sp.GetRequiredService(); - await Task.Run(() => (sp as IDisposable).Dispose()); - - Assert.True(service.IsDisposed); - } - [Fact] public void GetService_DisposeOnSameThread_Throws() { @@ -315,20 +303,6 @@ public void GetAsyncService_DisposeAsyncOnSameThread_ThrowsAndDoesNotHang() Assert.True(success); } - private class DisposableSleepInCtor : IDisposable - { - public DisposableSleepInCtor() - { - Thread.Sleep(5000); - } - public bool IsDisposed { get; private set; } - - public void Dispose() - { - IsDisposed = true; - } - } - private class DisposeServiceProviderInCtor : IDisposable { public DisposeServiceProviderInCtor(IServiceProvider sp) From 7864401a4a15c0f4cfa63c215d80ec4b796c2a2c Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Wed, 2 Dec 2020 11:21:18 -0800 Subject: [PATCH 18/18] - Assert in test that service gets disposed --- .../DI.Tests/ServiceProviderContainerTests.cs | 62 +++++++++++++++++-- 1 file changed, 56 insertions(+), 6 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 0bc32103a08fc..d83a52d4f9e19 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -281,13 +281,16 @@ public void GetService_DisposeOnSameThread_Throws() } [Fact] - public void GetAsyncService_DisposeAsyncOnSameThread_ThrowsAndDoesNotHang() + public void GetAsyncService_DisposeAsyncOnSameThread_ThrowsAndDoesNotHangAndDisposeAsyncGetsCalled() { // Arrange var services = new ServiceCollection(); - services.AddSingleton(); + var asyncDisposableResource = new AsyncDisposable(); + services.AddSingleton(sp => + new DisposeServiceProviderInCtorAsyncDisposable(asyncDisposableResource, sp)); + var sp = services.BuildServiceProvider(); - bool success = Task.Run(() => + bool doesNotHang = Task.Run(() => { SingleThreadedSynchronizationContext.Run(() => { @@ -300,7 +303,35 @@ public void GetAsyncService_DisposeAsyncOnSameThread_ThrowsAndDoesNotHang() }); }).Wait(TimeSpan.FromSeconds(10)); - Assert.True(success); + Assert.True(doesNotHang); + Assert.True(asyncDisposableResource.DisposeAsyncCalled); + } + + [Fact] + public void GetService_DisposeOnSameThread_ThrowsAndDoesNotHangAndDisposeGetsCalled() + { + // Arrange + var services = new ServiceCollection(); + var disposableResource = new Disposable(); + services.AddSingleton(sp => + new DisposeServiceProviderInCtorDisposable(disposableResource, sp)); + + var sp = services.BuildServiceProvider(); + bool doesNotHang = Task.Run(() => + { + SingleThreadedSynchronizationContext.Run(() => + { + // Act + Assert.Throws(() => + { + // ctor disposes ServiceProvider + var service = sp.GetRequiredService(); + }); + }); + }).Wait(TimeSpan.FromSeconds(10)); + + Assert.True(doesNotHang); + Assert.True(disposableResource.Disposed); } private class DisposeServiceProviderInCtor : IDisposable @@ -346,16 +377,35 @@ public async Task AddDisposablesAndAsyncDisposables_DisposeAsync_AllDisposed(boo private class DisposeServiceProviderInCtorAsyncDisposable : IFakeService, IAsyncDisposable { - public DisposeServiceProviderInCtorAsyncDisposable(IServiceProvider sp) + private readonly AsyncDisposable _asyncDisposable; + + public DisposeServiceProviderInCtorAsyncDisposable(AsyncDisposable asyncDisposable, IServiceProvider sp) { - (sp as IDisposable).Dispose(); + _asyncDisposable = asyncDisposable; + (sp as IAsyncDisposable).DisposeAsync(); } public async ValueTask DisposeAsync() { + await _asyncDisposable.DisposeAsync(); await Task.Yield(); } } + private class DisposeServiceProviderInCtorDisposable : IFakeService, IDisposable + { + private readonly Disposable _disposable; + + public DisposeServiceProviderInCtorDisposable(Disposable disposable, IServiceProvider sp) + { + _disposable = disposable; + (sp as IDisposable).Dispose(); + } + public void Dispose() + { + _disposable.Dispose(); + } + } + [ActiveIssue("https://github.com/dotnet/runtime/issues/42160")] // We don't support value task services currently [Theory] [InlineData(ServiceLifetime.Transient)]