Avoid deadlock with ConfigurationManager

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 (var reference = _providerManager.GetReference())
             {
-                foreach (var provider in _providers)
+                foreach (var 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);
+            var 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 (var source in _sources)
+            {
+                newProvidersList.Add(source.Build(this));
+            }
+
+            foreach (var 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)
             {
                 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,16 @@ 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();
+            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<string>(),
                     (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();
         }
     }
 }

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<IConfigurationProvider> _providers;
+
+        public ReferenceCountedProviders(List<IConfigurationProvider> providers)
+        {
+            _providers = providers;
+        }
+
+        public List<IConfigurationProvider> 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<IConfigurationProvider> 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();
+                }
+            }
+        }
+    }
+}

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<IConfigurationProvider>());
+
+        // This is only used to support IConfigurationRoot.Providers because we cannot track the lifetime of that reference.
+        public IEnumerable<IConfigurationProvider> 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<IConfigurationProvider> 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<IConfigurationProvider>(_refCountedProviders.Providers)
+            {
+                provider
+            };
+        }
+
+        public void Dispose() => _refCountedProviders.Dispose();
+    }
+}