From 4fc363f74f8776177d990e10c92a39743a18de38 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 13 Jan 2022 17:20:28 -0800 Subject: [PATCH 1/2] Avoid deadlock with ConfigurationManager --- .../src/ConfigurationManager.cs | 110 ++++++-------- .../InternalConfigurationRootExtensions.cs | 15 +- .../src/ReferenceCountedProviders.cs | 93 ++++++++++++ .../src/ReferenceCountedProvidersManager.cs | 93 ++++++++++++ .../tests/ConfigurationManagerTest.cs | 143 ++++++++++++++++++ 5 files changed, 387 insertions(+), 67 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 9578f3c334ac3f..84cceb06218d74 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs @@ -12,15 +12,20 @@ namespace Microsoft.Extensions.Configuration { /// /// Configuration is 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 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; - private readonly object _providerLock = new(); - private readonly List _providers = 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 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(); @@ -43,17 +48,13 @@ public string this[string key] { get { - lock (_providerLock) - { - return ConfigurationRoot.GetConfiguration(_providers, key); - } + using ReferenceCountedProviders reference = _providerManager.GetReference(); + return ConfigurationRoot.GetConfiguration(reference.Providers, key); } set { - lock (_providerLock) - { - ConfigurationRoot.SetConfiguration(_providers, key, value); - } + using ReferenceCountedProviders reference = _providerManager.GetReference(); + ConfigurationRoot.SetConfiguration(reference.Providers, key, value); } } @@ -61,37 +62,22 @@ public string this[string key] 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); - } - } - } + // 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.NonReferenceCountedProviders; /// public void Dispose() { - lock (_providerLock) - { - DisposeRegistrationsAndProvidersUnsynchronized(); - } + DisposeRegistrations(); + _providerManager.Dispose(); } IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source) @@ -106,9 +92,9 @@ IConfigurationBuilder IConfigurationBuilder.Add(IConfigurationSource source) void IConfigurationRoot.Reload() { - lock (_providerLock) + using (ReferenceCountedProviders reference = _providerManager.GetReference()) { - foreach (var provider in _providers) + foreach (IConfigurationProvider provider in reference.Providers) { provider.Load(); } @@ -117,6 +103,8 @@ void IConfigurationRoot.Reload() RaiseChanged(); } + internal ReferenceCountedProviders GetProvidersReference() => _providerManager.GetReference(); + private void RaiseChanged() { var previousToken = Interlocked.Exchange(ref _changeToken, new ConfigurationReloadToken()); @@ -126,59 +114,49 @@ 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); + IConfigurationProvider provider = source.Build(this); - provider.Load(); - _changeTokenRegistrations.Add(ChangeToken.OnChange(() => provider.GetReloadToken(), () => RaiseChanged())); - } + provider.Load(); + _changeTokenRegistrations.Add(ChangeToken.OnChange(() => provider.GetReloadToken(), () => RaiseChanged())); + _providerManager.AddProvider(provider); RaiseChanged(); } // Something other than Add was called on IConfigurationBuilder.Sources or IConfigurationBuilder.Properties has changed. private void ReloadSources() { - lock (_providerLock) - { - DisposeRegistrationsAndProvidersUnsynchronized(); + DisposeRegistrations(); - _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 (IConfigurationSource source in _sources) + { + newProvidersList.Add(source.Build(this)); + } + + foreach (IConfigurationProvider p in newProvidersList) + { + p.Load(); + _changeTokenRegistrations.Add(ChangeToken.OnChange(() => p.GetReloadToken(), () => RaiseChanged())); } + _providerManager.ReplaceProviders(newProvidersList); RaiseChanged(); } - private void DisposeRegistrationsAndProvidersUnsynchronized() + private void DisposeRegistrations() { // dispose change token registrations - foreach (var registration in _changeTokenRegistrations) + foreach (IDisposable registration in _changeTokenRegistrations) { registration.Dispose(); } - - // dispose providers - foreach (var provider in _providers) - { - (provider as IDisposable)?.Dispose(); - } } - private class ConfigurationSources : IList + private sealed class ConfigurationSources : IList { private readonly List _sources = new(); private readonly ConfigurationManager _config; @@ -259,7 +237,7 @@ IEnumerator IEnumerable.GetEnumerator() } } - private class ConfigurationBuilderProperties : IDictionary + private sealed class ConfigurationBuilderProperties : IDictionary { private readonly Dictionary _properties = new(); private readonly ConfigurationManager _config; diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs b/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs index bff5c527104008..de86a8473d9812 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs @@ -20,11 +20,24 @@ internal static class InternalConfigurationRootExtensions /// Immediate children sub-sections of section specified by key. internal static IEnumerable GetChildrenImplementation(this IConfigurationRoot root, string path) { - return root.Providers + using ReferenceCountedProviders? reference = (root as ConfigurationManager)?.GetProvidersReference(); + IEnumerable providers = reference?.Providers ?? root.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))); + + 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 new file mode 100644 index 00000000000000..2c2e32fc7e47d0 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProviders.cs @@ -0,0 +1,93 @@ +// 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 abstract class ReferenceCountedProviders : IDisposable + { + public static ReferenceCountedProviders Create(List providers) => new ActiveReferenceCountedProviders(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; } + + // 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(); + // 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 + { + 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) + { + _providers = providers; + } + + public override List Providers + { + get + { + Debug.Assert(_refCount > 0); + return _providers; + } + set + { + Debug.Assert(_refCount > 0); + _providers = value; + } + } + + public override List NonReferenceCountedProviders => _providers; + + 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); + } + + public override void Dispose() + { + if (Interlocked.Decrement(ref _refCount) == 0) + { + foreach (IConfigurationProvider provider in _providers) + { + (provider as IDisposable)?.Dispose(); + } + } + } + } + + private sealed class DisposedReferenceCountedProviders : ReferenceCountedProviders + { + 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 new file mode 100644 index 00000000000000..210e41f665a47a --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Configuration/src/ReferenceCountedProvidersManager.cs @@ -0,0 +1,93 @@ +// 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 = 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; + + public ReferenceCountedProviders GetReference() + { + // 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; + } + } + + // Providers should never be concurrently modified. Reading during modification is allowed. + public void ReplaceProviders(List providers) + { + ReferenceCountedProviders oldRefCountedProviders = _refCountedProviders; + + lock (_replaceProvidersLock) + { + 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 + // 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(); + } + + public void AddProvider(IConfigurationProvider provider) + { + lock (_replaceProvidersLock) + { + 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() + { + 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; + } + + // Never dispose ReferenceCountedProviders with a lock because this may call into user code. + oldRefCountedProviders.Dispose(); + } + } +} diff --git a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs index 89da7883517544..aa36cceb424e5d 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration/tests/ConfigurationManagerTest.cs @@ -4,6 +4,8 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; +using System.Threading.Tasks; using Microsoft.Extensions.Configuration.Memory; using Microsoft.Extensions.Primitives; using Moq; @@ -171,6 +173,91 @@ public void DisposesProvidersOnRemoval() Assert.True(provider5.IsDisposed); } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] + public async Task ProviderCanBlockLoadWaitingOnConcurrentRead() + { + using var mre = new ManualResetEventSlim(false); + var provider = new BlockLoadOnMREProvider(mre, timeout: TimeSpan.FromSeconds(30)); + + var config = new ConfigurationManager(); + IConfigurationBuilder builder = config; + + // builder.Add(source) will block on provider.Load(). + var loadTask = Task.Run(() => builder.Add(new TestConfigurationSource(provider))); + 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; + } + + 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 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() 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. + builder.Sources.Clear(); + + Assert.False(provider.IsDisposed); + + // Unblock TryRead() or GetChildKeys() + mre.Set(); + + // 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 the concurrentReadAction releases the last reference to the provider. + Assert.True(provider.IsDisposed); + } + + [Fact] + public void DisposingConfigurationManagerCausesOnlySourceChangesToThrow() + { + var config = new ConfigurationManager + { + ["TestKey"] = "TestValue", + }; + + config.Dispose(); + + Assert.Equal("TestValue", config["TestKey"]); + config["TestKey"] = "TestValue2"; + Assert.Equal("TestValue2", config["TestKey"]); + + Assert.Throws(() => config.AddInMemoryCollection()); + Assert.Throws(() => ((IConfigurationBuilder)config).Sources.Clear()); + } + [Fact] public void DisposesChangeTokenRegistrationsOnDispose() { @@ -1128,6 +1215,62 @@ public TestConfigurationProvider(string key, string value) => Data.Add(key, value); } + private class BlockLoadOnMREProvider : ConfigurationProvider + { + private readonly ManualResetEventSlim _mre; + private readonly TimeSpan _timeout; + + private readonly TaskCompletionSource _loadStartedTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public BlockLoadOnMREProvider(ManualResetEventSlim mre, TimeSpan timeout) + { + _mre = mre; + _timeout = timeout; + } + + public Task LoadStartedTask => _loadStartedTcs.Task; + + public override void Load() + { + _loadStartedTcs.SetResult(null); + Assert.True(_mre.Wait(_timeout), "BlockLoadOnMREProvider.Load() timed out."); + } + } + + private class BlockReadOnMREProvider : ConfigurationProvider, IDisposable + { + private readonly ManualResetEventSlim _mre; + private readonly TimeSpan _timeout; + + private readonly TaskCompletionSource _readStartedTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public BlockReadOnMREProvider(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), "BlockReadOnMREProvider.TryGet() timed out."); + return base.TryGet(key, out value); + } + + 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 { public bool IsDisposed { get; set; } From 3258ebbbb8df7caaa93423ca651449789f6a9dab Mon Sep 17 00:00:00 2001 From: Santiago Fernandez Madero Date: Mon, 7 Feb 2022 10:51:34 -0800 Subject: [PATCH 2/2] Add packaging changes --- .../src/Microsoft.Extensions.Configuration.csproj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.Configuration/src/Microsoft.Extensions.Configuration.csproj b/src/libraries/Microsoft.Extensions.Configuration/src/Microsoft.Extensions.Configuration.csproj index 62243f8db8537a..89d1b42bcfaa91 100644 --- a/src/libraries/Microsoft.Extensions.Configuration/src/Microsoft.Extensions.Configuration.csproj +++ b/src/libraries/Microsoft.Extensions.Configuration/src/Microsoft.Extensions.Configuration.csproj @@ -4,6 +4,8 @@ netstandard2.0;net461 true Implementation of key-value pair based configuration for Microsoft.Extensions.Configuration. Includes the memory configuration provider. + 1 + true