Avoid deadlock with ConfigurationManager (#62209)

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Author: halter73

src/libraries/Microsoft.Extensions.Configuration/src/ConfigurationManager.cs

@@ -14,15 +14,20 @@ namespace Microsoft.Extensions.Configuration
 {
     /// <summary>
     /// ConfigurationManager is a mutable configuration object. It is both an <see cref="IConfigurationBuilder"/> and an <see cref="IConfigurationRoot"/>.
-    /// 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.
     /// </summary>
     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<IConfigurationProvider> _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<IDisposable> _changeTokenRegistrations = new();
         private ConfigurationReloadToken _changeToken = new();
 
@@ -43,55 +48,36 @@ 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);
             }
         }
 
         /// <inheritdoc/>
         public IConfigurationSection GetSection(string key) => new ConfigurationSection(this, key);
 
         /// <inheritdoc/>
-        public IEnumerable<IConfigurationSection> GetChildren()
-        {
-            lock (_providerLock)
-            {
-                // ToList() to eagerly evaluate inside lock.
-                return this.GetChildrenImplementation(null).ToList();
-            }
-        }
+        public IEnumerable<IConfigurationSection> GetChildren() => this.GetChildrenImplementation(null);
 
         IDictionary<string, object> IConfigurationBuilder.Properties => _properties;
 
         IList<IConfigurationSource> IConfigurationBuilder.Sources => _sources;
 
-        IEnumerable<IConfigurationProvider> IConfigurationRoot.Providers
-        {
-            get
-            {
-                lock (_providerLock)
-                {
-                    return new List<IConfigurationProvider>(_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<IConfigurationProvider> IConfigurationRoot.Providers => _providerManager.NonReferenceCountedProviders;
 
         /// <inheritdoc/>
         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<IConfigurationProvider>();
 
-                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<IConfigurationSource>
+        private sealed class ConfigurationSources : IList<IConfigurationSource>
         {
             private readonly List<IConfigurationSource> _sources = new();
             private readonly ConfigurationManager _config;
@@ -259,7 +237,7 @@ IEnumerator IEnumerable.GetEnumerator()
             }
         }
 
-        private class ConfigurationBuilderProperties : IDictionary<string, object>
+        private sealed class ConfigurationBuilderProperties : IDictionary<string, object>
         {
             private readonly Dictionary<string, object> _properties = new();
             private readonly ConfigurationManager _config;

src/libraries/Microsoft.Extensions.Configuration/src/InternalConfigurationRootExtensions.cs

@@ -20,11 +20,24 @@ internal static class InternalConfigurationRootExtensions
         /// <returns>Immediate children sub-sections of section specified by key.</returns>
         internal static IEnumerable<IConfigurationSection> GetChildrenImplementation(this IConfigurationRoot root, string? path)
         {
-            return root.Providers
+            using ReferenceCountedProviders? reference = (root as ConfigurationManager)?.GetProvidersReference();
+            IEnumerable<IConfigurationProvider> providers = reference?.Providers ?? root.Providers;
+
+            IEnumerable<IConfigurationSection> children = providers
                 .Aggregate(Enumerable.Empty<string>(),
                     (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();
+            }
         }
     }
 }

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<IConfigurationProvider> 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<IConfigurationProvider> providers) => new DisposedReferenceCountedProviders(providers);
+
+        public abstract List<IConfigurationProvider> 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<IConfigurationProvider> 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<IConfigurationProvider> _providers;
+
+            public ActiveReferenceCountedProviders(List<IConfigurationProvider> providers)
+            {
+                _providers = providers;
+            }
+
+            public override List<IConfigurationProvider> Providers
+            {
+                get
+                {
+                    Debug.Assert(_refCount > 0);
+                    return _providers;
+                }
+                set
+                {
+                    Debug.Assert(_refCount > 0);
+                    _providers = value;
+                }
+            }
+
+            public override List<IConfigurationProvider> 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<IConfigurationProvider> providers)
+            {
+                Providers = providers;
+            }
+
+            public override List<IConfigurationProvider> Providers { get; set; }
+            public override List<IConfigurationProvider> NonReferenceCountedProviders => Providers;
+
+            public override void AddReference() { }
+            public override void Dispose() { }
+        }
+    }
+}