Make getting lazy properties on ServiceConfig threadsafe (#1752)

Co-authored-by: Brennan <brecon@microsoft.com>

Author: JamesNK

src/Grpc.Net.Client/Internal/Configuration/ConfigProperty.cs

@@ -20,7 +20,7 @@
 
 namespace Grpc.Net.Client.Internal.Configuration
 {
-    internal struct ConfigProperty<TValue, TInner> where TValue : IConfigValue
+    internal sealed class ConfigProperty<TValue, TInner> where TValue : IConfigValue
     {
         private TValue? _value;
         private readonly Func<TInner?, TValue?> _valueFactory;
@@ -37,13 +37,23 @@ public ConfigProperty(Func<TInner?, TValue?> valueFactory, string key)
         {
             if (_value == null)
             {
-                var innerValue = inner.GetValue<TInner>(_key);
-                _value = _valueFactory(innerValue);
-
-                if (_value != null && innerValue == null)
+                // Multiple threads can get a property at the same time. We want this to be safe.
+                // Because a value could be lazily initialized, lock to ensure multiple threads
+                // don't try to update the underlying dictionary at the same time.
+                lock (this)
                 {
-                    // Set newly created value
-                    SetValue(inner, _value);
+                    // Double-check locking.
+                    if (_value == null)
+                    {
+                        var innerValue = inner.GetValue<TInner>(_key);
+                        _value = _valueFactory(innerValue);
+
+                        if (_value != null && innerValue == null)
+                        {
+                            // Set newly created value
+                            SetValue(inner, _value);
+                        }
+                    }
                 }
             }
 

src/Grpc.Net.Client/Internal/Configuration/Values.cs

@@ -20,7 +20,7 @@
 
 namespace Grpc.Net.Client.Internal.Configuration
 {
-    internal class Values<T, TInner> : IList<T>, IConfigValue
+    internal sealed class Values<T, TInner> : IList<T>, IConfigValue
     {
         internal readonly IList<TInner> Inner;
 

test/Grpc.Net.Client.Tests/Balancer/ConnectionManagerTests.cs

@@ -329,7 +329,7 @@ public async Task UpdateAddresses_ConnectIsInProgress_InProgressConnectIsCancele
 
         private static ConnectionManager CreateConnectionManager(
             ILoggerFactory loggerFactory,
-            TestResolver resolver,
+            Resolver resolver,
             TestSubchannelTransportFactory transportFactory)
         {
             return new ConnectionManager(

test/Grpc.Net.Client.Tests/ServiceConfigTests.cs

@@ -137,6 +137,38 @@ public void LoadBalancingConfig_CreateUnderlyingConfig()
             Assert.IsNotNull((IDictionary<string, object>)pickFirstConfig[LoadBalancingConfig.PickFirstPolicyName]);
         }
 
+        [Test]
+        public async Task ServiceConfig_LoadBalancingConfigs_ConcurrentAccess()
+        {
+            // Arrange & Act
+            for (var i = 0; i < 100; i++)
+            {
+                await AccessServiceConfigConcurrently();
+            }
+        }
+
+        private static async Task AccessServiceConfigConcurrently()
+        {
+            var serviceConfig = new ServiceConfig();
+
+            List<Task> connectTasks = new List<Task>();
+            for (var i = 0; i < 100; i++)
+            {
+                connectTasks.Add(Task.Run(ConnectAsync));
+            }
+
+            await Task.WhenAll(connectTasks);
+
+            void ConnectAsync()
+            {
+                // Excuse to access LoadBalancingConfigs
+                if (serviceConfig.LoadBalancingConfigs.Count > 0)
+                {
+                    serviceConfig.ToString();
+                }
+            }
+        }
+
         [Test]
         public void LoadBalancingConfig_ReadUnderlyingConfig()
         {