From 0c4bea0472085f542d8a0862308faec689a149ff Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 30 Mar 2021 18:45:17 -0700 Subject: [PATCH 1/5] Pool the underlying list and dictionary in scopes. - This change pools a set of scopes assuming they are short lived. - One breaking change is that after disposal, pooled scopes will throw if services are accessed afterwards on the scope. --- .../src/ScopePool.cs | 69 ++++++++++++++++++ .../ServiceLookup/CallSiteRuntimeResolver.cs | 6 +- .../Expressions/ExpressionResolverBuilder.cs | 16 +++-- .../ILEmit/ILEmitResolverBuilder.cs | 17 ++++- .../ServiceLookup/ServiceProviderEngine.cs | 3 + .../ServiceProviderEngineScope.cs | 70 +++++++++++++------ 6 files changed, 149 insertions(+), 32 deletions(-) create mode 100644 src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs new file mode 100644 index 0000000000000..86bfb58319506 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs @@ -0,0 +1,69 @@ +// 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 System.Collections.Concurrent; +using System.Collections.Generic; +using System.Threading; +using Microsoft.Extensions.DependencyInjection.ServiceLookup; + +namespace Microsoft.Extensions.DependencyInjection +{ + internal class ScopePool + { + // Modest number to re-use. We only really care about reuse for short lived scopes + private static readonly int s_maxQueueSize = Environment.ProcessorCount * 2; + + private int _count; + private readonly ConcurrentQueue _queue = new(); + + public State Rent() + { + if (_queue.TryDequeue(out State state)) + { + Interlocked.Decrement(ref _count); + return state; + } + return new State(this); + } + + public bool Return(State state) + { + if (Interlocked.Increment(ref _count) > s_maxQueueSize) + { + Interlocked.Decrement(ref _count); + return false; + } + + state.Clear(); + _queue.Enqueue(state); + return true; + } + + public class State + { + private readonly ScopePool _pool; + + public IDictionary ResolvedServices { get; } + public List Disposables { get; set; } + + public State(ScopePool pool = null) + { + _pool = pool; + ResolvedServices = pool == null ? new ConcurrentDictionary() : new Dictionary(); + } + + internal void Clear() + { + // REVIEW: Should we trim excess here as well? + ResolvedServices.Clear(); + Disposables?.Clear(); + } + + public bool Return() + { + return _pool?.Return(this) ?? false; + } + } + } +} diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs index fd5c896278bbd..4db50bcd63314 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs @@ -90,7 +90,7 @@ protected override object VisitScopeCache(ServiceCallSite callSite, RuntimeResol private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext context, ServiceProviderEngineScope serviceProviderEngine, RuntimeResolverLock lockType) { bool lockTaken = false; - IDictionary resolvedServices = serviceProviderEngine.ResolvedServices; + object sync = serviceProviderEngine.Sync; // Taking locks only once allows us to fork resolution process // on another thread without causing the deadlock because we @@ -98,7 +98,7 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte // releasing the lock if ((context.AcquiredLocks & lockType) == 0) { - Monitor.Enter(resolvedServices, ref lockTaken); + Monitor.Enter(sync, ref lockTaken); } try @@ -109,7 +109,7 @@ private object VisitCache(ServiceCallSite callSite, RuntimeResolverContext conte { if (lockTaken) { - Monitor.Exit(resolvedServices); + Monitor.Exit(sync); } } } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs index cfc9f9f21e63f..4ad093a9f6ce0 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/Expressions/ExpressionResolverBuilder.cs @@ -26,12 +26,19 @@ internal sealed class ExpressionResolverBuilder : CallSiteVisitor), ScopeParameter.Name + "resolvedServices"); + private static readonly ParameterExpression Sync = Expression.Variable(typeof(object), ScopeParameter.Name + "sync"); private static readonly BinaryExpression ResolvedServicesVariableAssignment = Expression.Assign(ResolvedServices, Expression.Property( ScopeParameter, typeof(ServiceProviderEngineScope).GetProperty(nameof(ServiceProviderEngineScope.ResolvedServices), BindingFlags.Instance | BindingFlags.NonPublic))); + private static readonly BinaryExpression SyncVariableAssignment = + Expression.Assign(Sync, + Expression.Property( + ScopeParameter, + typeof(ServiceProviderEngineScope).GetProperty(nameof(ServiceProviderEngineScope.Sync), BindingFlags.Instance | BindingFlags.NonPublic))); + private static readonly ParameterExpression CaptureDisposableParameter = Expression.Parameter(typeof(object)); private static readonly LambdaExpression CaptureDisposable = Expression.Lambda( Expression.Call(ScopeParameter, CaptureDisposableMethodInfo, CaptureDisposableParameter), @@ -96,8 +103,9 @@ private Expression> BuildExpression(Ser { return Expression.Lambda>( Expression.Block( - new[] { ResolvedServices }, + new[] { ResolvedServices, Sync }, ResolvedServicesVariableAssignment, + SyncVariableAssignment, BuildScopedExpression(callSite)), ScopeParameter); } @@ -213,7 +221,6 @@ protected override Expression VisitScopeCache(ServiceCallSite callSite, object c // Move off the main stack private Expression BuildScopedExpression(ServiceCallSite callSite) { - ConstantExpression keyExpression = Expression.Constant( callSite.Cache.Key, typeof(ServiceCacheKey)); @@ -257,9 +264,10 @@ private Expression BuildScopedExpression(ServiceCallSite callSite) // The C# compiler would copy the lock object to guard against mutation. // We don't, since we know the lock object is readonly. ParameterExpression lockWasTaken = Expression.Variable(typeof(bool), "lockWasTaken"); + ParameterExpression sync = Sync; - MethodCallExpression monitorEnter = Expression.Call(MonitorEnterMethodInfo, resolvedServices, lockWasTaken); - MethodCallExpression monitorExit = Expression.Call(MonitorExitMethodInfo, resolvedServices); + MethodCallExpression monitorEnter = Expression.Call(MonitorEnterMethodInfo, sync, lockWasTaken); + MethodCallExpression monitorExit = Expression.Call(MonitorExitMethodInfo, sync); BlockExpression tryBody = Expression.Block(monitorEnter, blockExpression); ConditionalExpression finallyBody = Expression.IfThen(lockWasTaken, monitorExit); diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs index 0843602e1401f..52026aa509090 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs @@ -14,6 +14,9 @@ internal sealed class ILEmitResolverBuilder : CallSiteVisitor)); + LocalBuilder syncLocal = context.Generator.DeclareLocal(typeof(object)); LocalBuilder lockTakenLocal = context.Generator.DeclareLocal(typeof(bool)); LocalBuilder resultLocal = context.Generator.DeclareLocal(typeof(object)); @@ -339,8 +343,15 @@ private ILEmitResolverBuilderRuntimeContext GenerateMethodBody(ServiceCallSite c // Store resolved services Stloc(context.Generator, resolvedServicesLocal.LocalIndex); - // Load resolvedServices - Ldloc(context.Generator, resolvedServicesLocal.LocalIndex); + // scope + context.Generator.Emit(OpCodes.Ldarg_1); + // .Sync + context.Generator.Emit(OpCodes.Callvirt, ScopeLockGetter); + // Store syncLocal + Stloc(context.Generator, syncLocal.LocalIndex); + + // Load syncLocal + Ldloc(context.Generator, syncLocal.LocalIndex); // Load address of lockTaken context.Generator.Emit(OpCodes.Ldloca_S, lockTakenLocal.LocalIndex); // Monitor.Enter @@ -389,7 +400,7 @@ private ILEmitResolverBuilderRuntimeContext GenerateMethodBody(ServiceCallSite c // return if not context.Generator.Emit(OpCodes.Brfalse, returnLabel); // Load resolvedServices - Ldloc(context.Generator, resolvedServicesLocal.LocalIndex); + Ldloc(context.Generator, syncLocal.LocalIndex); // Monitor.Exit context.Generator.Emit(OpCodes.Call, ExpressionResolverBuilder.MonitorExitMethodInfo); diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs index d6d804c648448..48bab2f5fa510 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs @@ -25,8 +25,11 @@ protected ServiceProviderEngine(IEnumerable serviceDescriptor CallSiteFactory.Add(typeof(IServiceProvider), new ServiceProviderCallSite()); CallSiteFactory.Add(typeof(IServiceScopeFactory), new ServiceScopeFactoryCallSite()); RealizedServices = new ConcurrentDictionary>(); + ScopePool = new ScopePool(); } + internal ScopePool ScopePool { get; } + internal ConcurrentDictionary> RealizedServices { get; } internal CallSiteFactory CallSiteFactory { get; } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 52bac8dd8038c..a015310706018 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -2,9 +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; using Microsoft.Extensions.Internal; @@ -15,20 +13,25 @@ internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid // For testing only internal Action _captureDisposableCallback; - private List _disposables; - private bool _disposed; - private readonly object _disposelock = new object(); + private ScopePool.State _state; + + // This protects the disposed state + private readonly object _disposeLock = new object(); + + // This protects resolved services, this is only used if isRoot is false + private readonly object _scopeLock; 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(); + _state = isRoot ? new ScopePool.State() : engine.ScopePool.Rent(); + _scopeLock = isRoot ? null : new object(); } - internal IDictionary ResolvedServices { get; } + internal IDictionary ResolvedServices => _state?.ResolvedServices ?? ScopeDisposed(); + + internal object Sync => _scopeLock; public ServiceProviderEngine Engine { get; } @@ -53,7 +56,7 @@ internal object CaptureDisposable(object service) return service; } - lock (_disposelock) + lock (_disposeLock) { if (_disposed) { @@ -66,16 +69,17 @@ internal object CaptureDisposable(object service) // 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(); - } - if (_disposables == null) - { - _disposables = new List(); + return service; } - _disposables.Add(service); + _state.Disposables ??= new(); + + _state.Disposables.Add(service); } + return service; } @@ -97,6 +101,8 @@ public void Dispose() } } } + + ClearState(); } public ValueTask DisposeAsync() @@ -134,9 +140,11 @@ public ValueTask DisposeAsync() } } + ClearState(); + return default; - static async ValueTask Await(int i, ValueTask vt, List toDispose) + async ValueTask Await(int i, ValueTask vt, List toDispose) { await vt.ConfigureAwait(false); // vt is acting on the disposable at index i, @@ -155,30 +163,48 @@ static async ValueTask Await(int i, ValueTask vt, List toDispose) ((IDisposable)disposable).Dispose(); } } + + ClearState(); + } + } + + private IDictionary ScopeDisposed() + { + ThrowHelper.ThrowObjectDisposedException(); + return null; + } + + private void ClearState() + { + // Dispose the state, which will end up attempting to return the state pool. + // This will return false if the pool is full or if this state object is the root scope + if (_state.Return()) + { + _state = null; } } private List BeginDispose() { - List toDispose; - lock (_disposelock) + lock (_disposeLock) { if (_disposed) { return null; } + // We've transitioned to the disposed state, so future calls to + // CaptureDisposable will immediately dispose the object. + // No further changes to _state.Disposables, are allowed. _disposed = true; - toDispose = _disposables; - _disposables = null; + + return _state.Disposables; // Not clearing ResolvedServices here because there might be a compilation running in background // trying to get a cached singleton service instance and if it won't find // it it will try to create a new one tripping the Debug.Assert in CaptureDisposable // and leaking a Disposable object in Release mode } - - return toDispose; } } } From 6569493825721cd811da1be8d86ae118fcfee23e Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 30 Mar 2021 18:57:16 -0700 Subject: [PATCH 2/5] Fixed race --- .../ServiceProviderEngineScope.cs | 19 +++++++++++++++---- 1 file changed, 15 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 a015310706018..69b1f837356f5 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -176,11 +176,22 @@ private IDictionary ScopeDisposed() private void ClearState() { - // Dispose the state, which will end up attempting to return the state pool. - // This will return false if the pool is full or if this state object is the root scope - if (_state.Return()) + // Root scope doesn't need to lock anything + if (_scopeLock == null) { - _state = null; + return; + } + + // We lock here since ResolvedServices is always accessed in the scope lock, this means we'll never + // try to return to the pool while somebody is trying to access ResolvedServices. + lock (_scopeLock) + { + // Dispose the state, which will end up attempting to return the state pool. + // This will return false if the pool is full or if this state object is the root scope + if (_state.Return()) + { + _state = null; + } } } From ec7509362912811468e7a7704756bbc6fedb5179 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 30 Mar 2021 22:38:50 -0700 Subject: [PATCH 3/5] More changes - Allocate the lock. Scoped services can be resolved without a scope when the scope validation flag is off. - Assert the correct lock - Modified test to throw after dispose --- .../ServiceLookup/CallSiteRuntimeResolver.cs | 2 +- .../ServiceProviderEngineScope.cs | 3 +-- .../ServiceProviderEngineScopeTests.cs | 23 ++++++++++++++++--- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs index 4db50bcd63314..36de790a24efd 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs @@ -123,7 +123,7 @@ private object ResolveService(ServiceCallSite callSite, RuntimeResolverContext c // 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))); + (lockType == RuntimeResolverLock.Scope && Monitor.IsEntered(serviceProviderEngine.Sync))); if (resolvedServices.TryGetValue(callSite.Cache.Key, out object resolved)) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index 69b1f837356f5..b19f4af7e7a8b 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -20,13 +20,12 @@ internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid private readonly object _disposeLock = new object(); // This protects resolved services, this is only used if isRoot is false - private readonly object _scopeLock; + private readonly object _scopeLock = new object(); public ServiceProviderEngineScope(ServiceProviderEngine engine, bool isRoot = false) { Engine = engine; _state = isRoot ? new ScopePool.State() : engine.ScopePool.Rent(); - _scopeLock = isRoot ? null : new object(); } internal IDictionary ResolvedServices => _state?.ResolvedServices ?? ScopeDisposed(); 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 a4fe5b6b5afd5..ae36f2e1d64bb 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,8 @@ // 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 System.Collections.Generic; using Microsoft.Extensions.DependencyInjection.Specification.Fakes; using Xunit; @@ -9,13 +11,28 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup public class ServiceProviderEngineScopeTests { [Fact] - public void Dispose_DoesntClearResolvedServices() + public void ResolvedServicesAfterDispose_ThrowsObjectDispose() { - var serviceProviderEngineScope = new ServiceProviderEngineScope(null); + var engine = new FakeEngine(); + var serviceProviderEngineScope = new ServiceProviderEngineScope(engine); serviceProviderEngineScope.ResolvedServices.Add(new ServiceCacheKey(typeof(IFakeService), 0), null); serviceProviderEngineScope.Dispose(); - Assert.Single(serviceProviderEngineScope.ResolvedServices); + Assert.Throws(() => serviceProviderEngineScope.ResolvedServices); + } + + private class FakeEngine : ServiceProviderEngine + { + public FakeEngine() : + base(Array.Empty()) + { + } + + protected override Func RealizeService(ServiceCallSite callSite) + { + return scope => null; + } + } } } From 8ecde745a68cd45342c21f841b314f044e73757b Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 31 Mar 2021 12:39:56 -0700 Subject: [PATCH 4/5] PR feedback --- .../src/ScopePool.cs | 4 ++- .../ILEmit/ILEmitResolverBuilder.cs | 2 +- .../ServiceProviderEngineScope.cs | 25 +++++++------------ 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs index 86bfb58319506..3aa994882429e 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs @@ -12,7 +12,7 @@ namespace Microsoft.Extensions.DependencyInjection internal class ScopePool { // Modest number to re-use. We only really care about reuse for short lived scopes - private static readonly int s_maxQueueSize = Environment.ProcessorCount * 2; + private const int s_maxQueueSize = 128; private int _count; private readonly ConcurrentQueue _queue = new(); @@ -50,6 +50,8 @@ public class State public State(ScopePool pool = null) { _pool = pool; + // When pool is null, we're in the global scope which doesn't need pooling. + // To reduce lock contention for singletons upon resolve we use a concurrent dictionary. ResolvedServices = pool == null ? new ConcurrentDictionary() : new Dictionary(); } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs index 52026aa509090..25066d90e220c 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs @@ -399,7 +399,7 @@ private ILEmitResolverBuilderRuntimeContext GenerateMethodBody(ServiceCallSite c Ldloc(context.Generator, lockTakenLocal.LocalIndex); // return if not context.Generator.Emit(OpCodes.Brfalse, returnLabel); - // Load resolvedServices + // Load syncLocal Ldloc(context.Generator, syncLocal.LocalIndex); // Monitor.Exit context.Generator.Emit(OpCodes.Call, ExpressionResolverBuilder.MonitorExitMethodInfo); diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index b19f4af7e7a8b..a87b59573d92f 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -16,10 +16,9 @@ internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid private bool _disposed; private ScopePool.State _state; - // This protects the disposed state - private readonly object _disposeLock = new object(); - - // This protects resolved services, this is only used if isRoot is false + // This lock protects state on the scope, in particular, for the root scope, it protects + // the list of disposable entries only as ResolvedServices is a concurrent dictionary + // For other scopes, it protects ResolvedServices and the list of disposables private readonly object _scopeLock = new object(); public ServiceProviderEngineScope(ServiceProviderEngine engine, bool isRoot = false) @@ -55,7 +54,7 @@ internal object CaptureDisposable(object service) return service; } - lock (_disposeLock) + lock (_scopeLock) { if (_disposed) { @@ -74,7 +73,7 @@ internal object CaptureDisposable(object service) return service; } - _state.Disposables ??= new(); + _state.Disposables ??= new List(); _state.Disposables.Add(service); } @@ -120,7 +119,7 @@ public ValueTask DisposeAsync() ValueTask vt = asyncDisposable.DisposeAsync(); if (!vt.IsCompletedSuccessfully) { - return Await(i, vt, toDispose); + return Await(this, i, vt, toDispose); } // If its a IValueTaskSource backed ValueTask, @@ -143,7 +142,7 @@ public ValueTask DisposeAsync() return default; - async ValueTask Await(int i, ValueTask vt, List toDispose) + static async ValueTask Await(ServiceProviderEngineScope scope, int i, ValueTask vt, List toDispose) { await vt.ConfigureAwait(false); // vt is acting on the disposable at index i, @@ -163,7 +162,7 @@ async ValueTask Await(int i, ValueTask vt, List toDispose) } } - ClearState(); + scope.ClearState(); } } @@ -175,12 +174,6 @@ private IDictionary ScopeDisposed() private void ClearState() { - // Root scope doesn't need to lock anything - if (_scopeLock == null) - { - return; - } - // We lock here since ResolvedServices is always accessed in the scope lock, this means we'll never // try to return to the pool while somebody is trying to access ResolvedServices. lock (_scopeLock) @@ -196,7 +189,7 @@ private void ClearState() private List BeginDispose() { - lock (_disposeLock) + lock (_scopeLock) { if (_disposed) { From aaf95cf795564b6a24a40299b7760db082a5966d Mon Sep 17 00:00:00 2001 From: David Fowler Date: Thu, 1 Apr 2021 10:53:07 -0700 Subject: [PATCH 5/5] PR feedback --- .../src/ScopePool.cs | 7 +++++-- .../src/ServiceLookup/ServiceProviderEngineScope.cs | 13 +++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs index 3aa994882429e..b077306e5a5a9 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Threading; using Microsoft.Extensions.DependencyInjection.ServiceLookup; @@ -12,7 +13,7 @@ namespace Microsoft.Extensions.DependencyInjection internal class ScopePool { // Modest number to re-use. We only really care about reuse for short lived scopes - private const int s_maxQueueSize = 128; + private const int MaxQueueSize = 128; private int _count; private readonly ConcurrentQueue _queue = new(); @@ -29,7 +30,7 @@ public State Rent() public bool Return(State state) { - if (Interlocked.Increment(ref _count) > s_maxQueueSize) + if (Interlocked.Increment(ref _count) > MaxQueueSize) { Interlocked.Decrement(ref _count); return false; @@ -57,6 +58,8 @@ public State(ScopePool pool = null) internal void Clear() { + // This should only get called from the pool + Debug.Assert(_pool != null); // REVIEW: Should we trim excess here as well? ResolvedServices.Clear(); Disposables?.Clear(); diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs index a87b59573d92f..656cf9b10c4f1 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 sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid private ScopePool.State _state; // This lock protects state on the scope, in particular, for the root scope, it protects - // the list of disposable entries only as ResolvedServices is a concurrent dictionary + // the list of disposable entries only, since ResolvedServices is a concurrent dictionary. // For other scopes, it protects ResolvedServices and the list of disposables private readonly object _scopeLock = new object(); @@ -69,8 +69,6 @@ internal object CaptureDisposable(object service) } ThrowHelper.ThrowObjectDisposedException(); - - return service; } _state.Disposables ??= new List(); @@ -178,6 +176,10 @@ private void ClearState() // try to return to the pool while somebody is trying to access ResolvedServices. lock (_scopeLock) { + // 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. + // Dispose the state, which will end up attempting to return the state pool. // This will return false if the pool is full or if this state object is the root scope if (_state.Return()) @@ -202,11 +204,6 @@ private List BeginDispose() _disposed = true; return _state.Disposables; - - // Not clearing ResolvedServices here because there might be a compilation running in background - // trying to get a cached singleton service instance and if it won't find - // it it will try to create a new one tripping the Debug.Assert in CaptureDisposable - // and leaking a Disposable object in Release mode } } }