Changes

src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc

@@ -229,17 +229,20 @@ bool ValueInJsonArray(grpc_json* array, const char* value) {
   return false;
 }
 
-char* ChooseServiceConfig(char* service_config_choice_json) {
+char* ChooseServiceConfig(char* service_config_choice_json,
+                          grpc_error** error) {
   grpc_json* choices_json = grpc_json_parse_string(service_config_choice_json);
   if (choices_json == nullptr || choices_json->type != GRPC_JSON_ARRAY) {
-    gpr_log(GPR_ERROR, "cannot parse service config JSON string");
+    *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+        "cannot parse service config JSON string");
     return nullptr;
   }
   char* service_config = nullptr;
   for (grpc_json* choice = choices_json->child; choice != nullptr;
        choice = choice->next) {
     if (choice->type != GRPC_JSON_OBJECT) {
-      gpr_log(GPR_ERROR, "cannot parse service config JSON string");
+      *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+          "cannot parse service config JSON string");
       break;
     }
     grpc_json* service_config_json = nullptr;
@@ -305,31 +308,40 @@ void AresDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) {
     Result result;
     result.addresses = std::move(*r->addresses_);
     if (r->service_config_json_ != nullptr) {
+      grpc_error* service_config_error = GRPC_ERROR_NONE;
       char* service_config_string =
-          ChooseServiceConfig(r->service_config_json_);
+          ChooseServiceConfig(r->service_config_json_, &service_config_error);
       gpr_free(r->service_config_json_);
-      if (service_config_string != nullptr) {
-        GRPC_CARES_TRACE_LOG("resolver:%p selected service config choice: %s",
-                             r, service_config_string);
-        grpc_error* service_config_error = GRPC_ERROR_NONE;
-        auto new_service_config =
-            ServiceConfig::Create(service_config_string, &service_config_error);
-        if (service_config_error == GRPC_ERROR_NONE) {
-          // Valid service config receivd
-          r->saved_service_config_ = std::move(new_service_config);
+      RefCountedPtr<ServiceConfig> new_service_config;
+      if (service_config_error == GRPC_ERROR_NONE) {
+        if (service_config_string != nullptr) {
+          GRPC_CARES_TRACE_LOG("resolver:%p selected service config choice: %s",
+                               r, service_config_string);
+          new_service_config = ServiceConfig::Create(service_config_string,
+                                                     &service_config_error);
+          gpr_free(service_config_string);
         } else {
-          if (r->saved_service_config_ != nullptr) {
-            // Ignore the new service config error, since we have a previously
-            // saved service config
-            GRPC_ERROR_UNREF(service_config_error);
-          } else {
-            // No previously valid service config found.
-            // service_config_error is passed to the channel.
-            result.service_config_error = service_config_error;
-          }
+          // Use an empty service config since we did not find a choice
+          GRPC_CARES_TRACE_LOG("resolver:%p selected service config choice: {}",
+                               r);
+          new_service_config =
+              ServiceConfig::Create("{}", &service_config_error);
+        }
+      }
+      if (service_config_error == GRPC_ERROR_NONE) {
+        // Valid service config receivd
+        r->saved_service_config_ = std::move(new_service_config);
+      } else {
+        if (r->saved_service_config_ != nullptr) {
+          // Ignore the new service config error, since we have a previously
+          // saved service config
+          GRPC_ERROR_UNREF(service_config_error);
+        } else {
+          // No previously valid service config found.
+          // service_config_error is passed to the channel.
+          result.service_config_error = service_config_error;
         }
       }
-      gpr_free(service_config_string);
     } else {
       // No service config received
       r->saved_service_config_.reset();

src/core/ext/filters/client_channel/resolver_result_parsing.cc

@@ -61,8 +61,8 @@ void ClientChannelServiceConfigParser::Register() {
 ProcessedResolverResult::ProcessedResolverResult(
     const Resolver::Result& resolver_result)
     : service_config_(resolver_result.service_config) {
-  // If resolver did not return a service config or returned an invalid service config, use the default
-  // specified via the client API.
+  // If resolver did not return a service config or returned an invalid service
+  // config, use the default specified via the client API.
   if (service_config_ == nullptr) {
     const char* service_config_json = grpc_channel_arg_get_string(
         grpc_channel_args_find(resolver_result.args, GRPC_ARG_SERVICE_CONFIG));
@@ -70,7 +70,8 @@ ProcessedResolverResult::ProcessedResolverResult(
       service_config_ =
           ServiceConfig::Create(service_config_json, &service_config_error_);
     } else {
-      service_config_error_ = GRPC_ERROR_REF(resolver_result.service_config_error);
+      service_config_error_ =
+          GRPC_ERROR_REF(resolver_result.service_config_error);
     }
   } else {
     service_config_error_ =

test/cpp/end2end/service_config_end2end_test.cc

@@ -188,14 +188,16 @@ class ServiceConfigEnd2endTest : public ::testing::Test {
   void SetNextResolutionValidServiceConfig(const std::vector<int>& ports) {
     grpc_core::ExecCtx exec_ctx;
     grpc_core::Resolver::Result result = BuildFakeResults(ports);
-    result.service_config = grpc_core::ServiceConfig::Create("{}", &result.service_config_error);
+    result.service_config =
+        grpc_core::ServiceConfig::Create("{}", &result.service_config_error);
     response_generator_->SetResponse(result);
   }
 
   void SetNextResolutionInvalidServiceConfig(const std::vector<int>& ports) {
     grpc_core::ExecCtx exec_ctx;
     grpc_core::Resolver::Result result = BuildFakeResults(ports);
-    result.service_config = grpc_core::ServiceConfig::Create("{", &result.service_config_error);
+    result.service_config =
+        grpc_core::ServiceConfig::Create("{", &result.service_config_error);
     response_generator_->SetResponse(result);
   }
 
@@ -415,7 +417,7 @@ TEST_F(ServiceConfigEnd2endTest, ValidServiceConfigAfterInvalidTest) {
   auto channel = BuildChannel();
   auto stub = BuildStub(channel);
   SetNextResolutionInvalidServiceConfig(GetServersPorts());
-  CheckRpcSendFailure(stub);  
+  CheckRpcSendFailure(stub);
   SetNextResolutionValidServiceConfig(GetServersPorts());
   CheckRpcSendOk(stub, DEBUG_LOCATION);
 }
@@ -427,7 +429,7 @@ TEST_F(ServiceConfigEnd2endTest, InvalidServiceConfigWithDefaultConfigTest) {
   auto channel = BuildChannel(args);
   auto stub = BuildStub(channel);
   SetNextResolutionInvalidServiceConfig(GetServersPorts());
-  CheckRpcSendOk(stub, DEBUG_LOCATION);  
+  CheckRpcSendOk(stub, DEBUG_LOCATION);
 }
 
 }  // namespace