From df0c920480b75b9d125fbca2a38b8747d0971ec9 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 18 Nov 2021 14:48:20 -0800 Subject: [PATCH 01/14] WIP Add test --- .../tests/ConfigurationManagerTest.cs | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs index 256d15a16c6258..72f11b93a25cbc 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs @@ -4,6 +4,9 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; using Microsoft.Extensions.Configuration.Memory; using Microsoft.Extensions.Primitives; using Moq; @@ -171,6 +174,44 @@ public void DisposesProvidersOnRemoval() Assert.True(provider5.IsDisposed); } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] + public async Task ProvidersCanBlockLoadWhileWaitingOnAConcurrentRead() + { + // Lose xUnit's MaxConcurrencySyncContext + //await LoseExecutionContextAwaitable.Instance; + + using var mre = new ManualResetEventSlim(false); + var provider = new BlockLoadOnMREProvider(mre, timeout: TimeSpan.FromSeconds(5)); + var source = new TestConfigurationSource(provider); + + var config = new ConfigurationManager(); + IConfigurationBuilder builder = config; + + // The first load when the source is added is ignored by the provider. + builder.Add(source); + + var loadTask = Task.Run(() => + { + ((IConfigurationRoot)config).Reload(); + + //foreach (var provider in ((IConfigurationRoot)config).Providers) + //{ + // provider.Load(); + //} + }); + + await provider.LoadStartedTask; + + // Read configuration while provider.Load() is blocked waiting on us. + _ = config["key"]; + + // Unblock provider.Load() + mre.Set(); + + // This will throw if provider.Load() timed out instead of unblocking gracefully after the read. + await loadTask; + } + // Moq heavily utilizes RefEmit, which does not work on most aot workloads [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] public void DisposesChangeTokenRegistrationsOnDispose() @@ -1130,6 +1171,54 @@ public TestConfigurationProvider(string key, string value) => Data.Add(key, value); } + private class BlockLoadOnMREProvider : ConfigurationProvider + { + private readonly ManualResetEventSlim _mre; + private readonly TaskCompletionSource _loadStartedTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TimeSpan _timeout; + + private int _loadCount; + + public BlockLoadOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) + { + _mre = mre; + _timeout = timeout; + } + + public Task LoadStartedTask => _loadStartedTcs.Task; + + public override void Load() + { + _loadCount++; + + Assert.True(_loadCount <= 2, "BlockLoadOnMREProvider.Load() was called more than twice."); + + // Ignore first load when the source was added so we can set the test up. + if (_loadCount == 2) + { + _loadStartedTcs.SetResult(null); + Assert.True(_mre.Wait(_timeout), "BlockLoadOnMREProvider.Load() timed out."); + } + } + } + + private class LoseExecutionContextAwaitable : INotifyCompletion + { + public static readonly LoseExecutionContextAwaitable Instance = new(); + + public LoseExecutionContextAwaitable GetAwaiter() => this; + public bool IsCompleted => false; + + public void GetResult() + { + } + + public void OnCompleted(Action continuation) + { + ThreadPool.UnsafeQueueUserWorkItem(state => ((Action)state!)(), continuation); + } + } + private class DisposableTestConfigurationProvider : ConfigurationProvider, IDisposable { public bool IsDisposed { get; set; } From 9a625f9845f5f538aca0bd83f61e3bb9e0a24f71 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 18 Nov 2021 16:47:42 -0800 Subject: [PATCH 02/14] Simplify ProviderCanBlockLoadWaitingOnConcurrentRead --- .../tests/ConfigurationManagerTest.cs | 53 +++---------------- 1 file changed, 6 insertions(+), 47 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs index 72f11b93a25cbc..fe79496e4e9421 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Configuration.Memory; @@ -175,30 +174,17 @@ public void DisposesProvidersOnRemoval() } [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] - public async Task ProvidersCanBlockLoadWhileWaitingOnAConcurrentRead() + public async Task ProviderCanBlockLoadWaitingOnConcurrentRead() { - // Lose xUnit's MaxConcurrencySyncContext - //await LoseExecutionContextAwaitable.Instance; - using var mre = new ManualResetEventSlim(false); - var provider = new BlockLoadOnMREProvider(mre, timeout: TimeSpan.FromSeconds(5)); + var provider = new BlockLoadOnMREProvider(mre, timeout: TimeSpan.FromSeconds(30)); var source = new TestConfigurationSource(provider); var config = new ConfigurationManager(); IConfigurationBuilder builder = config; - // The first load when the source is added is ignored by the provider. - builder.Add(source); - - var loadTask = Task.Run(() => - { - ((IConfigurationRoot)config).Reload(); - - //foreach (var provider in ((IConfigurationRoot)config).Providers) - //{ - // provider.Load(); - //} - }); + // Add calls provider.Load(). + var loadTask = Task.Run(() => builder.Add(source)); await provider.LoadStartedTask; @@ -1177,8 +1163,6 @@ private class BlockLoadOnMREProvider : ConfigurationProvider private readonly TaskCompletionSource _loadStartedTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); private readonly TimeSpan _timeout; - private int _loadCount; - public BlockLoadOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) { _mre = mre; @@ -1189,33 +1173,8 @@ public BlockLoadOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) public override void Load() { - _loadCount++; - - Assert.True(_loadCount <= 2, "BlockLoadOnMREProvider.Load() was called more than twice."); - - // Ignore first load when the source was added so we can set the test up. - if (_loadCount == 2) - { - _loadStartedTcs.SetResult(null); - Assert.True(_mre.Wait(_timeout), "BlockLoadOnMREProvider.Load() timed out."); - } - } - } - - private class LoseExecutionContextAwaitable : INotifyCompletion - { - public static readonly LoseExecutionContextAwaitable Instance = new(); - - public LoseExecutionContextAwaitable GetAwaiter() => this; - public bool IsCompleted => false; - - public void GetResult() - { - } - - public void OnCompleted(Action continuation) - { - ThreadPool.UnsafeQueueUserWorkItem(state => ((Action)state!)(), continuation); + _loadStartedTcs.SetResult(null); + Assert.True(_mre.Wait(_timeout), "BlockLoadOnMREProvider.Load() timed out."); } } From 1766a7c05e40b4a1aa9457c9b398e459d899237c Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 18 Nov 2021 16:48:13 -0800 Subject: [PATCH 03/14] Remove ConfigurationManager._providerLock - Instead copy-on-write and replace the _providers once updated --- .../src/ConfigurationManager.cs | 94 ++++++------------- 1 file changed, 29 insertions(+), 65 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs index 6fe4c1a0046634..e809eac889f8f6 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs @@ -21,9 +21,8 @@ public sealed class ConfigurationManager : IConfigurationBuilder, IConfiguration private readonly ConfigurationSources _sources; private readonly ConfigurationBuilderProperties _properties; - private readonly object _providerLock = new(); - private readonly List _providers = new(); private readonly List _changeTokenRegistrations = new(); + private List _providers = new(); private ConfigurationReloadToken _changeToken = new(); /// @@ -41,57 +40,26 @@ public ConfigurationManager() /// public string? this[string key] { - get - { - lock (_providerLock) - { - return ConfigurationRoot.GetConfiguration(_providers, key); - } - } - set - { - lock (_providerLock) - { - ConfigurationRoot.SetConfiguration(_providers, key, value); - } - } + get => ConfigurationRoot.GetConfiguration(_providers, key); + set => ConfigurationRoot.SetConfiguration(_providers, key, value); } /// public IConfigurationSection GetSection(string key) => new ConfigurationSection(this, key); /// - public IEnumerable GetChildren() - { - lock (_providerLock) - { - // ToList() to eagerly evaluate inside lock. - return this.GetChildrenImplementation(null).ToList(); - } - } + public IEnumerable GetChildren() => this.GetChildrenImplementation(null); IDictionary IConfigurationBuilder.Properties => _properties; IList IConfigurationBuilder.Sources => _sources; - IEnumerable IConfigurationRoot.Providers - { - get - { - lock (_providerLock) - { - return new List(_providers); - } - } - } + IEnumerable IConfigurationRoot.Providers => _providers; /// public void Dispose() { - lock (_providerLock) - { - DisposeRegistrationsAndProvidersUnsynchronized(); - } + DisposeRegistrationsAndProviders(); } IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source) @@ -106,12 +74,9 @@ IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source) void IConfigurationRoot.Reload() { - lock (_providerLock) + foreach (var provider in _providers) { - foreach (var provider in _providers) - { - provider.Load(); - } + provider.Load(); } RaiseChanged(); @@ -126,44 +91,43 @@ private void RaiseChanged() // Don't rebuild and reload all providers in the common case when a source is simply added to the IList. private void AddSource(IConfigurationSource source) { - lock (_providerLock) - { - var provider = source.Build(this); - _providers.Add(provider); + var provider = source.Build(this); - provider.Load(); - _changeTokenRegistrations.Add(ChangeToken.OnChange(() => provider.GetReloadToken(), () => RaiseChanged())); - } + var newProvidersList = new List(_providers); + newProvidersList.Add(provider); + + provider.Load(); + _changeTokenRegistrations.Add(ChangeToken.OnChange(() => provider.GetReloadToken(), () => RaiseChanged())); + _providers = newProvidersList; RaiseChanged(); } // Something other than Add was called on IConfigurationBuilder.Sources or IConfigurationBuilder.Properties has changed. private void ReloadSources() { - lock (_providerLock) - { - DisposeRegistrationsAndProvidersUnsynchronized(); + DisposeRegistrationsAndProviders(); - _changeTokenRegistrations.Clear(); - _providers.Clear(); + _changeTokenRegistrations.Clear(); - foreach (var source in _sources) - { - _providers.Add(source.Build(this)); - } + var newProvidersList = new List(); - foreach (var p in _providers) - { - p.Load(); - _changeTokenRegistrations.Add(ChangeToken.OnChange(() => p.GetReloadToken(), () => RaiseChanged())); - } + foreach (var source in _sources) + { + newProvidersList.Add(source.Build(this)); + } + + foreach (var p in newProvidersList) + { + p.Load(); + _changeTokenRegistrations.Add(ChangeToken.OnChange(() => p.GetReloadToken(), () => RaiseChanged())); } + _providers = newProvidersList; RaiseChanged(); } - private void DisposeRegistrationsAndProvidersUnsynchronized() + private void DisposeRegistrationsAndProviders() { // dispose change token registrations foreach (var registration in _changeTokenRegistrations) From 25da85dbe24b7749728760c9076b889b734dfb0a Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 18 Nov 2021 18:11:39 -0800 Subject: [PATCH 04/14] Add RefCountedProviders --- .../src/ConfigurationManager.cs | 124 +++++++++++++++--- .../src/ConfigurationRoot.cs | 2 +- .../src/ConfigurationSection.cs | 2 +- .../InternalConfigurationRootExtensions.cs | 5 +- .../tests/ConfigurationManagerTest.cs | 2 +- 5 files changed, 109 insertions(+), 26 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs index e809eac889f8f6..2251d3ae628115 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs @@ -21,8 +21,9 @@ public sealed class ConfigurationManager : IConfigurationBuilder, IConfiguration private readonly ConfigurationSources _sources; private readonly ConfigurationBuilderProperties _properties; + // _providerManager provides copy-on-write references. It waits until all readers unreference before disposing any providers. + private readonly ProviderManager _providerManager = new(); private readonly List _changeTokenRegistrations = new(); - private List _providers = new(); private ConfigurationReloadToken _changeToken = new(); /// @@ -40,26 +41,42 @@ public ConfigurationManager() /// public string? this[string key] { - get => ConfigurationRoot.GetConfiguration(_providers, key); - set => ConfigurationRoot.SetConfiguration(_providers, key, value); + get + { + using var refCounter = _providerManager.GetReference(); + return ConfigurationRoot.GetConfiguration(refCounter.Providers, key); + } + set + { + using var refCounter = _providerManager.GetReference(); + ConfigurationRoot.SetConfiguration(refCounter.Providers, key, value); + } } /// public IConfigurationSection GetSection(string key) => new ConfigurationSection(this, key); /// - public IEnumerable GetChildren() => this.GetChildrenImplementation(null); + public IEnumerable GetChildren() + { + using var refCounter = _providerManager.GetReference(); + return this.GetChildrenImplementation(refCounter.Providers, path: null); + } IDictionary IConfigurationBuilder.Properties => _properties; IList IConfigurationBuilder.Sources => _sources; - IEnumerable IConfigurationRoot.Providers => _providers; + // We cannot track the duration of the reference to the providers if this property is used. + // If a configuration source is removed after this is accessed but before it's completely enumerated, + // this may allow access to a disposed provider. + IEnumerable IConfigurationRoot.Providers => _providerManager.Providers; /// public void Dispose() { - DisposeRegistrationsAndProviders(); + DisposeRegistrations(); + _providerManager.Dispose(); } IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source) @@ -74,9 +91,12 @@ IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source) void IConfigurationRoot.Reload() { - foreach (var provider in _providers) + using (var refCounter = _providerManager.GetReference()) { - provider.Load(); + foreach (var provider in refCounter.Providers) + { + provider.Load(); + } } RaiseChanged(); @@ -93,20 +113,17 @@ private void AddSource(IConfigurationSource source) { var provider = source.Build(this); - var newProvidersList = new List(_providers); - newProvidersList.Add(provider); - provider.Load(); _changeTokenRegistrations.Add(ChangeToken.OnChange(() => provider.GetReloadToken(), () => RaiseChanged())); - _providers = newProvidersList; + _providerManager.AddProvider(provider); RaiseChanged(); } // Something other than Add was called on IConfigurationBuilder.Sources or IConfigurationBuilder.Properties has changed. private void ReloadSources() { - DisposeRegistrationsAndProviders(); + DisposeRegistrations(); _changeTokenRegistrations.Clear(); @@ -123,23 +140,17 @@ private void ReloadSources() _changeTokenRegistrations.Add(ChangeToken.OnChange(() => p.GetReloadToken(), () => RaiseChanged())); } - _providers = newProvidersList; + _providerManager.ReplaceProviders(newProvidersList); RaiseChanged(); } - private void DisposeRegistrationsAndProviders() + private void DisposeRegistrations() { // dispose change token registrations foreach (var registration in _changeTokenRegistrations) { registration.Dispose(); } - - // dispose providers - foreach (var provider in _providers) - { - (provider as IDisposable)?.Dispose(); - } } private class ConfigurationSources : IList @@ -223,6 +234,77 @@ IEnumerator IEnumerable.GetEnumerator() } } + private class ProviderManager : IDisposable + { + private readonly object _replaceProvidersLock = new object(); + private RefCountedProviders _refCountedProviders = new(new List()); + + public IEnumerable Providers => _refCountedProviders.Providers; + + public RefCountedProviders GetReference() + { + // Lock to ensure oldRefCountedProviders.Dispose() in ReplaceProviders() doesn't decrement ref count to zero + // before calling _refCountedProviders.AddRef(). + lock (_replaceProvidersLock) + { + _refCountedProviders.AddRef(); + return _refCountedProviders; + } + } + + // Providers should never be concurrently modified. Reading during modification is allowed. + public void ReplaceProviders(List providers) + { + RefCountedProviders oldRefCountedProviders = _refCountedProviders; + + lock (_replaceProvidersLock) + { + _refCountedProviders = new RefCountedProviders(providers); + } + + oldRefCountedProviders.Dispose(); + } + + public void AddProvider(IConfigurationProvider provider) + { + // Maintain existing references, but replace list with copy containing new item. + _refCountedProviders.Providers = new List(_refCountedProviders.Providers) + { + provider + }; + } + + public void Dispose() => _refCountedProviders.Dispose(); + } + + private class RefCountedProviders : IDisposable + { + private long _refCount = 1; + + public RefCountedProviders(List providers) + { + Providers = providers; + } + + public List Providers { get; set; } + + public void AddRef() + { + Interlocked.Increment(ref _refCount); + } + + public void Dispose() + { + if (Interlocked.Decrement(ref _refCount) == 0) + { + foreach (var provider in Providers) + { + (provider as IDisposable)?.Dispose(); + } + } + } + } + private class ConfigurationBuilderProperties : IDictionary { private readonly Dictionary _properties = new(); diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs index 397ef5160b1ec7..1cf167237b3816 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs @@ -57,7 +57,7 @@ public string? this[string key] /// Gets the immediate children sub-sections. /// /// The children. - public IEnumerable GetChildren() => this.GetChildrenImplementation(null); + public IEnumerable GetChildren() => this.GetChildrenImplementation(providersCopy: null, path: null); /// /// Returns a that can be used to observe when this configuration is reloaded. diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs index 0896e587d4082d..118c68ec8f0c54 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs @@ -105,7 +105,7 @@ public string? this[string key] /// Gets the immediate descendant configuration sub-sections. /// /// The configuration sub-sections. - public IEnumerable GetChildren() => _root.GetChildrenImplementation(Path); + public IEnumerable GetChildren() => _root.GetChildrenImplementation(providersCopy: null, Path); /// /// Returns a that can be used to observe when this configuration is reloaded. diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs b/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs index 7fd17a98acf475..8e26328a5b6f65 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs @@ -16,11 +16,12 @@ internal static class InternalConfigurationRootExtensions /// Gets the immediate children sub-sections of configuration root based on key. /// /// Configuration from which to retrieve sub-sections. + /// The providers to retrieve sub-sections from if not directly from . /// Key of a section of which children to retrieve. /// Immediate children sub-sections of section specified by key. - internal static IEnumerable GetChildrenImplementation(this IConfigurationRoot root, string? path) + internal static IEnumerable GetChildrenImplementation(this IConfigurationRoot root, IEnumerable? providersCopy, string? path) { - return root.Providers + return (providersCopy ?? root.Providers) .Aggregate(Enumerable.Empty(), (seed, source) => source.GetChildKeys(seed, path)) .Distinct(StringComparer.OrdinalIgnoreCase) diff --git a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs index fe79496e4e9421..9fdaf8af58f518 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs @@ -183,7 +183,7 @@ public async Task ProviderCanBlockLoadWaitingOnConcurrentRead() var config = new ConfigurationManager(); IConfigurationBuilder builder = config; - // Add calls provider.Load(). + // builder.Add(source) calls provider.Load(). var loadTask = Task.Run(() => builder.Add(source)); await provider.LoadStartedTask; From d26846a15688f40e97e6978b7bfc6b7b9f3642d9 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 30 Nov 2021 10:51:38 -0800 Subject: [PATCH 05/14] Add ProviderDisposeDelayedWaitingOnConcurrentRead test --- .../tests/ConfigurationManagerTest.cs | 55 ++++++++++++++++++- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs index 9fdaf8af58f518..ec28b0b0b44ca6 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs @@ -178,13 +178,12 @@ public async Task ProviderCanBlockLoadWaitingOnConcurrentRead() { using var mre = new ManualResetEventSlim(false); var provider = new BlockLoadOnMREProvider(mre, timeout: TimeSpan.FromSeconds(30)); - var source = new TestConfigurationSource(provider); var config = new ConfigurationManager(); IConfigurationBuilder builder = config; // builder.Add(source) calls provider.Load(). - var loadTask = Task.Run(() => builder.Add(source)); + var loadTask = Task.Run(() => builder.Add(new TestConfigurationSource(provider))); await provider.LoadStartedTask; @@ -198,6 +197,35 @@ public async Task ProviderCanBlockLoadWaitingOnConcurrentRead() await loadTask; } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] + public async Task ProviderDisposeDelayedWaitingOnConcurrentRead() + { + using var mre = new ManualResetEventSlim(false); + var provider = new BlockTryGetOnMREProvider(mre, timeout: TimeSpan.FromSeconds(30)); + + var config = new ConfigurationManager(); + IConfigurationBuilder builder = config; + + builder.Add(new TestConfigurationSource(provider)); + + // Reading configuration will block on provider.TryRead(). + var readTask = Task.Run(() => config["key"]); + + // Removing the source normally disposes the provider except when there provider is in use as is the case here. + builder.Sources.Clear(); + + Assert.False(provider.IsDisposed); + + // Unblock provider.TryRead() + mre.Set(); + + // This will throw if provider.TryRead() timed out instead of unblocking gracefully after setting the MRE. + await readTask; + + // The provider should be disposed when provider.TryRead() releases the last reference to the provider. + Assert.True(provider.IsDisposed); + } + // Moq heavily utilizes RefEmit, which does not work on most aot workloads [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] public void DisposesChangeTokenRegistrationsOnDispose() @@ -1178,6 +1206,29 @@ public override void Load() } } + private class BlockTryGetOnMREProvider : ConfigurationProvider, IDisposable + { + private readonly ManualResetEventSlim _mre; + private readonly TimeSpan _timeout; + + public BlockTryGetOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) + { + _mre = mre; + _timeout = timeout; + } + + public bool IsDisposed { get; set; } + + public override bool TryGet(string key, out string? value) + { + Assert.True(_mre.Wait(_timeout), "BlockTryGetOnMREProvider.TryGet() timed out."); + return base.TryGet(key, out value); + } + + public void Dispose() + => IsDisposed = true; + } + private class DisposableTestConfigurationProvider : ConfigurationProvider, IDisposable { public bool IsDisposed { get; set; } From 55b07b7d983dce5819957b15138712374dc083b7 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 30 Nov 2021 14:26:23 -0800 Subject: [PATCH 06/14] fix test --- .../tests/ConfigurationManagerTest.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs index ec28b0b0b44ca6..fd991f1a79c1f6 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs @@ -184,7 +184,6 @@ public async Task ProviderCanBlockLoadWaitingOnConcurrentRead() // builder.Add(source) calls provider.Load(). var loadTask = Task.Run(() => builder.Add(new TestConfigurationSource(provider))); - await provider.LoadStartedTask; // Read configuration while provider.Load() is blocked waiting on us. @@ -210,6 +209,7 @@ public async Task ProviderDisposeDelayedWaitingOnConcurrentRead() // Reading configuration will block on provider.TryRead(). var readTask = Task.Run(() => config["key"]); + await provider.ReadStartedTask; // Removing the source normally disposes the provider except when there provider is in use as is the case here. builder.Sources.Clear(); @@ -1188,9 +1188,10 @@ public TestConfigurationProvider(string key, string value) private class BlockLoadOnMREProvider : ConfigurationProvider { private readonly ManualResetEventSlim _mre; - private readonly TaskCompletionSource _loadStartedTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); private readonly TimeSpan _timeout; + private readonly TaskCompletionSource _loadStartedTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); + public BlockLoadOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) { _mre = mre; @@ -1211,16 +1212,21 @@ private class BlockTryGetOnMREProvider : ConfigurationProvider, IDisposable private readonly ManualResetEventSlim _mre; private readonly TimeSpan _timeout; + private readonly TaskCompletionSource _readStartedTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); + public BlockTryGetOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) { _mre = mre; _timeout = timeout; } + public Task ReadStartedTask => _readStartedTcs.Task; + public bool IsDisposed { get; set; } public override bool TryGet(string key, out string? value) { + _readStartedTcs.SetResult(null); Assert.True(_mre.Wait(_timeout), "BlockTryGetOnMREProvider.TryGet() timed out."); return base.TryGet(key, out value); } From f99b2af6f77e6a9ca94cf71752875c99664c6c2e Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 30 Nov 2021 14:27:23 -0800 Subject: [PATCH 07/14] private sealed class --- .../src/ConfigurationManager.cs | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs index 2251d3ae628115..9d44962c14bfbc 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs @@ -43,13 +43,13 @@ public string? this[string key] { get { - using var refCounter = _providerManager.GetReference(); - return ConfigurationRoot.GetConfiguration(refCounter.Providers, key); + using var reference = _providerManager.GetReference(); + return ConfigurationRoot.GetConfiguration(reference.Providers, key); } set { - using var refCounter = _providerManager.GetReference(); - ConfigurationRoot.SetConfiguration(refCounter.Providers, key, value); + using var reference = _providerManager.GetReference(); + ConfigurationRoot.SetConfiguration(reference.Providers, key, value); } } @@ -59,8 +59,8 @@ public string? this[string key] /// public IEnumerable GetChildren() { - using var refCounter = _providerManager.GetReference(); - return this.GetChildrenImplementation(refCounter.Providers, path: null); + using var reference = _providerManager.GetReference(); + return this.GetChildrenImplementation(reference.Providers, path: null); } IDictionary IConfigurationBuilder.Properties => _properties; @@ -91,9 +91,9 @@ IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source) void IConfigurationRoot.Reload() { - using (var refCounter = _providerManager.GetReference()) + using (var reference = _providerManager.GetReference()) { - foreach (var provider in refCounter.Providers) + foreach (var provider in reference.Providers) { provider.Load(); } @@ -153,7 +153,7 @@ private void DisposeRegistrations() } } - private class ConfigurationSources : IList + private sealed class ConfigurationSources : IList { private readonly List _sources = new(); private readonly ConfigurationManager _config; @@ -234,14 +234,14 @@ IEnumerator IEnumerable.GetEnumerator() } } - private class ProviderManager : IDisposable + private sealed class ProviderManager : IDisposable { private readonly object _replaceProvidersLock = new object(); - private RefCountedProviders _refCountedProviders = new(new List()); + private ProvidersReference _refCountedProviders = new(new List()); public IEnumerable Providers => _refCountedProviders.Providers; - public RefCountedProviders GetReference() + public ProvidersReference GetReference() { // Lock to ensure oldRefCountedProviders.Dispose() in ReplaceProviders() doesn't decrement ref count to zero // before calling _refCountedProviders.AddRef(). @@ -255,11 +255,11 @@ public RefCountedProviders GetReference() // Providers should never be concurrently modified. Reading during modification is allowed. public void ReplaceProviders(List providers) { - RefCountedProviders oldRefCountedProviders = _refCountedProviders; + ProvidersReference oldRefCountedProviders = _refCountedProviders; lock (_replaceProvidersLock) { - _refCountedProviders = new RefCountedProviders(providers); + _refCountedProviders = new ProvidersReference(providers); } oldRefCountedProviders.Dispose(); @@ -277,11 +277,11 @@ public void AddProvider(IConfigurationProvider provider) public void Dispose() => _refCountedProviders.Dispose(); } - private class RefCountedProviders : IDisposable + private sealed class ProvidersReference : IDisposable { private long _refCount = 1; - public RefCountedProviders(List providers) + public ProvidersReference(List providers) { Providers = providers; } @@ -305,7 +305,7 @@ public void Dispose() } } - private class ConfigurationBuilderProperties : IDictionary + private sealed class ConfigurationBuilderProperties : IDictionary { private readonly Dictionary _properties = new(); private readonly ConfigurationManager _config; From 228e978a5609be64052493c398eba73ceb068fd5 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 6 Dec 2021 15:06:01 -0800 Subject: [PATCH 08/14] Address PR feedback --- .../src/ConfigurationManager.cs | 65 +++++++++++++------ 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs index 9d44962c14bfbc..ea2bbfd120b629 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs @@ -4,6 +4,7 @@ using System; using System.Collections; using System.Collections.Generic; +using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Threading; @@ -14,15 +15,20 @@ namespace Microsoft.Extensions.Configuration { /// /// ConfigurationManager is a mutable configuration object. It is both an and an . - /// As sources are added, it updates its current view of configuration. Once Build is called, configuration is frozen. + /// As sources are added, it updates its current view of configuration. /// public sealed class ConfigurationManager : IConfigurationBuilder, IConfigurationRoot, IDisposable { + // Concurrently modifying config sources is not thread-safe. However, it is thread-safe to read config while modifying sources. private readonly ConfigurationSources _sources; private readonly ConfigurationBuilderProperties _properties; - // _providerManager provides copy-on-write references. It waits until all readers unreference before disposing any providers. + // ProviderManager provides copy-on-write references to support concurrently reading config while modifying sources. + // It waits until all readers unreference it before disposing any providers. private readonly ProviderManager _providerManager = new(); + + // _changeTokenRegistrations are only modified when config sources are modified and are not referenced during read operations. + // Because modify config sources is not thread-safe, modifying _changeTokenRegistrations does not need to be thread-safe either. private readonly List _changeTokenRegistrations = new(); private ConfigurationReloadToken _changeToken = new(); @@ -43,12 +49,12 @@ public string? this[string key] { get { - using var reference = _providerManager.GetReference(); + using ReferenceCountedProviders reference = _providerManager.GetReference(); return ConfigurationRoot.GetConfiguration(reference.Providers, key); } set { - using var reference = _providerManager.GetReference(); + using ReferenceCountedProviders reference = _providerManager.GetReference(); ConfigurationRoot.SetConfiguration(reference.Providers, key, value); } } @@ -59,7 +65,7 @@ public string? this[string key] /// public IEnumerable GetChildren() { - using var reference = _providerManager.GetReference(); + using ReferenceCountedProviders reference = _providerManager.GetReference(); return this.GetChildrenImplementation(reference.Providers, path: null); } @@ -70,7 +76,7 @@ public IEnumerable GetChildren() // We cannot track the duration of the reference to the providers if this property is used. // If a configuration source is removed after this is accessed but before it's completely enumerated, // this may allow access to a disposed provider. - IEnumerable IConfigurationRoot.Providers => _providerManager.Providers; + IEnumerable IConfigurationRoot.Providers => _providerManager.NonRefCountedProviders; /// public void Dispose() @@ -140,7 +146,7 @@ private void ReloadSources() _changeTokenRegistrations.Add(ChangeToken.OnChange(() => p.GetReloadToken(), () => RaiseChanged())); } - _providerManager.ReplaceProviders(newProvidersList); + _providerManager.ReplaceReferenceCountedProviders(newProvidersList); RaiseChanged(); } @@ -237,29 +243,30 @@ IEnumerator IEnumerable.GetEnumerator() private sealed class ProviderManager : IDisposable { private readonly object _replaceProvidersLock = new object(); - private ProvidersReference _refCountedProviders = new(new List()); + private ReferenceCountedProviders _refCountedProviders = new(new List()); - public IEnumerable Providers => _refCountedProviders.Providers; + // This is only used to support IConfigurationRoot.Providers because we cannot track the lifetime of that reference. + public IEnumerable NonRefCountedProviders => _refCountedProviders.NonRefCountedProviders; - public ProvidersReference GetReference() + public ReferenceCountedProviders GetReference() { // Lock to ensure oldRefCountedProviders.Dispose() in ReplaceProviders() doesn't decrement ref count to zero // before calling _refCountedProviders.AddRef(). lock (_replaceProvidersLock) { - _refCountedProviders.AddRef(); + _refCountedProviders.AddReference(); return _refCountedProviders; } } // Providers should never be concurrently modified. Reading during modification is allowed. - public void ReplaceProviders(List providers) + public void ReplaceReferenceCountedProviders(List providers) { - ProvidersReference oldRefCountedProviders = _refCountedProviders; + ReferenceCountedProviders oldRefCountedProviders = _refCountedProviders; lock (_replaceProvidersLock) { - _refCountedProviders = new ProvidersReference(providers); + _refCountedProviders = new ReferenceCountedProviders(providers); } oldRefCountedProviders.Dispose(); @@ -268,31 +275,47 @@ public void ReplaceProviders(List providers) public void AddProvider(IConfigurationProvider provider) { // Maintain existing references, but replace list with copy containing new item. - _refCountedProviders.Providers = new List(_refCountedProviders.Providers) + _refCountedProviders.ReplaceProvidersList(new List(_refCountedProviders.Providers) { provider - }; + }); } public void Dispose() => _refCountedProviders.Dispose(); } - private sealed class ProvidersReference : IDisposable + private sealed class ReferenceCountedProviders : IDisposable { private long _refCount = 1; + private volatile List _providers; - public ProvidersReference(List providers) + public ReferenceCountedProviders(List providers) { - Providers = providers; + _providers = providers; } - public List Providers { get; set; } + public List Providers + { + get + { + Debug.Assert(_refCount > 0); + return _providers; + } + } - public void AddRef() + public List NonRefCountedProviders => _providers; + + public void AddReference() { + // This is always called with a lock to ensure _refCount hasn't already decremented to zero. Interlocked.Increment(ref _refCount); } + public void ReplaceProvidersList(List providers) + { + _providers = providers; + } + public void Dispose() { if (Interlocked.Decrement(ref _refCount) == 0) From b361192d3ffc85c88c4965c86e000b3904319e6d Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 6 Dec 2021 15:27:48 -0800 Subject: [PATCH 09/14] Always hold providers reference in GetChildren() --- .../src/ConfigurationManager.cs | 111 ++---------------- .../src/ConfigurationRoot.cs | 2 +- .../src/ConfigurationSection.cs | 2 +- .../InternalConfigurationRootExtensions.cs | 12 +- .../src/ReferenceCountedProviders.cs | 59 ++++++++++ .../src/ReferenceCountedProvidersManager.cs | 55 +++++++++ .../tests/ConfigurationManagerTest.cs | 48 +++++--- 7 files changed, 168 insertions(+), 121 deletions(-) create mode 100644 src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs create mode 100644 src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs index ea2bbfd120b629..830d9913ced834 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs @@ -4,7 +4,6 @@ using System; using System.Collections; using System.Collections.Generic; -using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Threading; @@ -19,15 +18,15 @@ namespace Microsoft.Extensions.Configuration /// public sealed class ConfigurationManager : IConfigurationBuilder, IConfigurationRoot, IDisposable { - // Concurrently modifying config sources is not thread-safe. However, it is thread-safe to read config while modifying sources. + // Concurrently modifying config sources or properties is not thread-safe. However, it is thread-safe to read config while modifying sources or properties. private readonly ConfigurationSources _sources; private readonly ConfigurationBuilderProperties _properties; - // ProviderManager provides copy-on-write references to support concurrently reading config while modifying sources. - // It waits until all readers unreference it before disposing any providers. - private readonly ProviderManager _providerManager = new(); + // ReferenceCountedProviderManager manages copy-on-write references to support concurrently reading config while modifying sources. + // It waits for readers to unreference the providers before disposing them without blocking on any concurrent operations. + private readonly ReferenceCountedProviderManager _providerManager = new(); - // _changeTokenRegistrations are only modified when config sources are modified and are not referenced during read operations. + // _changeTokenRegistrations is only modified when config sources are modified. It is not referenced by any read operations. // Because modify config sources is not thread-safe, modifying _changeTokenRegistrations does not need to be thread-safe either. private readonly List _changeTokenRegistrations = new(); private ConfigurationReloadToken _changeToken = new(); @@ -63,11 +62,7 @@ public string? this[string key] public IConfigurationSection GetSection(string key) => new ConfigurationSection(this, key); /// - public IEnumerable GetChildren() - { - using ReferenceCountedProviders reference = _providerManager.GetReference(); - return this.GetChildrenImplementation(reference.Providers, path: null); - } + public IEnumerable GetChildren() => this.GetChildrenImplementation(null); IDictionary IConfigurationBuilder.Properties => _properties; @@ -76,7 +71,7 @@ public IEnumerable GetChildren() // We cannot track the duration of the reference to the providers if this property is used. // If a configuration source is removed after this is accessed but before it's completely enumerated, // this may allow access to a disposed provider. - IEnumerable IConfigurationRoot.Providers => _providerManager.NonRefCountedProviders; + IEnumerable IConfigurationRoot.Providers => _providerManager.NonReferenceCountedProviders; /// public void Dispose() @@ -108,6 +103,8 @@ void IConfigurationRoot.Reload() RaiseChanged(); } + internal ReferenceCountedProviders GetProvidersReference() => _providerManager.GetReference(); + private void RaiseChanged() { var previousToken = Interlocked.Exchange(ref _changeToken, new ConfigurationReloadToken()); @@ -146,7 +143,7 @@ private void ReloadSources() _changeTokenRegistrations.Add(ChangeToken.OnChange(() => p.GetReloadToken(), () => RaiseChanged())); } - _providerManager.ReplaceReferenceCountedProviders(newProvidersList); + _providerManager.ReplaceProviders(newProvidersList); RaiseChanged(); } @@ -240,94 +237,6 @@ IEnumerator IEnumerable.GetEnumerator() } } - private sealed class ProviderManager : IDisposable - { - private readonly object _replaceProvidersLock = new object(); - private ReferenceCountedProviders _refCountedProviders = new(new List()); - - // This is only used to support IConfigurationRoot.Providers because we cannot track the lifetime of that reference. - public IEnumerable NonRefCountedProviders => _refCountedProviders.NonRefCountedProviders; - - public ReferenceCountedProviders GetReference() - { - // Lock to ensure oldRefCountedProviders.Dispose() in ReplaceProviders() doesn't decrement ref count to zero - // before calling _refCountedProviders.AddRef(). - lock (_replaceProvidersLock) - { - _refCountedProviders.AddReference(); - return _refCountedProviders; - } - } - - // Providers should never be concurrently modified. Reading during modification is allowed. - public void ReplaceReferenceCountedProviders(List providers) - { - ReferenceCountedProviders oldRefCountedProviders = _refCountedProviders; - - lock (_replaceProvidersLock) - { - _refCountedProviders = new ReferenceCountedProviders(providers); - } - - oldRefCountedProviders.Dispose(); - } - - public void AddProvider(IConfigurationProvider provider) - { - // Maintain existing references, but replace list with copy containing new item. - _refCountedProviders.ReplaceProvidersList(new List(_refCountedProviders.Providers) - { - provider - }); - } - - public void Dispose() => _refCountedProviders.Dispose(); - } - - private sealed class ReferenceCountedProviders : IDisposable - { - private long _refCount = 1; - private volatile List _providers; - - public ReferenceCountedProviders(List providers) - { - _providers = providers; - } - - public List Providers - { - get - { - Debug.Assert(_refCount > 0); - return _providers; - } - } - - public List NonRefCountedProviders => _providers; - - public void AddReference() - { - // This is always called with a lock to ensure _refCount hasn't already decremented to zero. - Interlocked.Increment(ref _refCount); - } - - public void ReplaceProvidersList(List providers) - { - _providers = providers; - } - - public void Dispose() - { - if (Interlocked.Decrement(ref _refCount) == 0) - { - foreach (var provider in Providers) - { - (provider as IDisposable)?.Dispose(); - } - } - } - } - private sealed class ConfigurationBuilderProperties : IDictionary { private readonly Dictionary _properties = new(); diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs index 1cf167237b3816..397ef5160b1ec7 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationRoot.cs @@ -57,7 +57,7 @@ public string? this[string key] /// Gets the immediate children sub-sections. /// /// The children. - public IEnumerable GetChildren() => this.GetChildrenImplementation(providersCopy: null, path: null); + public IEnumerable GetChildren() => this.GetChildrenImplementation(null); /// /// Returns a that can be used to observe when this configuration is reloaded. diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs index 118c68ec8f0c54..0896e587d4082d 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationSection.cs @@ -105,7 +105,7 @@ public string? this[string key] /// Gets the immediate descendant configuration sub-sections. /// /// The configuration sub-sections. - public IEnumerable GetChildren() => _root.GetChildrenImplementation(providersCopy: null, Path); + public IEnumerable GetChildren() => _root.GetChildrenImplementation(Path); /// /// Returns a that can be used to observe when this configuration is reloaded. diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs b/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs index 8e26328a5b6f65..1a475cce67c168 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs @@ -16,16 +16,20 @@ internal static class InternalConfigurationRootExtensions /// Gets the immediate children sub-sections of configuration root based on key. /// /// Configuration from which to retrieve sub-sections. - /// The providers to retrieve sub-sections from if not directly from . /// Key of a section of which children to retrieve. /// Immediate children sub-sections of section specified by key. - internal static IEnumerable GetChildrenImplementation(this IConfigurationRoot root, IEnumerable? providersCopy, string? path) + internal static IEnumerable GetChildrenImplementation(this IConfigurationRoot root, string? path) { - return (providersCopy ?? root.Providers) + using ReferenceCountedProviders? reference = (root as ConfigurationManager)?.GetProvidersReference(); + var providers = reference?.Providers ?? root.Providers; + + // Eagerly evaluate the IEnumerable before releasing the reference so we don't allow iteration over disposed providers. + return providers .Aggregate(Enumerable.Empty(), (seed, source) => source.GetChildKeys(seed, path)) .Distinct(StringComparer.OrdinalIgnoreCase) - .Select(key => root.GetSection(path == null ? key : ConfigurationPath.Combine(path, key))); + .Select(key => root.GetSection(path == null ? key : ConfigurationPath.Combine(path, key))) + .ToList(); } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs new file mode 100644 index 00000000000000..b45a5f6e4b2dd9 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs @@ -0,0 +1,59 @@ +// 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 System.Diagnostics; +using System.Threading; + +namespace Microsoft.Extensions.Configuration +{ + // ReferenceCountedProviders is used by ConfigurationManager to wait until all readers unreference it before disposing any providers. + internal sealed class ReferenceCountedProviders : IDisposable + { + private long _refCount = 1; + // volatile is not strictly necessary because the runtime adds a barrier either way, but volatile indicates that this field has + // unsynchronized readers meaning the all writes initializing the list must be published before updating the _providers reference. + private volatile List _providers; + + public ReferenceCountedProviders(List providers) + { + _providers = providers; + } + + public List Providers + { + get + { + Debug.Assert(_refCount > 0); + return _providers; + } + set + { + Debug.Assert(_refCount > 0); + _providers = value; + } + } + + // This is only used to support IConfigurationRoot.Providers because we cannot track the lifetime of that reference. + public List NonReferenceCountedProviders => _providers; + + public void AddReference() + { + // AddReference() is always called with a lock to ensure _refCount hasn't already decremented to zero. + Debug.Assert(_refCount > 0); + Interlocked.Increment(ref _refCount); + } + + public void Dispose() + { + if (Interlocked.Decrement(ref _refCount) == 0) + { + foreach (var provider in _providers) + { + (provider as IDisposable)?.Dispose(); + } + } + } + } +} diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs new file mode 100644 index 00000000000000..0eeaa875925f79 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs @@ -0,0 +1,55 @@ +// 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; + +namespace Microsoft.Extensions.Configuration +{ + // ReferenceCountedProviderManager is used by ConfigurationManager to provide copy-on-write references that support concurrently + // reading config while modifying sources. It waits for readers to unreference the providers before disposing them + // without blocking on any concurrent operations. + internal sealed class ReferenceCountedProviderManager : IDisposable + { + private readonly object _replaceProvidersLock = new object(); + private ReferenceCountedProviders _refCountedProviders = new(new List()); + + // This is only used to support IConfigurationRoot.Providers because we cannot track the lifetime of that reference. + public IEnumerable NonReferenceCountedProviders => _refCountedProviders.NonReferenceCountedProviders; + + public ReferenceCountedProviders GetReference() + { + // Lock to ensure oldRefCountedProviders.Dispose() in ReplaceProviders() doesn't decrement ref count to zero + // before calling _refCountedProviders.AddRef(). + lock (_replaceProvidersLock) + { + _refCountedProviders.AddReference(); + return _refCountedProviders; + } + } + + // Providers should never be concurrently modified. Reading during modification is allowed. + public void ReplaceProviders(List providers) + { + ReferenceCountedProviders oldRefCountedProviders = _refCountedProviders; + + lock (_replaceProvidersLock) + { + _refCountedProviders = new ReferenceCountedProviders(providers); + } + + oldRefCountedProviders.Dispose(); + } + + public void AddProvider(IConfigurationProvider provider) + { + // Maintain existing references, but replace list with copy containing new item. + _refCountedProviders.Providers = new List(_refCountedProviders.Providers) + { + provider + }; + } + + public void Dispose() => _refCountedProviders.Dispose(); + } +} diff --git a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs index fd991f1a79c1f6..414962d5c10d87 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs @@ -182,7 +182,7 @@ public async Task ProviderCanBlockLoadWaitingOnConcurrentRead() var config = new ConfigurationManager(); IConfigurationBuilder builder = config; - // builder.Add(source) calls provider.Load(). + // builder.Add(source) will block on provider.Load(). var loadTask = Task.Run(() => builder.Add(new TestConfigurationSource(provider))); await provider.LoadStartedTask; @@ -196,19 +196,33 @@ public async Task ProviderCanBlockLoadWaitingOnConcurrentRead() await loadTask; } - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] - public async Task ProviderDisposeDelayedWaitingOnConcurrentRead() + public static TheoryData ConcurrentReadActions + { + get + { + return new TheoryData> + { + config => _ = config["key"], + config => config.GetChildren(), + config => config.GetSection("key").GetChildren(), + }; + } + } + + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] + [MemberData(nameof(ConcurrentReadActions))] + public async Task ProviderDisposeDelayedWaitingOnConcurrentRead(Action concurrentReadAction) { using var mre = new ManualResetEventSlim(false); - var provider = new BlockTryGetOnMREProvider(mre, timeout: TimeSpan.FromSeconds(30)); + var provider = new BlockReadOnMREProvider(mre, timeout: TimeSpan.FromSeconds(30)); var config = new ConfigurationManager(); IConfigurationBuilder builder = config; builder.Add(new TestConfigurationSource(provider)); - // Reading configuration will block on provider.TryRead(). - var readTask = Task.Run(() => config["key"]); + // Reading configuration will block on provider.TryRead() or profvider.GetChildKeys(). + var readTask = Task.Run(() => concurrentReadAction(config)); await provider.ReadStartedTask; // Removing the source normally disposes the provider except when there provider is in use as is the case here. @@ -216,13 +230,13 @@ public async Task ProviderDisposeDelayedWaitingOnConcurrentRead() Assert.False(provider.IsDisposed); - // Unblock provider.TryRead() + // Unblock TryRead() or GetChildKeys() mre.Set(); - // This will throw if provider.TryRead() timed out instead of unblocking gracefully after setting the MRE. + // This will throw if TryRead() or GetChildKeys() timed out instead of unblocking gracefully after setting the MRE. await readTask; - // The provider should be disposed when provider.TryRead() releases the last reference to the provider. + // The provider should be disposed when the concurrentReadAction releases the last reference to the provider. Assert.True(provider.IsDisposed); } @@ -1207,14 +1221,14 @@ public override void Load() } } - private class BlockTryGetOnMREProvider : ConfigurationProvider, IDisposable + private class BlockReadOnMREProvider : ConfigurationProvider, IDisposable { private readonly ManualResetEventSlim _mre; private readonly TimeSpan _timeout; private readonly TaskCompletionSource _readStartedTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); - public BlockTryGetOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) + public BlockReadOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) { _mre = mre; _timeout = timeout; @@ -1227,12 +1241,18 @@ public BlockTryGetOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) public override bool TryGet(string key, out string? value) { _readStartedTcs.SetResult(null); - Assert.True(_mre.Wait(_timeout), "BlockTryGetOnMREProvider.TryGet() timed out."); + Assert.True(_mre.Wait(_timeout), "BlockReadOnMREProvider.TryGet() timed out."); return base.TryGet(key, out value); } - public void Dispose() - => IsDisposed = true; + public override IEnumerable GetChildKeys(IEnumerable earlierKeys, string? parentPath) + { + _readStartedTcs.SetResult(null); + Assert.True(_mre.Wait(_timeout), "BlockReadOnMREProvider.GetChildKeys() timed out."); + return base.GetChildKeys(earlierKeys, parentPath); + } + + public void Dispose() => IsDisposed = true; } private class DisposableTestConfigurationProvider : ConfigurationProvider, IDisposable From 69ec64d236b6bdb59db47b3329528e8b4117d320 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 9 Dec 2021 16:53:47 -0800 Subject: [PATCH 10/14] Update src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs Co-authored-by: Eric Erhardt --- .../src/ReferenceCountedProvidersManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs index 0eeaa875925f79..45dfc4a32e9583 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs @@ -20,7 +20,7 @@ internal sealed class ReferenceCountedProviderManager : IDisposable public ReferenceCountedProviders GetReference() { // Lock to ensure oldRefCountedProviders.Dispose() in ReplaceProviders() doesn't decrement ref count to zero - // before calling _refCountedProviders.AddRef(). + // before calling _refCountedProviders.AddReference(). lock (_replaceProvidersLock) { _refCountedProviders.AddReference(); From c785706aacb4a6ed3fcc219222518bd0e946cb4e Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 10 Dec 2021 16:18:49 -0800 Subject: [PATCH 11/14] Address PR feedback --- .../src/ConfigurationManager.cs | 12 ++++++------ .../src/InternalConfigurationRootExtensions.cs | 18 +++++++++++++----- .../src/ReferenceCountedProviders.cs | 15 ++++++++++++++- .../src/ReferenceCountedProvidersManager.cs | 8 +++++++- 4 files changed, 40 insertions(+), 13 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs index 830d9913ced834..1fb575e0c0c0d7 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs @@ -92,9 +92,9 @@ IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source) void IConfigurationRoot.Reload() { - using (var reference = _providerManager.GetReference()) + using (ReferenceCountedProviders reference = _providerManager.GetReference()) { - foreach (var provider in reference.Providers) + foreach (IConfigurationProvider provider in reference.Providers) { provider.Load(); } @@ -114,7 +114,7 @@ private void RaiseChanged() // Don't rebuild and reload all providers in the common case when a source is simply added to the IList. private void AddSource(IConfigurationSource source) { - var provider = source.Build(this); + IConfigurationProvider provider = source.Build(this); provider.Load(); _changeTokenRegistrations.Add(ChangeToken.OnChange(() => provider.GetReloadToken(), () => RaiseChanged())); @@ -132,12 +132,12 @@ private void ReloadSources() var newProvidersList = new List(); - foreach (var source in _sources) + foreach (IConfigurationSource source in _sources) { newProvidersList.Add(source.Build(this)); } - foreach (var p in newProvidersList) + foreach (IConfigurationProvider p in newProvidersList) { p.Load(); _changeTokenRegistrations.Add(ChangeToken.OnChange(() => p.GetReloadToken(), () => RaiseChanged())); @@ -150,7 +150,7 @@ private void ReloadSources() private void DisposeRegistrations() { // dispose change token registrations - foreach (var registration in _changeTokenRegistrations) + foreach (IDisposable registration in _changeTokenRegistrations) { registration.Dispose(); } diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs b/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs index 1a475cce67c168..8951697d8d68af 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs @@ -21,15 +21,23 @@ internal static class InternalConfigurationRootExtensions internal static IEnumerable GetChildrenImplementation(this IConfigurationRoot root, string? path) { using ReferenceCountedProviders? reference = (root as ConfigurationManager)?.GetProvidersReference(); - var providers = reference?.Providers ?? root.Providers; + IEnumerable providers = reference?.Providers ?? root.Providers; - // Eagerly evaluate the IEnumerable before releasing the reference so we don't allow iteration over disposed providers. - return providers + IEnumerable children = providers .Aggregate(Enumerable.Empty(), (seed, source) => source.GetChildKeys(seed, path)) .Distinct(StringComparer.OrdinalIgnoreCase) - .Select(key => root.GetSection(path == null ? key : ConfigurationPath.Combine(path, key))) - .ToList(); + .Select(key => root.GetSection(path == null ? key : ConfigurationPath.Combine(path, key))); + + if (reference is null) + { + return children; + } + else + { + // Eagerly evaluate the IEnumerable before releasing the reference so we don't allow iteration over disposed providers. + return children.ToList(); + } } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs index b45a5f6e4b2dd9..e7b52e72604a7b 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs @@ -45,15 +45,28 @@ public void AddReference() Interlocked.Increment(ref _refCount); } + // This is not a "real" Dispose(). It exists to conveniently release a reference at the end of a using block. public void Dispose() { if (Interlocked.Decrement(ref _refCount) == 0) { - foreach (var provider in _providers) + foreach (IConfigurationProvider provider in _providers) { (provider as IDisposable)?.Dispose(); } } } + + // This is the "real" Dispose() that is only called as part of ConfigurationManager.Dispose(). + // If there are any active references, that indicates a use-after-dispose bug in the code using the ConfigurationManager. + // + // If there are active references after dispose, the providers could get disposed more than once. We could prevent this + // by preemptively throwing an ODE from ReferenceCountedProviderManager.GetReference() after it's disposed, but this might + // break existing apps that are today able to continue to read configuration after disposing an ConfigurationManager. + public void ReleaseFinalReference() + { + Debug.Assert(_refCount == 1); + Dispose(); + } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs index 45dfc4a32e9583..1552ba41a7d776 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs @@ -38,6 +38,8 @@ public void ReplaceProviders(List providers) _refCountedProviders = new ReferenceCountedProviders(providers); } + // Decrement the reference count to the old providers. If they are being concurrently read from + // the actual disposal of the old providers will be delayed until the final reference is released. oldRefCountedProviders.Dispose(); } @@ -50,6 +52,10 @@ public void AddProvider(IConfigurationProvider provider) }; } - public void Dispose() => _refCountedProviders.Dispose(); + public void Dispose() + { + // Equivalent to _refCountedProvider.Dispose() but asserts there a no concurrent references. + _refCountedProviders.ReleaseFinalReference(); + } } } From 30b1c3602c38b427235e071d2b42133c69bfdc9e Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 10 Dec 2021 17:19:38 -0800 Subject: [PATCH 12/14] Never dispose providers more than once --- .../src/ReferenceCountedProviders.cs | 98 +++++++++++-------- .../src/ReferenceCountedProvidersManager.cs | 41 ++++++-- 2 files changed, 91 insertions(+), 48 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs index e7b52e72604a7b..4a3755c5ac1f96 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs @@ -9,64 +9,82 @@ namespace Microsoft.Extensions.Configuration { // ReferenceCountedProviders is used by ConfigurationManager to wait until all readers unreference it before disposing any providers. - internal sealed class ReferenceCountedProviders : IDisposable + internal abstract class ReferenceCountedProviders : IDisposable { - private long _refCount = 1; - // volatile is not strictly necessary because the runtime adds a barrier either way, but volatile indicates that this field has - // unsynchronized readers meaning the all writes initializing the list must be published before updating the _providers reference. - private volatile List _providers; + public static ReferenceCountedProviders Create(List providers) => new ActiveReferenceCountedProviders(providers); - public ReferenceCountedProviders(List providers) - { - _providers = providers; - } + // If anything references DisposedReferenceCountedProviders, it indicates something is using the ConfigurationManager after it's been disposed. + // We could preemptively throw an ODE from ReferenceCountedProviderManager.GetReference() instead of returning this type, but this might + // break existing apps that are previously able to continue to read configuration after disposing an ConfigurationManager. + public static ReferenceCountedProviders CreateDisposed(List providers) => new DisposedReferenceCountedProviders(providers); + + public abstract List Providers { get; set; } + // This is only used to support IConfigurationRoot.Providers because we cannot track the lifetime of that reference. + public abstract List NonReferenceCountedProviders { get; } - public List Providers + public abstract void AddReference(); + // This is Dispose() rather than RemoveReference() so we can conveniently release a reference at the end of a using block. + public abstract void Dispose(); + + private sealed class ActiveReferenceCountedProviders : ReferenceCountedProviders { - get + private long _refCount = 1; + // volatile is not strictly necessary because the runtime adds a barrier either way, but volatile indicates that this field has + // unsynchronized readers meaning the all writes initializing the list must be published before updating the _providers reference. + private volatile List _providers; + + public ActiveReferenceCountedProviders(List providers) { - Debug.Assert(_refCount > 0); - return _providers; + _providers = providers; } - set + + public override List Providers { - Debug.Assert(_refCount > 0); - _providers = value; + get + { + Debug.Assert(_refCount > 0); + return _providers; + } + set + { + Debug.Assert(_refCount > 0); + _providers = value; + } } - } - // This is only used to support IConfigurationRoot.Providers because we cannot track the lifetime of that reference. - public List NonReferenceCountedProviders => _providers; + public override List NonReferenceCountedProviders => _providers; - public void AddReference() - { - // AddReference() is always called with a lock to ensure _refCount hasn't already decremented to zero. - Debug.Assert(_refCount > 0); - Interlocked.Increment(ref _refCount); - } + public override void AddReference() + { + // AddReference() is always called with a lock to ensure _refCount hasn't already decremented to zero. + Debug.Assert(_refCount > 0); + Interlocked.Increment(ref _refCount); + } - // This is not a "real" Dispose(). It exists to conveniently release a reference at the end of a using block. - public void Dispose() - { - if (Interlocked.Decrement(ref _refCount) == 0) + public override void Dispose() { - foreach (IConfigurationProvider provider in _providers) + if (Interlocked.Decrement(ref _refCount) == 0) { - (provider as IDisposable)?.Dispose(); + foreach (IConfigurationProvider provider in _providers) + { + (provider as IDisposable)?.Dispose(); + } } } } - // This is the "real" Dispose() that is only called as part of ConfigurationManager.Dispose(). - // If there are any active references, that indicates a use-after-dispose bug in the code using the ConfigurationManager. - // - // If there are active references after dispose, the providers could get disposed more than once. We could prevent this - // by preemptively throwing an ODE from ReferenceCountedProviderManager.GetReference() after it's disposed, but this might - // break existing apps that are today able to continue to read configuration after disposing an ConfigurationManager. - public void ReleaseFinalReference() + private sealed class DisposedReferenceCountedProviders : ReferenceCountedProviders { - Debug.Assert(_refCount == 1); - Dispose(); + public DisposedReferenceCountedProviders(List providers) + { + Providers = providers; + } + + public override List Providers { get; set; } + public override List NonReferenceCountedProviders => Providers; + + public override void AddReference() { } + public override void Dispose() { } } } } diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs index 1552ba41a7d776..8cea565a1df9b1 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs @@ -12,7 +12,8 @@ namespace Microsoft.Extensions.Configuration internal sealed class ReferenceCountedProviderManager : IDisposable { private readonly object _replaceProvidersLock = new object(); - private ReferenceCountedProviders _refCountedProviders = new(new List()); + private ReferenceCountedProviders _refCountedProviders = ReferenceCountedProviders.Create(new List()); + private bool _disposed; // This is only used to support IConfigurationRoot.Providers because we cannot track the lifetime of that reference. public IEnumerable NonReferenceCountedProviders => _refCountedProviders.NonReferenceCountedProviders; @@ -35,7 +36,12 @@ public void ReplaceProviders(List providers) lock (_replaceProvidersLock) { - _refCountedProviders = new ReferenceCountedProviders(providers); + if (_disposed) + { + throw new ObjectDisposedException(nameof(ConfigurationManager)); + } + + _refCountedProviders = ReferenceCountedProviders.Create(providers); } // Decrement the reference count to the old providers. If they are being concurrently read from @@ -45,17 +51,36 @@ public void ReplaceProviders(List providers) public void AddProvider(IConfigurationProvider provider) { - // Maintain existing references, but replace list with copy containing new item. - _refCountedProviders.Providers = new List(_refCountedProviders.Providers) + lock (_replaceProvidersLock) { - provider - }; + if (_disposed) + { + throw new ObjectDisposedException(nameof(ConfigurationManager)); + } + + // Maintain existing references, but replace list with copy containing new item. + _refCountedProviders.Providers = new List(_refCountedProviders.Providers) + { + provider + }; + } } public void Dispose() { - // Equivalent to _refCountedProvider.Dispose() but asserts there a no concurrent references. - _refCountedProviders.ReleaseFinalReference(); + ReferenceCountedProviders oldRefCountedProviders = _refCountedProviders; + + lock (_replaceProvidersLock) + { + _disposed = true; + + // Create a non-reference-counting ReferenceCountedProviders instance now that the ConfigurationManager is disposed. + // We could preemptively throw an ODE from ReferenceCountedProviderManager.GetReference() instead, but this might + // break existing apps that were previously able to continue to read configuration after disposing an ConfigurationManager. + _refCountedProviders = ReferenceCountedProviders.CreateDisposed(oldRefCountedProviders.Providers); + } + + oldRefCountedProviders.Dispose(); } } } From 1725b2b41e85d8bf19c27e2e1cb35639a37980d5 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 11 Jan 2022 16:34:34 -0800 Subject: [PATCH 13/14] Add DisposingConfigurationManagerCausesOnlySourceChangesToThrow --- .../tests/ConfigurationManagerTest.cs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs index 414962d5c10d87..1a61a47394f2b4 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs @@ -240,6 +240,24 @@ public async Task ProviderDisposeDelayedWaitingOnConcurrentRead(Action(() => config.AddInMemoryCollection()); + Assert.Throws(() => ((IConfigurationBuilder)config).Sources.Clear()); + } + // Moq heavily utilizes RefEmit, which does not work on most aot workloads [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))] public void DisposesChangeTokenRegistrationsOnDispose() From 9073ebca94df87a230b148b55d3054d28014b5eb Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 13 Jan 2022 14:14:18 -0800 Subject: [PATCH 14/14] Lazily allocate DisposedReferenceCountedProviders --- .../src/ReferenceCountedProviders.cs | 5 ++++- .../src/ReferenceCountedProvidersManager.cs | 19 +++++++++++++------ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs index 4a3755c5ac1f96..2c2e32fc7e47d0 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs @@ -19,7 +19,10 @@ internal abstract class ReferenceCountedProviders : IDisposable public static ReferenceCountedProviders CreateDisposed(List providers) => new DisposedReferenceCountedProviders(providers); public abstract List Providers { get; set; } - // This is only used to support IConfigurationRoot.Providers because we cannot track the lifetime of that reference. + + // NonReferenceCountedProviders is only used to: + // 1. Support IConfigurationRoot.Providers because we cannot track the lifetime of that reference. + // 2. Construct DisposedReferenceCountedProviders because the providers are disposed anyway and no longer reference counted. public abstract List NonReferenceCountedProviders { get; } public abstract void AddReference(); diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs index 8cea565a1df9b1..210e41f665a47a 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs @@ -20,10 +20,18 @@ internal sealed class ReferenceCountedProviderManager : IDisposable public ReferenceCountedProviders GetReference() { - // Lock to ensure oldRefCountedProviders.Dispose() in ReplaceProviders() doesn't decrement ref count to zero + // Lock to ensure oldRefCountedProviders.Dispose() in ReplaceProviders() or Dispose() doesn't decrement ref count to zero // before calling _refCountedProviders.AddReference(). lock (_replaceProvidersLock) { + if (_disposed) + { + // Return a non-reference-counting ReferenceCountedProviders instance now that the ConfigurationManager is disposed. + // We could preemptively throw an ODE instead, but this might break existing apps that were previously able to + // continue to read configuration after disposing an ConfigurationManager. + return ReferenceCountedProviders.CreateDisposed(_refCountedProviders.NonReferenceCountedProviders); + } + _refCountedProviders.AddReference(); return _refCountedProviders; } @@ -46,6 +54,7 @@ public void ReplaceProviders(List providers) // Decrement the reference count to the old providers. If they are being concurrently read from // the actual disposal of the old providers will be delayed until the final reference is released. + // Never dispose ReferenceCountedProviders with a lock because this may call into user code. oldRefCountedProviders.Dispose(); } @@ -70,16 +79,14 @@ public void Dispose() { ReferenceCountedProviders oldRefCountedProviders = _refCountedProviders; + // This lock ensures that we cannot reduce the ref count to zero before GetReference() calls AddReference(). + // Once _disposed is set, GetReference() stops reference counting. lock (_replaceProvidersLock) { _disposed = true; - - // Create a non-reference-counting ReferenceCountedProviders instance now that the ConfigurationManager is disposed. - // We could preemptively throw an ODE from ReferenceCountedProviderManager.GetReference() instead, but this might - // break existing apps that were previously able to continue to read configuration after disposing an ConfigurationManager. - _refCountedProviders = ReferenceCountedProviders.CreateDisposed(oldRefCountedProviders.Providers); } + // Never dispose ReferenceCountedProviders with a lock because this may call into user code. oldRefCountedProviders.Dispose(); } }