Resolver has separate error for fetching service config (#1475)

JamesNK · web-flow · commit e48a0364d5cc · 2021-11-16T20:50:33.000+13:00
diff --git a/src/Grpc.Net.Client/Balancer/DnsResolver.cs b/src/Grpc.Net.Client/Balancer/DnsResolver.cs
@@ -147,7 +147,7 @@ private async Task RefreshCoreAsync(CancellationToken cancellationToken)
 
                 var resolvedPort = _address.Port == -1 ? 80 : _address.Port;
                 var endpoints = addresses.Select(a => new BalancerAddress(a.ToString(), resolvedPort)).ToArray();
-                var resolverResult = ResolverResult.ForResult(endpoints, serviceConfig: null);
+                var resolverResult = ResolverResult.ForResult(endpoints);
                 _listener(resolverResult);
             }
             catch (Exception ex)
diff --git a/src/Grpc.Net.Client/Balancer/Internal/ConnectionManager.cs b/src/Grpc.Net.Client/Balancer/Internal/ConnectionManager.cs
@@ -35,6 +35,8 @@ namespace Grpc.Net.Client.Balancer.Internal
 {
     internal class ConnectionManager : IDisposable, IChannelControlHelper
     {
+        private static readonly ServiceConfig DefaultServiceConfig = new ServiceConfig();
+
         private readonly SemaphoreSlim _nextPickerLock;
         private readonly object _lock;
         internal readonly Resolver _resolver;
@@ -50,6 +52,7 @@ internal class ConnectionManager : IDisposable, IChannelControlHelper
         private Task? _resolveTask;
         private TaskCompletionSource<SubchannelPicker> _nextPickerTcs;
         private int _currentSubchannelId;
+        private ServiceConfig? _previousServiceConfig;
 
         internal ConnectionManager(
             Resolver resolver,
@@ -148,30 +151,65 @@ private void OnResolverResult(ResolverResult result)
                 throw new InvalidOperationException($"Load balancer not configured.");
             }
 
+            var channelStatus = result.Status;
+
             // https://github.com/grpc/proposal/blob/master/A21-service-config-error-handling.md
-            // 1. Only use resolved service config if not disabled.
-            // 2. Only use resolved service config if valid. This includes having a factory for one of the load balancing configs.
+            // Additionally, only use resolved service config if not disabled.
             LoadBalancingConfig? loadBalancingConfig = null;
-            if (result.ServiceConfig != null)
+            if (!DisableResolverServiceConfig)
             {
-                if (!DisableResolverServiceConfig)
+                ServiceConfig? workingServiceConfig = null;
+                if (result.ServiceConfig == null)
                 {
-                    if (result.ServiceConfig.LoadBalancingConfigs.Count > 0)
+                    // Step 4 and 5
+                    if (result.ServiceConfigStatus == null)
+                    {
+                        // Step 5: Use default service config if none is provided.
+                        _previousServiceConfig = DefaultServiceConfig;
+                        workingServiceConfig = DefaultServiceConfig;
+                    }
+                    else
                     {
-                        if (!ChildHandlerLoadBalancer.TryGetValidServiceConfigFactory(result.ServiceConfig.LoadBalancingConfigs, LoadBalancerFactories, out loadBalancingConfig, out var _))
+                        // Step 4
+                        if (_previousServiceConfig == null)
                         {
-                            ConnectionManagerLog.ResolverUnsupportedLoadBalancingConfig(Logger, result.ServiceConfig.LoadBalancingConfigs);
+                            // Step 4.ii: If no config was provided or set previously, then treat resolution as a failure.
+                            channelStatus = result.ServiceConfigStatus.GetValueOrDefault();
+                        }
+                        else
+                        {
+                            // Step 4.i: Continue using previous service config if it was set and a new one is not provided.
+                            workingServiceConfig = _previousServiceConfig;
+                            ConnectionManagerLog.ResolverServiceConfigFallback(Logger, result.ServiceConfigStatus.GetValueOrDefault());
                         }
                     }
                 }
                 else
+                {
+                    // Step 3: Use provided service config if it is set.
+                    workingServiceConfig = result.ServiceConfig;
+                    _previousServiceConfig = result.ServiceConfig;
+                }
+
+
+                if (workingServiceConfig?.LoadBalancingConfigs.Count > 0)
+                {
+                    if (!ChildHandlerLoadBalancer.TryGetValidServiceConfigFactory(workingServiceConfig.LoadBalancingConfigs, LoadBalancerFactories, out loadBalancingConfig, out var _))
+                    {
+                        ConnectionManagerLog.ResolverUnsupportedLoadBalancingConfig(Logger, workingServiceConfig.LoadBalancingConfigs);
+                    }
+                }
+            }
+            else
+            {
+                if (result.ServiceConfig != null)
                 {
                     ConnectionManagerLog.ResolverServiceConfigNotUsed(Logger);
                 }
             }
 
             var state = new ChannelState(
-                result.Status,
+                channelStatus,
                 result.Addresses,
                 loadBalancingConfig,
                 BalancerAttributes.Empty);
@@ -513,6 +551,9 @@ internal static class ConnectionManagerLog
         private static readonly Action<ILogger, Exception?> _pickWaiting =
             LoggerMessage.Define(LogLevel.Trace, new EventId(14, "PickWaiting"), "Waiting for a new picker.");
 
+        private static readonly Action<ILogger, Status, Exception?> _resolverServiceConfigFallback =
+            LoggerMessage.Define<Status>(LogLevel.Debug, new EventId(15, "ResolverServiceConfigFallback"), "Falling back to previously loaded service config. Resolver failure when retreiving or parsing service config with status: {Status}");
+
         public static void ResolverRefreshRequested(ILogger logger)
         {
             _resolverRefreshRequested(logger, null);
@@ -586,6 +627,11 @@ public static void PickWaiting(ILogger logger)
         {
             _pickWaiting(logger, null);
         }
+
+        public static void ResolverServiceConfigFallback(ILogger logger, Status status)
+        {
+            _resolverServiceConfigFallback(logger, status, null);
+        }
     }
 }
 #endif
diff --git a/src/Grpc.Net.Client/Balancer/Resolver.cs b/src/Grpc.Net.Client/Balancer/Resolver.cs
@@ -99,11 +99,12 @@ public sealed class ResolverResult
         private BalancerAttributes? _attributes;
 
         [DebuggerStepThrough]
-        private ResolverResult(Status status, IReadOnlyList<BalancerAddress>? addresses, ServiceConfig? serviceConfig)
+        private ResolverResult(Status status, IReadOnlyList<BalancerAddress>? addresses, ServiceConfig? serviceConfig, Status? serviceConfigStatus)
         {
             Status = status;
             Addresses = addresses;
             ServiceConfig = serviceConfig;
+            ServiceConfigStatus = serviceConfigStatus;
         }
 
         /// <summary>
@@ -121,6 +122,11 @@ private ResolverResult(Status status, IReadOnlyList<BalancerAddress>? addresses,
         /// </summary>
         public ServiceConfig? ServiceConfig { get; }
 
+        /// <summary>
+        /// Gets an optional service config status.
+        /// </summary>
+        public Status? ServiceConfigStatus { get; }
+
         /// <summary>
         /// Gets metadata attributes.
         /// </summary>
@@ -139,19 +145,36 @@ public static ResolverResult ForFailure(Status status)
                 throw new ArgumentException("Error status code must not be OK.", nameof(status));
             }
 
-            return new ResolverResult(status, addresses: null, serviceConfig: null);
+            return new ResolverResult(status, addresses: null, serviceConfig: null, serviceConfigStatus: null);
         }
 
         /// <summary>
         /// Create <see cref="ResolverResult"/> for the specified addresses.
         /// </summary>
         /// <param name="addresses">The resolved addresses.</param>
-        /// <param name="serviceConfig">An optional service config.</param>
         /// <returns>A resolver result.</returns>
         [DebuggerStepThrough]
-        public static ResolverResult ForResult(IReadOnlyList<BalancerAddress> addresses, ServiceConfig? serviceConfig)
+        public static ResolverResult ForResult(IReadOnlyList<BalancerAddress> addresses)
+        {
+            return new ResolverResult(Status.DefaultSuccess, addresses, serviceConfig: null, serviceConfigStatus: null);
+        }
+
+        /// <summary>
+        /// Create <see cref="ResolverResult"/> for the specified addresses and service config.
+        /// </summary>
+        /// <param name="addresses">The resolved addresses.</param>
+        /// <param name="serviceConfig">An optional service config. A <c>null</c> value indicates that the resolver either didn't retreive a service config or an error occurred. The error must be specified using <paramref name="serviceConfigStatus"/>.</param>
+        /// <param name="serviceConfigStatus">A service config status. The status indicates an error retreiveing or parsing the config. The status must not be <see cref="StatusCode.OK"/> if no service config is specified.</param>
+        /// <returns>A resolver result.</returns>
+        [DebuggerStepThrough]
+        public static ResolverResult ForResult(IReadOnlyList<BalancerAddress> addresses, ServiceConfig? serviceConfig, Status? serviceConfigStatus)
         {
-            return new ResolverResult(Status.DefaultSuccess, addresses, serviceConfig);
+            if (serviceConfigStatus?.StatusCode == StatusCode.OK && serviceConfig == null)
+            {
+                throw new ArgumentException("Service config status code must not be OK when there is no service config.", nameof(serviceConfigStatus));
+            }
+
+            return new ResolverResult(Status.DefaultSuccess, addresses, serviceConfig, serviceConfigStatus);
         }
     }
 }
diff --git a/src/Grpc.Net.Client/Balancer/StaticResolver.cs b/src/Grpc.Net.Client/Balancer/StaticResolver.cs
@@ -59,7 +59,7 @@ public override Task RefreshAsync(CancellationToken cancellationToken)
                 throw new InvalidOperationException("Resolver hasn't been started.");
             }
 
-            _listener(ResolverResult.ForResult(_addresses, serviceConfig: null));
+            _listener(ResolverResult.ForResult(_addresses));
             return Task.CompletedTask;
         }
 
diff --git a/test/FunctionalTests/Balancer/PickFirstBalancerTests.cs b/test/FunctionalTests/Balancer/PickFirstBalancerTests.cs
@@ -23,6 +23,7 @@
 using System.Linq;
 using System.Net;
 using System.Net.Http;
+using System.Net.Sockets;
 using System.Threading.Tasks;
 using FunctionalTestsWebsite;
 using Google.Protobuf;
@@ -102,6 +103,46 @@ Task<HelloReply> UnaryMethod(HelloRequest request, ServerCallContext context)
             Assert.AreEqual("127.0.0.1:50051", host);
         }
 
+        [Test]
+        public async Task UnaryCall_SingleAddressFailure_RpcError()
+        {
+            // Ignore errors
+            SetExpectedErrorsFilter(writeContext =>
+            {
+                return true;
+            });
+
+            string? host = null;
+            Task<HelloReply> UnaryMethod(HelloRequest request, ServerCallContext context)
+            {
+                host = context.Host;
+                return Task.FromResult(new HelloReply { Message = request.Name });
+            }
+
+            // Arrange
+            using var endpoint = BalancerHelpers.CreateGrpcEndpoint<HelloRequest, HelloReply>(50051, UnaryMethod, nameof(UnaryMethod));
+
+            var channel = await BalancerHelpers.CreateChannel(LoggerFactory, new PickFirstConfig(), new[] { endpoint.Address }).DefaultTimeout();
+
+            var client = TestClientFactory.Create(channel, endpoint.Method);
+
+            // Act
+            var reply = await client.UnaryCall(new HelloRequest { Name = "Balancer" }).ResponseAsync.DefaultTimeout();
+
+            // Assert
+            Assert.AreEqual("Balancer", reply.Message);
+            Assert.AreEqual("127.0.0.1:50051", host);
+
+            Logger.LogInformation("Ending " + endpoint.Address);
+            endpoint.Dispose();
+
+            var ex = await ExceptionAssert.ThrowsAsync<RpcException>(
+                () => client.UnaryCall(new HelloRequest { Name = "Balancer" }).ResponseAsync).DefaultTimeout();
+
+            Assert.AreEqual(StatusCode.Unavailable, ex.StatusCode);
+            Assert.IsInstanceOf(typeof(SocketException), ex.Status.DebugException);
+        }
+
         [Test]
         public async Task UnaryCall_UnavailableAddress_FallbackToWorkingAddress()
         {
diff --git a/test/Grpc.Net.Client.Tests/Balancer/ResolverTests.cs b/test/Grpc.Net.Client.Tests/Balancer/ResolverTests.cs
diff --git a/test/Shared/TestResolver.cs b/test/Shared/TestResolver.cs

Original file line number	Diff line number	Diff line change
`@@ -147,7 +147,7 @@ private async Task RefreshCoreAsync(CancellationToken cancellationToken)`
`147`	`147`
`148`	`148`	`var resolvedPort = _address.Port == -1 ? 80 : _address.Port;`
`149`	`149`	`var endpoints = addresses.Select(a => new BalancerAddress(a.ToString(), resolvedPort)).ToArray();`
`150`		`- var resolverResult = ResolverResult.ForResult(endpoints, serviceConfig: null);`
	`150`	`+ var resolverResult = ResolverResult.ForResult(endpoints);`
`151`	`151`	`_listener(resolverResult);`
`152`	`152`	`}`
`153`	`153`	`catch (Exception ex)`
Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ public override Task RefreshAsync(CancellationToken cancellationToken)`
`59`	`59`	`throw new InvalidOperationException("Resolver hasn't been started.");`
`60`	`60`	`}`
`61`	`61`
`62`		`- _listener(ResolverResult.ForResult(_addresses, serviceConfig: null));`
	`62`	`+ _listener(ResolverResult.ForResult(_addresses));`
`63`	`63`	`return Task.CompletedTask;`
`64`	`64`	`}`
`65`	`65`