Address review comments

5 files changed, 47 insertions, 45 deletions

diff --git a/doc/environment_variables.md b/doc/environment_variables.md
index d02801bc9b..11ac890cd5 100644
--- a/doc/environment_variables.md
+++ b/doc/environment_variables.md
@@ -65,3 +65,9 @@ some configuration as environment variables that can be set.
   - DEBUG - log all gRPC messages
   - INFO - log INFO and ERROR message
   - ERROR - log only errors
+
+* GRPC_DNS_RESOLVER
+  Declares which DNS resolver to use. Available DNS resolver include:
+  - ares - a DNS resolver based around the c-ares library
+  - native - a DNS resolver based around getaddrinfo(), creates a new thread to
+    perform name resolution
diff --git a/src/core/ext/resolver/dns/c_ares/dns_resolver_ares.c b/src/core/ext/resolver/dns/c_ares/dns_resolver_ares.c
index e1db898e7f..64d4a5933e 100644
--- a/src/core/ext/resolver/dns/c_ares/dns_resolver_ares.c
+++ b/src/core/ext/resolver/dns/c_ares/dns_resolver_ares.c
@@ -75,7 +75,7 @@ typedef struct {
   grpc_closure dns_ares_on_resolved_locked;
 
   /** Combiner guarding the rest of the state */
-  grpc_combiner *lock;
+  grpc_combiner *combiner;
   /** are we currently resolving? */
   bool resolving;
   /** which version of the result have we published? */
@@ -136,7 +136,7 @@ static void dns_ares_shutdown(grpc_exec_ctx *exec_ctx,
                               grpc_resolver *resolver) {
   ares_dns_resolver *r = (ares_dns_resolver *)resolver;
   GRPC_RESOLVER_REF(&r->base, "dns-ares-shutdown");
-  grpc_combiner_execute(exec_ctx, r->lock, &r->dns_ares_shutdown_locked,
+  grpc_combiner_execute(exec_ctx, r->combiner, &r->dns_ares_shutdown_locked,
                         GRPC_ERROR_NONE, false);
 }
 
@@ -155,7 +155,7 @@ static void dns_ares_channel_saw_error(grpc_exec_ctx *exec_ctx,
                                        grpc_resolver *resolver) {
   ares_dns_resolver *r = (ares_dns_resolver *)resolver;
   GRPC_RESOLVER_REF(&r->base, "ares-channel-saw-error");
-  grpc_combiner_execute(exec_ctx, r->lock,
+  grpc_combiner_execute(exec_ctx, r->combiner,
                         &r->dns_ares_channel_saw_error_locked, GRPC_ERROR_NONE,
                         false);
 }
@@ -175,7 +175,8 @@ static void dns_ares_on_retry_timer_locked(grpc_exec_ctx *exec_ctx, void *arg,
 static void dns_ares_on_retry_timer(grpc_exec_ctx *exec_ctx, void *arg,
                                     grpc_error *error) {
   ares_dns_resolver *r = arg;
-  grpc_combiner_execute(exec_ctx, r->lock, &r->dns_ares_on_retry_timer_locked,
+  grpc_combiner_execute(exec_ctx, r->combiner,
+                        &r->dns_ares_on_retry_timer_locked,
                         GRPC_ERROR_REF(error), false);
 }
 
@@ -229,7 +230,7 @@ static void dns_ares_on_resolved_locked(grpc_exec_ctx *exec_ctx, void *arg,
 static void dns_ares_on_resolved(grpc_exec_ctx *exec_ctx, void *arg,
                                  grpc_error *error) {
   ares_dns_resolver *r = arg;
-  grpc_combiner_execute(exec_ctx, r->lock, &r->dns_ares_on_resolved_locked,
+  grpc_combiner_execute(exec_ctx, r->combiner, &r->dns_ares_on_resolved_locked,
                         GRPC_ERROR_REF(error), false);
 }
 
@@ -268,7 +269,7 @@ static void dns_ares_next(grpc_exec_ctx *exec_ctx, grpc_resolver *resolver,
   args->on_complete = on_complete;
   args->resolver = resolver;
   GRPC_RESOLVER_REF(resolver, "ares-next");
-  grpc_combiner_execute(exec_ctx, r->lock,
+  grpc_combiner_execute(exec_ctx, r->combiner,
                         grpc_closure_create(dns_ares_next_locked, args),
                         GRPC_ERROR_NONE, false);
 }
@@ -301,7 +302,7 @@ static void dns_ares_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) {
   gpr_log(GPR_DEBUG, "dns_ares_destroy");
   ares_dns_resolver *r = (ares_dns_resolver *)gr;
   grpc_ares_ev_driver_destroy(exec_ctx, r->ev_driver);
-  grpc_combiner_destroy(exec_ctx, r->lock);
+  grpc_combiner_destroy(exec_ctx, r->combiner);
   grpc_ares_cleanup();
   if (r->resolved_result != NULL) {
     grpc_channel_args_destroy(r->resolved_result);
@@ -345,7 +346,7 @@ static grpc_resolver *dns_ares_create(grpc_resolver_args *args,
     gpr_free(r);
     return NULL;
   }
-  r->lock = grpc_combiner_create(NULL);
+  r->combiner = grpc_combiner_create(NULL);
   r->name_to_resolve = proxy_name == NULL ? gpr_strdup(path) : proxy_name;
   r->default_port = gpr_strdup(default_port);
   grpc_arg server_name_arg;
diff --git a/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c b/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c
index 49b7485c94..8d8f5f0580 100644
--- a/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c
+++ b/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c
@@ -292,8 +292,8 @@ static void grpc_ares_notify_on_event_locked(grpc_exec_ctx *exec_ctx,
       gpr_mu_unlock(&fdn->mu);
     }
   }
-  // Any remaining fds in ev_driver->fds was not returned by ares_getsock() and
-  // is therefore no longer in use, so they can be shut donw and removed from
+  // Any remaining fds in ev_driver->fds were not returned by ares_getsock() and
+  // are therefore no longer in use, so they can be shut down and removed from
   // the list.
   while (ev_driver->fds != NULL) {
     fd_node *cur = ev_driver->fds;
diff --git a/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c b/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c
index cf43c941a0..9c83a8bf39 100644
--- a/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c
+++ b/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c
@@ -74,8 +74,6 @@ typedef struct grpc_ares_request {
   grpc_resolved_addresses **addrs_out;
   /** the evernt driver used by this request */
   grpc_ares_ev_driver *ev_driver;
-  /** the closure wraps request_resolving_address */
-  grpc_closure request_closure;
   /** number of ongoing queries */
   gpr_refcount pending_queries;
 
@@ -89,19 +87,6 @@ typedef struct grpc_ares_request {
 
 static void do_basic_init(void) { gpr_mu_init(&g_init_mu); }
 
-static void ares_request_unref(grpc_ares_request *r) {
-  if (gpr_unref(&r->pending_queries)) {
-    grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
-    grpc_exec_ctx_sched(&exec_ctx, r->on_done, r->error, NULL);
-    grpc_exec_ctx_finish(&exec_ctx);
-    gpr_mu_destroy(&r->mu);
-    gpr_free(r->host);
-    gpr_free(r->port);
-    gpr_free(r->default_port);
-    gpr_free(r);
-  }
-}
-
 static uint16_t strhtons(const char *port) {
   if (strcmp(port, "http") == 0) {
     return htons(80);
@@ -180,22 +165,23 @@ static void on_done_cb(void *arg, int status, int timeouts,
     }
   }
   gpr_mu_unlock(&r->mu);
-  ares_request_unref(r);
-}
-
-static void request_resolving_address(grpc_exec_ctx *exec_ctx, void *arg,
-                                      grpc_error *error) {
-  grpc_ares_request *r = (grpc_ares_request *)arg;
-  grpc_ares_ev_driver *ev_driver = r->ev_driver;
-  ares_channel *channel =
-      (ares_channel *)grpc_ares_ev_driver_get_channel(ev_driver);
-  gpr_ref_init(&r->pending_queries, 1);
-  if (grpc_ipv6_loopback_available()) {
-    gpr_ref(&r->pending_queries);
-    ares_gethostbyname(*channel, r->host, AF_INET6, on_done_cb, r);
+  // If there are no pending queries, invoke on_done callback and destroy the
+  // request
+  if (gpr_unref(&r->pending_queries)) {
+    // A new exec_ctx is created here, as the c-ares interface does not provide
+    // one in this callback. It's safe to schedule on_done with the newly
+    // created exec_ctx, since the caller has been warned not to acquire locks
+    // in on_done. ares_dns_resolver is using combiner to protect resources
+    // needed by on_done.
+    grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
+    grpc_exec_ctx_sched(&exec_ctx, r->on_done, r->error, NULL);
+    grpc_exec_ctx_finish(&exec_ctx);
+    gpr_mu_destroy(&r->mu);
+    gpr_free(r->host);
+    gpr_free(r->port);
+    gpr_free(r->default_port);
+    gpr_free(r);
   }
-  ares_gethostbyname(*channel, r->host, AF_INET, on_done_cb, r);
-  grpc_ares_ev_driver_start(exec_ctx, ev_driver);
 }
 
 void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name,
@@ -240,8 +226,15 @@ void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name,
   r->host = host;
   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);
+  ares_channel *channel =
+      (ares_channel *)grpc_ares_ev_driver_get_channel(r->ev_driver);
+  gpr_ref_init(&r->pending_queries, 1);
+  if (grpc_ipv6_loopback_available()) {
+    gpr_ref(&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);
   return;
 
 error_cleanup:
diff --git a/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.h b/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.h
index 465b9af7bd..ffeff86344 100644
--- a/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.h
+++ b/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.h
@@ -40,9 +40,11 @@
 #include "src/core/lib/iomgr/polling_entity.h"
 #include "src/core/lib/iomgr/resolve_address.h"
 
-/* Asynchronously resolve addr. Use default_port if a port isn't designated
-   in addr, otherwise use the port in addr. grpc_ares_init() must be called
-   at least once before this function. */
+/* Asynchronously resolve addr. Use default_port if a port isn't designated in
+   addr, otherwise use the port in addr. grpc_ares_init() must be called at
+   least once before this function. \a on_done may be called directly in this
+   function without being scheduled with \a exec_ctx, it should not try to
+   acquire locks that are being held by the caller. */
 extern void (*grpc_resolve_address_ares)(grpc_exec_ctx *exec_ctx,
                                          const char *addr,
                                          const char *default_port,