From 6cf1b8ec012d52880d46fa4773f60ed52ddc9f3d Mon Sep 17 00:00:00 2001 From: Maryam Ariyan Date: Fri, 22 Jan 2021 22:28:53 -0500 Subject: [PATCH] Fix race conditions with resolving singletons in DI (#47310) --- .../ServiceLookup/CallSiteRuntimeResolver.cs | 38 +- .../ServiceLookup/ServiceProviderEngine.cs | 2 +- .../ServiceProviderEngineScope.cs | 8 +- .../DI.Tests/ServiceProviderContainerTests.cs | 402 ++++++++++++------ 4 files changed, 298 insertions(+), 152 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs index 8e12ab3767c77..20cb79c69fbf7 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs @@ -7,6 +7,7 @@ using System.Reflection; using System.Runtime.ExceptionServices; using System.Threading; +using System.Diagnostics; namespace Microsoft.Extensions.DependencyInjection.ServiceLookup { @@ -62,11 +63,8 @@ protected override object VisitRootCache(ServiceCallSite callSite, RuntimeResolv var lockType = RuntimeResolverLock.Root; bool lockTaken = false; - if ((context.AcquiredLocks & lockType) == 0) - { - // using more granular locking (per singleton) for the root - Monitor.Enter(callSite, ref lockTaken); - } + // using more granular locking (per singleton) for the root + Monitor.Enter(callSite, ref lockTaken); try { return ResolveService(callSite, context, lockType, serviceProviderEngine: context.Scope.Engine.Root); @@ -92,7 +90,7 @@ protected override object VisitScopeCache(ServiceCallSite callSite, RuntimeResol private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType) { bool lockTaken = false; - Dictionary resolvedServices = serviceProviderEngine.ResolvedServices; + IDictionary resolvedServices = serviceProviderEngine.ResolvedServices; // Taking locks only once allows us to fork resolution process // on another thread without causing the deadlock because we @@ -118,19 +116,27 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte private object ResolveService(ServiceCallSite callSite, RuntimeResolverContext context, RuntimeResolverLock lockType, ServiceProviderEngineScope serviceProviderEngine) { - Dictionary resolvedServices = serviceProviderEngine.ResolvedServices; - if (!resolvedServices.TryGetValue(callSite.Cache.Key, out object resolved)) - { - resolved = VisitCallSiteMain(callSite, new RuntimeResolverContext - { - Scope = serviceProviderEngine, - AcquiredLocks = context.AcquiredLocks | lockType - }); + IDictionary resolvedServices = serviceProviderEngine.ResolvedServices; - serviceProviderEngine.CaptureDisposable(resolved); - resolvedServices.Add(callSite.Cache.Key, resolved); + // Note: This method has already taken lock by the caller for resolution and access synchronization. + // For root: uses a concurrent dictionary and takes a per singleton lock for resolution. + // For scoped: takes a dictionary as both a resolution lock and a dictionary access lock. + Debug.Assert( + (lockType == RuntimeResolverLock.Root && resolvedServices is ConcurrentDictionary) || + (lockType == RuntimeResolverLock.Scope && Monitor.IsEntered(resolvedServices))); + + if (resolvedServices.TryGetValue(callSite.Cache.Key, out object resolved)) + { + return resolved; } + resolved = VisitCallSiteMain(callSite, new RuntimeResolverContext + { + Scope = serviceProviderEngine, + AcquiredLocks = context.AcquiredLocks | lockType + }); + serviceProviderEngine.CaptureDisposable(resolved); + resolvedServices.Add(callSite.Cache.Key, resolved); return resolved; } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs index ca33439284172..d6d804c648448 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs @@ -19,7 +19,7 @@ internal abstract class ServiceProviderEngine : IServiceProviderEngine, IService protected ServiceProviderEngine(IEnumerable serviceDescriptors) { _createServiceAccessor = CreateServiceAccessor; - Root = new ServiceProviderEngineScope(this); + Root = new ServiceProviderEngineScope(this, isRoot: true); RuntimeResolver = new CallSiteRuntimeResolver(); CallSiteFactory = new CallSiteFactory(serviceDescriptors); CallSiteFactory.Add(typeof(IServiceProvider), new ServiceProviderCallSite()); diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 48335806c9e06..76988982c8754 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; using System.Threading.Tasks; @@ -19,12 +20,15 @@ internal class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IAs private bool _disposed; private readonly object _disposelock = new object(); - public ServiceProviderEngineScope(ServiceProviderEngine engine) + public ServiceProviderEngineScope(ServiceProviderEngine engine, bool isRoot = false) { Engine = engine; + + // To reduce lock contention for singletons upon resolve we use a concurrent dictionary. + ResolvedServices = isRoot ? new ConcurrentDictionary() : new Dictionary(); } - internal Dictionary ResolvedServices { get; } = new Dictionary(); + internal IDictionary ResolvedServices { get; } public ServiceProviderEngine Engine { get; } 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 8bd7b0e68309e..8b8240c5d0b02 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -410,177 +410,261 @@ public void Dispose() [ThreadStatic] public static int ThreadId; - [Fact] - public async Task GetRequiredService_UsesSingletonAndLazyLocks_NoDeadlock() + private class OuterSingleton { - // Thread 1: Thing1 (transient) -> Thing0 (singleton) - // Thread 2: Thing2 (singleton) -> Thing1 (transient) -> Thing0 (singleton) + public InnerSingleton InnerSingleton; + public OuterSingleton(InnerSingleton innerSingleton) + { + InnerSingleton = innerSingleton; + } + } - // 1. Thread 1 resolves the Thing1 which is a transient service - // 2. In parallel, Thread 2 resolves Thing2 which is a singleton - // 3. Thread 1 enters the factory callback for Thing1 and takes the lazy lock - // 4. Thread 2 takes callsite for Thing2 as a singleton lock when it resolves Thing2 - // 5. Thread 2 enters the factory callback for Thing1 and waits on the lazy lock - // 6. Thread 1 calls GetRequiredService on the service provider, takes callsite for Thing0 causing no deadlock - // (rather than taking the locks that are already taken - either the lazy lock or the Thing2 callsite lock) + private class InnerSingleton + { + public InnerSingleton(ManualResetEvent mre1, ManualResetEvent mre2) + { + // Making sure ctor gets called only once + Assert.True(!mre1.WaitOne(0) && !mre2.WaitOne(0)); - Thing0 thing0 = null; - Thing1 thing1 = null; - Thing2 thing2 = null; - IServiceProvider sp = null; - var sb = new StringBuilder(); + // Then use mre2 to signal execution reached this ctor call + mre2.Set(); - // Arrange - var services = new ServiceCollection(); + // Wait until it's OK to leave ctor + mre1.WaitOne(); + } + } - var lazy = new Lazy(() => + [Fact] + public async Task GetRequiredService_ResolvingSameSingletonInTwoThreads_SameServiceReturned() + { + using (var mreForThread1 = new ManualResetEvent(false)) + using (var mreForThread2 = new ManualResetEvent(false)) + using (var mreForThread3 = new ManualResetEvent(false)) { - sb.Append("3"); - _mreForThread2.Set(); // Now that thread 1 holds lazy lock, allow thread 2 to continue + InnerSingleton innerSingleton = null; + OuterSingleton outerSingleton = null; + IServiceProvider sp = null; - // by this time, Thread 2 is holding a singleton lock for Thing2, - // and Thread one holds the lazy lock - // the call below to resolve Thing0 does not hang - // since singletons do not share the same lock upon resolve anymore. - thing0 = sp.GetRequiredService(); - return new Thing1(thing0); - }); + // Arrange + var services = new ServiceCollection(); + + services.AddSingleton(); + services.AddSingleton(sp => new InnerSingleton(mreForThread1, mreForThread2)); + + sp = services.BuildServiceProvider(); + + var t1 = Task.Run(() => + { + outerSingleton = sp.GetRequiredService(); + }); + + // Wait until mre2 gets set in InnerSingleton ctor + mreForThread2.WaitOne(); + + var t2 = Task.Run(() => + { + mreForThread3.Set(); + + // This waits on InnerSingleton singleton lock that is taken in thread 1 + innerSingleton = sp.GetRequiredService(); + }); + + mreForThread3.WaitOne(); + + // Set a timeout before unblocking execution of both thread1 and thread2 via mre1: + Assert.False(mreForThread1.WaitOne(10)); + + // By this time thread 1 has already reached InnerSingleton ctor and is waiting for mre1. + // within the GetRequiredService call, thread 2 should be waiting on a singleton lock for InnerSingleton + // (rather than trying to instantiating InnerSingleton twice). + mreForThread1.Set(); + + // Act + await t1; + await t2; - services.AddSingleton(); - services.AddTransient(sp => + // Assert + Assert.NotNull(outerSingleton); + Assert.NotNull(innerSingleton); + Assert.Same(outerSingleton.InnerSingleton, innerSingleton); + } + } + + [Fact] + public async Task GetRequiredService_UsesSingletonAndLazyLocks_NoDeadlock() + { + using (var mreForThread1 = new ManualResetEvent(false)) + using (var mreForThread2 = new ManualResetEvent(false)) { - if (ThreadId == 2) + // Thread 1: Thing1 (transient) -> Thing0 (singleton) + // Thread 2: Thing2 (singleton) -> Thing1 (transient) -> Thing0 (singleton) + + // 1. Thread 1 resolves the Thing1 which is a transient service + // 2. In parallel, Thread 2 resolves Thing2 which is a singleton + // 3. Thread 1 enters the factory callback for Thing1 and takes the lazy lock + // 4. Thread 2 takes callsite for Thing2 as a singleton lock when it resolves Thing2 + // 5. Thread 2 enters the factory callback for Thing1 and waits on the lazy lock + // 6. Thread 1 calls GetRequiredService on the service provider, takes callsite for Thing0 causing no deadlock + // (rather than taking the locks that are already taken - either the lazy lock or the Thing2 callsite lock) + + Thing0 thing0 = null; + Thing1 thing1 = null; + Thing2 thing2 = null; + IServiceProvider sp = null; + var sb = new StringBuilder(); + + // Arrange + var services = new ServiceCollection(); + + var lazy = new Lazy(() => + { + sb.Append("3"); + mreForThread2.Set(); // Now that thread 1 holds lazy lock, allow thread 2 to continue + + // by this time, Thread 2 is holding a singleton lock for Thing2, + // and Thread one holds the lazy lock + // the call below to resolve Thing0 does not hang + // since singletons do not share the same lock upon resolve anymore. + thing0 = sp.GetRequiredService(); + return new Thing1(thing0); + }); + + services.AddSingleton(); + services.AddTransient(sp => { - sb.Append("1"); - _mreForThread1.Set(); // [b] Allow thread 1 to continue execution and take the lazy lock - _mreForThread2.WaitOne(); // [c] Wait until thread 1 takes the lazy lock + if (ThreadId == 2) + { + sb.Append("1"); + mreForThread1.Set(); // [b] Allow thread 1 to continue execution and take the lazy lock + mreForThread2.WaitOne(); // [c] Wait until thread 1 takes the lazy lock - sb.Append("4"); - } + sb.Append("4"); + } // Let Thread 1 over take Thread 2 Thing1 value = lazy.Value; - return value; - }); - services.AddSingleton(); + return value; + }); + services.AddSingleton(); - sp = services.BuildServiceProvider(); + sp = services.BuildServiceProvider(); - var t1 = Task.Run(() => - { - ThreadId = 1; - using var scope1 = sp.CreateScope(); - _mreForThread1.WaitOne(); // [a] Waits until thread 2 reaches the transient call to ensure it holds Thing2 singleton lock + var t1 = Task.Run(() => + { + ThreadId = 1; + using var scope1 = sp.CreateScope(); + mreForThread1.WaitOne(); // [a] Waits until thread 2 reaches the transient call to ensure it holds Thing2 singleton lock - sb.Append("2"); - thing1 = scope1.ServiceProvider.GetRequiredService(); - }); + sb.Append("2"); + thing1 = scope1.ServiceProvider.GetRequiredService(); + }); - var t2 = Task.Run(() => - { - ThreadId = 2; - using var scope2 = sp.CreateScope(); - thing2 = scope2.ServiceProvider.GetRequiredService(); - }); + var t2 = Task.Run(() => + { + ThreadId = 2; + using var scope2 = sp.CreateScope(); + thing2 = scope2.ServiceProvider.GetRequiredService(); + }); - // Act - await t1; - await t2; + // Act + await t1; + await t2; - // Assert - Assert.NotNull(thing0); - Assert.NotNull(thing1); - Assert.NotNull(thing2); - Assert.Equal("1234", sb.ToString()); // Expected order of execution + // Assert + Assert.NotNull(thing0); + Assert.NotNull(thing1); + Assert.NotNull(thing2); + Assert.Equal("1234", sb.ToString()); // Expected order of execution + } } - private ManualResetEvent _mreForThread2 = new ManualResetEvent(false); - private ManualResetEvent _mreForThread1 = new ManualResetEvent(false); - [Fact] public async Task GetRequiredService_BiggerObjectGraphWithOpenGenerics_NoDeadlock() { // Test is similar to GetRequiredService_UsesSingletonAndLazyLocks_NoDeadlock (but for open generics and a larger object graph) + using (var mreForThread1 = new ManualResetEvent(false)) + using (var mreForThread2 = new ManualResetEvent(false)) + { + // Arrange + List> constrainedThing4Services = null; + List> constrainedThing5Services = null; - // Arrange - List> constrainedThing4Services = null; - List> constrainedThing5Services = null; - - Thing3 thing3 = null; - IServiceProvider sp = null; - var sb = new StringBuilder(); + Thing3 thing3 = null; + IServiceProvider sp = null; + var sb = new StringBuilder(); - var services = new ServiceCollection(); + var services = new ServiceCollection(); - services.AddSingleton(); - services.AddSingleton(); - services.AddSingleton(); - services.AddSingleton(); - services.AddTransient(typeof(IFakeOpenGenericService<>), typeof(FakeOpenGenericService<>)); + services.AddSingleton(); + services.AddSingleton(); + services.AddSingleton(); + services.AddSingleton(); + services.AddTransient(typeof(IFakeOpenGenericService<>), typeof(FakeOpenGenericService<>)); - var lazy = new Lazy(() => - { - sb.Append("3"); - _mreForThread2.Set(); // Now that thread 1 holds lazy lock, allow thread 2 to continue + var lazy = new Lazy(() => + { + sb.Append("3"); + mreForThread2.Set(); // Now that thread 1 holds lazy lock, allow thread 2 to continue thing3 = sp.GetRequiredService(); - return new Thing4(thing3); - }); + return new Thing4(thing3); + }); - services.AddTransient(sp => - { - if (ThreadId == 2) + services.AddTransient(sp => { - sb.Append("1"); - _mreForThread1.Set(); // [b] Allow thread 1 to continue execution and take the lazy lock - _mreForThread2.WaitOne(); // [c] Wait until thread 1 takes the lazy lock + if (ThreadId == 2) + { + sb.Append("1"); + mreForThread1.Set(); // [b] Allow thread 1 to continue execution and take the lazy lock + mreForThread2.WaitOne(); // [c] Wait until thread 1 takes the lazy lock - sb.Append("4"); - } + sb.Append("4"); + } - // Let Thread 1 over take Thread 2 - Thing4 value = lazy.Value; - return value; - }); - services.AddSingleton(); + // Let Thread 1 over take Thread 2 + Thing4 value = lazy.Value; + return value; + }); + services.AddSingleton(); - sp = services.BuildServiceProvider(); + sp = services.BuildServiceProvider(); - // Act - var t1 = Task.Run(() => - { - ThreadId = 1; - using var scope1 = sp.CreateScope(); - _mreForThread1.WaitOne(); // Waits until thread 2 reaches the transient call to ensure it holds Thing4 singleton lock + // Act + var t1 = Task.Run(() => + { + ThreadId = 1; + using var scope1 = sp.CreateScope(); + mreForThread1.WaitOne(); // Waits until thread 2 reaches the transient call to ensure it holds Thing4 singleton lock - sb.Append("2"); - constrainedThing4Services = sp.GetServices>().ToList(); - }); + sb.Append("2"); + constrainedThing4Services = sp.GetServices>().ToList(); + }); - var t2 = Task.Run(() => - { - ThreadId = 2; - using var scope2 = sp.CreateScope(); - constrainedThing5Services = sp.GetServices>().ToList(); - }); + var t2 = Task.Run(() => + { + ThreadId = 2; + using var scope2 = sp.CreateScope(); + constrainedThing5Services = sp.GetServices>().ToList(); + }); - // Act - await t1; - await t2; + // Act + await t1; + await t2; - Assert.Equal("1234", sb.ToString()); // Expected order of execution + Assert.Equal("1234", sb.ToString()); // Expected order of execution - var thing4 = sp.GetRequiredService(); - var thing5 = sp.GetRequiredService(); + var thing4 = sp.GetRequiredService(); + var thing5 = sp.GetRequiredService(); - // Assert - Assert.NotNull(thing3); - Assert.NotNull(thing4); - Assert.NotNull(thing5); - Assert.Equal(1, constrainedThing4Services.Count); - Assert.Equal(1, constrainedThing5Services.Count); - Assert.Same(thing4, constrainedThing4Services[0].Value); - Assert.Same(thing5, constrainedThing5Services[0].Value); + // Assert + Assert.NotNull(thing3); + Assert.NotNull(thing4); + Assert.NotNull(thing5); + Assert.Equal(1, constrainedThing4Services.Count); + Assert.Equal(1, constrainedThing5Services.Count); + Assert.Same(thing4, constrainedThing4Services[0].Value); + Assert.Same(thing5, constrainedThing5Services[0].Value); + } } private class Thing5 @@ -618,12 +702,7 @@ public Thing1(Thing0 thing0) } } - private class Thing0 - { - public Thing0() - { - } - } + private class Thing0 { } [ActiveIssue("https://github.com/dotnet/runtime/issues/42160")] // We don't support value task services currently [Theory] @@ -907,5 +986,62 @@ public async ValueTask DisposeAsync() DisposeCount++; } } + + [Fact] + public async Task GetRequiredService_ResolveUniqueServicesConcurrently_StressTestSuccessful() + { + for (int i = 0; i < 100; i++) + { + Assert.True(await ResolveUniqueServicesConcurrently()); + } + } + + private async Task ResolveUniqueServicesConcurrently() + { + var types = new Type[] + { + typeof(A), typeof(B), typeof(C), typeof(D), typeof(E), + typeof(F), typeof(G), typeof(H), typeof(I), typeof(J) + }; + + IServiceProvider sp = null; + var services = new ServiceCollection(); + foreach (var type in types) + { + services.AddSingleton(type); + } + + sp = services.BuildServiceProvider(); + var tasks = new List>(); + foreach (var type in types) + { + tasks.Add(Task.Run(() => + sp.GetRequiredService(type) != null) + ); + } + + await Task.WhenAll(tasks); + bool succeeded = true; + foreach (var task in tasks) + { + if (!task.Result) + { + succeeded = false; + break; + } + } + return succeeded; + } + + private class A { } + private class B { } + private class C { } + private class D { } + private class E { } + private class F { } + private class G { } + private class H { } + private class I { } + private class J { } } }