Address review comments

src/core/ext/client_channel/resolver_registry.c

@@ -93,7 +93,6 @@ static grpc_resolver_factory *lookup_factory(const char *name) {
       return g_all_of_the_resolvers[i];
     }
   }
-
   return NULL;
 }
 

src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c

@@ -147,6 +147,10 @@ static void driver_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) {
       ares_process_fd(d->channel, read_fd, write_fd);
     }
   } else {
+    // error != GRPC_ERROR_NONE means the waiting timed out or the fd has been
+    // shutdown. In this case, the event driver cancels all the ongoing requests
+    // that are using its channel. The fds get cleaned up in the next
+    // grpc_ares_notify_on_event.
     ares_cancel(d->channel);
   }
   grpc_ares_notify_on_event(exec_ctx, d);
@@ -170,15 +174,13 @@ static void grpc_ares_notify_on_event(grpc_exec_ctx *exec_ctx,
       if (ARES_GETSOCK_READABLE(ev_driver->socks_bitmask, i) ||
           ARES_GETSOCK_WRITABLE(ev_driver->socks_bitmask, i)) {
         fd_node *fdn = get_fd(&ev_driver->fds, ev_driver->socks[i]);
-        if (!fdn) {
+        if (fdn == NULL) {
           char *fd_name;
           gpr_asprintf(&fd_name, "ares_ev_driver-%" PRIuPTR, i);
-
           fdn = gpr_malloc(sizeof(fd_node));
           fdn->grpc_fd = grpc_fd_create(ev_driver->socks[i], fd_name);
           grpc_pollset_set_add_fd(exec_ctx, ev_driver->pollset_set,
                                   fdn->grpc_fd);
-
           gpr_free(fd_name);
         }
         fdn->next = new_list;
@@ -197,9 +199,7 @@ static void grpc_ares_notify_on_event(grpc_exec_ctx *exec_ctx,
   }
 
   while (ev_driver->fds != NULL) {
-    fd_node *cur;
-
-    cur = ev_driver->fds;
+    fd_node *cur = ev_driver->fds;
     ev_driver->fds = ev_driver->fds->next;
     grpc_pollset_set_del_fd(exec_ctx, ev_driver->pollset_set, cur->grpc_fd);
     grpc_fd_shutdown(exec_ctx, cur->grpc_fd);
@@ -218,7 +218,6 @@ static void grpc_ares_notify_on_event(grpc_exec_ctx *exec_ctx,
   if (ev_driver->closing) {
     ares_destroy(ev_driver->channel);
     gpr_free(ev_driver);
-    return;
   }
 }
 
@@ -228,9 +227,8 @@ void grpc_ares_ev_driver_start(grpc_exec_ctx *exec_ctx,
   if (ev_driver->working) {
     gpr_mu_unlock(&ev_driver->mu);
     return;
-  } else {
-    ev_driver->working = true;
   }
+  ev_driver->working = true;
   gpr_mu_unlock(&ev_driver->mu);
   grpc_ares_notify_on_event(exec_ctx, ev_driver);
 }

src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c

@@ -61,17 +61,31 @@ static gpr_once g_basic_init = GPR_ONCE_INIT;
 static gpr_mu g_init_mu;
 
 typedef struct grpc_ares_request {
-  char *name;
+  /** host to resolve, parsed from the name to resolve, set in
+      grpc_resolve_address_ares_impl */
   char *host;
+  /** port to fill in sockaddr_in, parsed from the name to resolve, set in
+      grpc_resolve_address_ares_impl */
   char *port;
+  /** default port to use, set in grpc_resolve_address_ares_impl */
   char *default_port;
-  grpc_polling_entity *pollent;
+  /** closure to call when the request completes, set in
+      grpc_resolve_address_ares_impl */
   grpc_closure *on_done;
+  /** the pointer to receive the resolved addresses, set in
+      grpc_resolve_address_ares_impl */
   grpc_resolved_addresses **addrs_out;
+  /** the closure wraps request_resolving_address, initialized in
+      grpc_resolve_address_ares_impl */
   grpc_closure request_closure;
-  int pending_quries;
-  int success;
+  /** number of ongoing queries, set in grpc_resolve_address_ares_impl */
+  int pending_queries;
+  /** is there at least one successful query, set in on_done_cb */
+  bool success;
+  /** the errors explaining the request failure, set in on_done_cb */
   grpc_error *error;
+  /** the evernt driver owned by this request, created in
+      grpc_resolve_address_ares_impl */
   grpc_ares_ev_driver *ev_driver;
 } grpc_ares_request;
 
@@ -79,8 +93,6 @@ static void do_basic_init(void) { gpr_mu_init(&g_init_mu); }
 
 static void destroy_request(grpc_ares_request *request) {
   grpc_ares_ev_driver_destroy(request->ev_driver);
-
-  gpr_free(request->name);
   gpr_free(request->host);
   gpr_free(request->port);
   gpr_free(request->default_port);
@@ -99,37 +111,33 @@ static void on_done_cb(void *arg, int status, int timeouts,
                        struct hostent *hostent) {
   grpc_ares_request *r = (grpc_ares_request *)arg;
   grpc_resolved_addresses **addresses = r->addrs_out;
-  size_t i;
-  size_t prev_naddr;
-
   if (status == ARES_SUCCESS) {
     GRPC_ERROR_UNREF(r->error);
     r->error = GRPC_ERROR_NONE;
-    r->success = 1;
+    r->success = true;
     if (*addresses == NULL) {
       *addresses = gpr_malloc(sizeof(grpc_resolved_addresses));
       (*addresses)->naddrs = 0;
       (*addresses)->addrs = NULL;
     }
-
-    prev_naddr = (*addresses)->naddrs;
+    size_t prev_naddr = (*addresses)->naddrs;
+    size_t i;
     for (i = 0; hostent->h_addr_list[i] != NULL; i++) {
     }
     (*addresses)->naddrs += i;
-
     (*addresses)->addrs =
         gpr_realloc((*addresses)->addrs,
                     sizeof(grpc_resolved_address) * (*addresses)->naddrs);
-
     for (i = prev_naddr; i < (*addresses)->naddrs; i++) {
       memset(&(*addresses)->addrs[i], 0, sizeof(grpc_resolved_address));
       if (hostent->h_addrtype == AF_INET6) {
-        char output[INET6_ADDRSTRLEN];
-        struct sockaddr_in6 *addr;
-
         (*addresses)->addrs[i].len = sizeof(struct sockaddr_in6);
-        addr = (struct sockaddr_in6 *)&(*addresses)->addrs[i].addr;
+        struct sockaddr_in6 *addr =
+            (struct sockaddr_in6 *)&(*addresses)->addrs[i].addr;
+        addr->sin6_family = (sa_family_t)hostent->h_addrtype;
+        addr->sin6_port = strhtons(r->port);
 
+        char output[INET6_ADDRSTRLEN];
         memcpy(&addr->sin6_addr, hostent->h_addr_list[i - prev_naddr],
                sizeof(struct in6_addr));
         ares_inet_ntop(AF_INET6, &addr->sin6_addr, output, INET6_ADDRSTRLEN);
@@ -137,40 +145,41 @@ static void on_done_cb(void *arg, int status, int timeouts,
                 "c-ares resolver gets a AF_INET6 result: \n"
                 "  addr: %s\n  port: %s\n",
                 output, r->port);
-        addr->sin6_family = (sa_family_t)hostent->h_addrtype;
-        addr->sin6_port = strhtons(r->port);
       } else {
-        char output[INET_ADDRSTRLEN];
-        struct sockaddr_in *addr;
-
         (*addresses)->addrs[i].len = sizeof(struct sockaddr_in);
-        addr = (struct sockaddr_in *)&(*addresses)->addrs[i].addr;
-
+        struct sockaddr_in *addr =
+            (struct sockaddr_in *)&(*addresses)->addrs[i].addr;
         memcpy(&addr->sin_addr, hostent->h_addr_list[i - prev_naddr],
                sizeof(struct in_addr));
+        addr->sin_family = (sa_family_t)hostent->h_addrtype;
+        addr->sin_port = strhtons(r->port);
+
+        char output[INET_ADDRSTRLEN];
         ares_inet_ntop(AF_INET, &addr->sin_addr, output, INET_ADDRSTRLEN);
         gpr_log(GPR_DEBUG,
                 "c-ares resolver gets a AF_INET result: \n"
                 "  addr: %s\n  port: %s\n",
                 output, r->port);
-        addr->sin_family = (sa_family_t)hostent->h_addrtype;
-        addr->sin_port = strhtons(r->port);
       }
     }
-    // ares_destroy(r->channel);
   } else if (!r->success) {
-    gpr_log(GPR_DEBUG, "c-ares status not ARES_SUCCESS");
-    // TODO(zyc): add more error detail
+    gpr_log(GPR_DEBUG, "c-ares status is not ARES_SUCCESS");
+    char *error_msg;
+    gpr_asprintf(&error_msg, "C-ares status is not ARES_SUCCESS: %s",
+                 ares_strerror(status));
+    grpc_error *error = GRPC_ERROR_CREATE(error_msg);
+    gpr_free(error_msg);
     if (r->error == GRPC_ERROR_NONE) {
-      r->error = GRPC_ERROR_CREATE("C-ares query error");
+      r->error = error;
+    } else {
+      r->error = grpc_error_add_child(error, r->error);
     }
   }
-  if (--r->pending_quries == 0) {
+  if (--r->pending_queries == 0) {
     grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
     grpc_exec_ctx_sched(&exec_ctx, r->on_done, r->error, NULL);
     grpc_exec_ctx_flush(&exec_ctx);
     grpc_exec_ctx_finish(&exec_ctx);
-
     destroy_request(r);
     gpr_free(r);
   }
@@ -181,17 +190,17 @@ static void request_resolving_address(grpc_exec_ctx *exec_ctx, void *arg,
   grpc_ares_request *r = (grpc_ares_request *)arg;
   grpc_ares_ev_driver *ev_driver = r->ev_driver;
   ares_channel *channel = grpc_ares_ev_driver_get_channel(ev_driver);
-  r->pending_quries = 1;
+  r->pending_queries = 1;
   if (grpc_ipv6_loopback_available()) {
-    r->pending_quries += 1;
+    ++r->pending_queries;
     ares_gethostbyname(*channel, r->host, AF_INET6, on_done_cb, r);
   }
   ares_gethostbyname(*channel, r->host, AF_INET, on_done_cb, r);
   grpc_ares_ev_driver_start(exec_ctx, ev_driver);
 }
 
-static int try_fake_resolve(const char *name, const char *port,
-                            grpc_resolved_addresses **addresses) {
+static int try_sockaddr_resolve(const char *name, const char *port,
+                                grpc_resolved_addresses **addresses) {
   struct sockaddr_in sa;
   struct sockaddr_in6 sa6;
   memset(&sa, 0, sizeof(struct sockaddr_in));
@@ -256,24 +265,23 @@ void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name,
     port = gpr_strdup(default_port);
   }
 
-  if (try_fake_resolve(host, port, addrs)) {
+  if (try_sockaddr_resolve(host, port, addrs)) {
     grpc_exec_ctx_sched(exec_ctx, on_done, GRPC_ERROR_NONE, NULL);
   } else {
     err = grpc_ares_ev_driver_create(&ev_driver, pollset_set);
     if (err != GRPC_ERROR_NONE) {
       grpc_exec_ctx_sched(exec_ctx, on_done, err, NULL);
-      return;
+      goto done;
     }
     r = gpr_malloc(sizeof(grpc_ares_request));
     r->ev_driver = ev_driver;
     r->on_done = on_done;
     r->addrs_out = addrs;
-    r->name = gpr_strdup(name);
     r->default_port = gpr_strdup(default_port);
     r->port = gpr_strdup(port);
     r->host = gpr_strdup(host);
-    r->pending_quries = 0;
-    r->success = 0;
+    r->pending_queries = 0;
+    r->success = false;
     r->error = GRPC_ERROR_NONE;
     grpc_closure_init(&r->request_closure, request_resolving_address, r);
     grpc_exec_ctx_sched(exec_ctx, &r->request_closure, GRPC_ERROR_NONE, NULL);
@@ -296,7 +304,12 @@ grpc_error *grpc_ares_init(void) {
   gpr_mu_unlock(&g_init_mu);
 
   if (status != ARES_SUCCESS) {
-    return GRPC_ERROR_CREATE("ares_library_init failed");
+    char *error_msg;
+    gpr_asprintf(&error_msg, "ares_library_init failed: %s",
+                 ares_strerror(status));
+    grpc_error *error = GRPC_ERROR_CREATE(error_msg);
+    gpr_free(error_msg);
+    return error;
   }
   return GRPC_ERROR_NONE;
 }

src/core/lib/security/credentials/fake/fake_credentials.c

@@ -37,7 +37,6 @@
 
 #include "src/core/lib/channel/channel_args.h"
 #include "src/core/lib/iomgr/executor.h"
-#include "src/core/lib/iomgr/timer.h"
 
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
@@ -95,21 +94,14 @@ static void md_only_test_destruct(grpc_call_credentials *creds) {
   grpc_credentials_md_store_unref(c->md_store);
 }
 
-typedef struct simulated_token_fetch_args {
-  grpc_timer timer;
-  grpc_credentials_metadata_request *md_request;
-} simulated_token_fetch_args;
-
 static void on_simulated_token_fetch_done(grpc_exec_ctx *exec_ctx,
                                           void *user_data, grpc_error *error) {
-  simulated_token_fetch_args *cb_arg = (simulated_token_fetch_args *)user_data;
-  grpc_md_only_test_credentials *c =
-      (grpc_md_only_test_credentials *)cb_arg->md_request->creds;
-  cb_arg->md_request->cb(exec_ctx, cb_arg->md_request->user_data,
-                         c->md_store->entries, c->md_store->num_entries,
-                         GRPC_CREDENTIALS_OK, NULL);
-  grpc_credentials_metadata_request_destroy(cb_arg->md_request);
-  gpr_free(cb_arg);
+  grpc_credentials_metadata_request *r =
+      (grpc_credentials_metadata_request *)user_data;
+  grpc_md_only_test_credentials *c = (grpc_md_only_test_credentials *)r->creds;
+  r->cb(exec_ctx, r->user_data, c->md_store->entries, c->md_store->num_entries,
+        GRPC_CREDENTIALS_OK, NULL);
+  grpc_credentials_metadata_request_destroy(r);
 }
 
 static void md_only_test_get_request_metadata(
@@ -117,16 +109,13 @@ static void md_only_test_get_request_metadata(
     grpc_polling_entity *pollent, grpc_auth_metadata_context context,
     grpc_credentials_metadata_cb cb, void *user_data) {
   grpc_md_only_test_credentials *c = (grpc_md_only_test_credentials *)creds;
+
   if (c->is_async) {
-    simulated_token_fetch_args *cb_arg =
-        gpr_malloc(sizeof(simulated_token_fetch_args));
-    cb_arg->md_request =
+    grpc_credentials_metadata_request *cb_arg =
         grpc_credentials_metadata_request_create(creds, cb, user_data);
-    grpc_timer_init(exec_ctx, &cb_arg->timer,
-                    gpr_time_add(gpr_now(GPR_CLOCK_MONOTONIC),
-                                 gpr_time_from_micros(100, GPR_TIMESPAN)),
-                    on_simulated_token_fetch_done, cb_arg,
-                    gpr_now(GPR_CLOCK_MONOTONIC));
+    grpc_executor_push(
+        grpc_closure_create(on_simulated_token_fetch_done, cb_arg),
+        GRPC_ERROR_NONE);
   } else {
     cb(exec_ctx, user_data, c->md_store->entries, 1, GRPC_CREDENTIALS_OK, NULL);
   }