diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs index fdca10e7696204..19616694cd0be1 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/DependencyInjectionEventSource.cs @@ -56,6 +56,21 @@ public void DynamicMethodBuilt(string serviceType, int methodSize) WriteEvent(4, serviceType, methodSize); } + [Event(5, Level = EventLevel.Verbose)] + public void ScopeDisposed(int serviceProviderHashCode, int scopedServicesResolved, int disposableServices) + { + WriteEvent(5, serviceProviderHashCode, scopedServicesResolved, disposableServices); + } + + [NonEvent] + public void ScopeDisposed(ServiceProviderEngine engine, ScopeState state) + { + if (IsEnabled(EventLevel.Verbose, EventKeywords.All)) + { + ScopeDisposed(engine.GetHashCode(), state.ResolvedServicesCount, state.DisposableServicesCount); + } + } + [NonEvent] public void ServiceResolved(Type serviceType) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs deleted file mode 100644 index b077306e5a5a9f..00000000000000 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopePool.cs +++ /dev/null @@ -1,74 +0,0 @@ -// 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.Diagnostics; -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 const int MaxQueueSize = 128; - - 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) > 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; - // 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(); - } - - 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(); - } - - public bool Return() - { - return _pool?.Return(this) ?? false; - } - } - } -} diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs new file mode 100644 index 00000000000000..edc06906a8e2d0 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ScopeState.cs @@ -0,0 +1,30 @@ +// 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 Microsoft.Extensions.DependencyInjection.ServiceLookup; + +namespace Microsoft.Extensions.DependencyInjection +{ + internal class ScopeState + { + public IDictionary ResolvedServices { get; } + public List Disposables { get; set; } + + public int DisposableServicesCount => Disposables?.Count ?? 0; + public int ResolvedServicesCount => ResolvedServices.Count; + + public ScopeState(bool isRoot) + { + // When isRoot is true to reduce lock contention for singletons upon resolve we use a concurrent dictionary. + ResolvedServices = isRoot ? new ConcurrentDictionary() : new Dictionary(); + } + + public void Track(ServiceProviderEngine engine) + { + DependencyInjectionEventSource.Log.ScopeDisposed(engine, this); + } + } +} diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs index 48bab2f5fa5101..d6d804c6484482 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngine.cs @@ -25,11 +25,8 @@ 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 b29b4016bc0f5b..ab31fabff48e4a 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs @@ -14,22 +14,20 @@ internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvid internal Action _captureDisposableCallback; private bool _disposed; - 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, since ResolvedServices is a concurrent dictionary. - // For other scopes, it protects ResolvedServices and the list of disposables - private readonly object _scopeLock = new object(); + private readonly ScopeState _state; public ServiceProviderEngineScope(ServiceProviderEngine engine, bool isRoot = false) { Engine = engine; - _state = isRoot ? new ScopePool.State() : engine.ScopePool.Rent(); + _state = new ScopeState(isRoot); } - internal IDictionary ResolvedServices => _state?.ResolvedServices ?? ScopeDisposed(); + internal IDictionary ResolvedServices => _state.ResolvedServices; - internal object Sync => _scopeLock; + // This lock protects state on the scope, in particular, for the root scope, it protects + // the list of disposable entries only, since ResolvedServices is a concurrent dictionary. + // For other scopes, it protects ResolvedServices and the list of disposables + internal object Sync => _state; public ServiceProviderEngine Engine { get; } @@ -54,7 +52,7 @@ internal object CaptureDisposable(object service) return service; } - lock (_scopeLock) + lock (Sync) { if (_disposed) { @@ -97,8 +95,6 @@ public void Dispose() } } } - - ClearState(); } public ValueTask DisposeAsync() @@ -117,7 +113,7 @@ public ValueTask DisposeAsync() ValueTask vt = asyncDisposable.DisposeAsync(); if (!vt.IsCompletedSuccessfully) { - return Await(this, i, vt, toDispose); + return Await(i, vt, toDispose); } // If its a IValueTaskSource backed ValueTask, @@ -136,11 +132,9 @@ public ValueTask DisposeAsync() } } - ClearState(); - return default; - static async ValueTask Await(ServiceProviderEngineScope scope, int i, ValueTask vt, List toDispose) + static async ValueTask Await(int i, ValueTask vt, List toDispose) { await vt.ConfigureAwait(false); // vt is acting on the disposable at index i, @@ -159,56 +153,30 @@ static async ValueTask Await(ServiceProviderEngineScope scope, int i, ValueTask ((IDisposable)disposable).Dispose(); } } - - scope.ClearState(); - } - } - - private IDictionary ScopeDisposed() - { - ThrowHelper.ThrowObjectDisposedException(); - return null; - } - - private void ClearState() - { - // 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) - { - // Don't attempt to dispose if we're already disposed - if (_state == null) - { - return; - } - - // 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()) - { - _state = null; - } } } private List BeginDispose() { - lock (_scopeLock) + lock (Sync) { if (_disposed) { return null; } + // Track statistics about the scope (number of disposable objects and number of disposed services) + _state.Track(Engine); + // 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; + // 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 _state.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 223c6bfd7bc8a9..95fde1c98e8eaa 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs @@ -2,7 +2,6 @@ // 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; @@ -10,17 +9,6 @@ namespace Microsoft.Extensions.DependencyInjection.ServiceLookup { public class ServiceProviderEngineScopeTests { - [Fact] - public void ResolvedServicesAfterDispose_ThrowsObjectDispose() - { - var engine = new FakeEngine(); - var serviceProviderEngineScope = new ServiceProviderEngineScope(engine); - serviceProviderEngineScope.ResolvedServices.Add(new ServiceCacheKey(typeof(IFakeService), 0), null); - serviceProviderEngineScope.Dispose(); - - Assert.Throws(() => serviceProviderEngineScope.ResolvedServices); - } - [Fact] public void DoubleDisposeWorks() {