Address review comments

doc/environment_variables.md

@@ -68,6 +68,6 @@ some configuration as environment variables that can be set.
 
 * GRPC_DNS_RESOLVER
   Declares which DNS resolver to use. Available DNS resolver include:
-  - ares - a DNS resolver based around the c-ares library
+  - ares (default) - a DNS resolver based around the c-ares library
   - native - a DNS resolver based around getaddrinfo(), creates a new thread to
     perform name resolution

gRPC-Core.podspec

@@ -102,7 +102,7 @@ Pod::Spec.new do |s|
   }
 
   s.default_subspecs = 'Interface', 'Implementation'
-  s.compiler_flags = '-DGRPC_NATIVE_ADDRESS_RESOLVE'
+  s.compiler_flags = '-DGRPC_ARES=0'
 
   # Like many other C libraries, gRPC-Core has its public headers under `include/<libname>/` and its
   # sources and private headers in other directories outside `include/`. Cocoapods' linter doesn't

include/grpc/impl/codegen/port_platform.h

@@ -360,9 +360,13 @@ typedef unsigned __int64 uint64_t;
    power of two */
 #define GPR_MAX_ALIGNMENT 16
 
+/* #define GRPC_ARES 0 */
+
+#ifndef GRPC_ARES
 #ifdef GPR_WINDOWS
-#ifndef GRPC_NATIVE_ADDRESS_RESOLVE
-#define GRPC_NATIVE_ADDRESS_RESOLVE
+#define GRPC_ARES 0
+#else
+#define GRPC_ARES 1
 #endif
 #endif
 

src/core/ext/client_channel/resolver_registry.c

@@ -77,8 +77,12 @@ void grpc_resolver_registry_set_default_prefix(
 void grpc_register_resolver_type(grpc_resolver_factory *factory) {
   int i;
   for (i = 0; i < g_number_of_resolvers; i++) {
-    GPR_ASSERT(0 != strcmp(factory->vtable->scheme,
-                           g_all_of_the_resolvers[i]->vtable->scheme));
+    if (0 == strcmp(factory->vtable->scheme,
+                    g_all_of_the_resolvers[i]->vtable->scheme)) {
+      grpc_resolver_factory_unref(g_all_of_the_resolvers[i]);
+      g_all_of_the_resolvers[i] = factory;
+      return;
+    }
   }
   GPR_ASSERT(g_number_of_resolvers != MAX_RESOLVERS);
   grpc_resolver_factory_ref(factory);

src/core/ext/client_channel/resolver_registry.h

@@ -43,6 +43,7 @@ void grpc_resolver_registry_shutdown(void);
 void grpc_resolver_registry_set_default_prefix(const char *default_prefix);
 
 /** Register a resolver type.
+    \a factory will replace a registered factory if they have the same scheme.
     URI's of \a scheme will be resolved with the given resolver.
     If \a priority is greater than zero, then the resolver will be eligible
     to resolve names that are passed in with no scheme. Higher priority

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

@@ -32,6 +32,8 @@
  */
 
 #include <grpc/support/port_platform.h>
+#if GRPC_ARES == 1
+
 #include <string.h>
 
 #include <grpc/support/alloc.h>
@@ -277,7 +279,7 @@ static void dns_ares_start_resolving_locked(grpc_exec_ctx *exec_ctx,
   GPR_ASSERT(!r->resolving);
   r->resolving = true;
   r->addresses = NULL;
-  grpc_resolve_address_ares(
+  grpc_resolve_address(
       exec_ctx, r->name_to_resolve, r->default_port, r->base.pollset_set,
       grpc_closure_create(dns_ares_on_resolved, r), &r->addresses);
 }
@@ -299,7 +301,6 @@ 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_combiner_destroy(exec_ctx, r->combiner);
-  grpc_ares_cleanup();
   if (r->resolved_result != NULL) {
     grpc_channel_args_destroy(r->resolved_result);
   }
@@ -311,29 +312,18 @@ static void dns_ares_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) {
 
 static grpc_resolver *dns_ares_create(grpc_resolver_args *args,
                                       const char *default_port) {
-  ares_dns_resolver *r;
-  grpc_error *error = GRPC_ERROR_NONE;
-  char *proxy_name;
   // Get name from args.
   const char *path = args->uri->path;
-
   if (0 != strcmp(args->uri->authority, "")) {
     gpr_log(GPR_ERROR, "authority based dns uri's not supported");
     return NULL;
   }
-
-  error = grpc_ares_init();
-  if (error != GRPC_ERROR_NONE) {
-    GRPC_LOG_IF_ERROR("ares_library_init() failed", error);
-    return NULL;
-  }
-
   if (path[0] == '/') ++path;
 
   // Get proxy name, if any.
-  proxy_name = grpc_get_http_proxy_server();
+  char *proxy_name = grpc_get_http_proxy_server();
   // Create resolver.
-  r = gpr_malloc(sizeof(ares_dns_resolver));
+  ares_dns_resolver *r = gpr_malloc(sizeof(ares_dns_resolver));
   memset(r, 0, sizeof(*r));
   grpc_resolver_init(&r->base, &dns_ares_resolver_vtable);
   r->combiner = grpc_combiner_create(NULL);
@@ -389,9 +379,24 @@ static grpc_resolver_factory *dns_ares_resolver_factory_create() {
 void grpc_resolver_dns_ares_init(void) {
   char *resolver = gpr_getenv("GRPC_DNS_RESOLVER");
   if (resolver == NULL || gpr_stricmp(resolver, "ares") == 0) {
+    grpc_error *error = grpc_ares_init();
+    if (error != GRPC_ERROR_NONE) {
+      GRPC_LOG_IF_ERROR("ares_library_init() failed", error);
+      return;
+    }
+    grpc_resolve_address = grpc_resolve_address_ares;
     grpc_register_resolver_type(dns_ares_resolver_factory_create());
   }
   gpr_free(resolver);
 }
 
+void grpc_resolver_dns_ares_shutdown(void) { grpc_ares_cleanup(); }
+
+#else /* GRPC_ARES == 1 */
+#include <grpc/support/log.h>
+
+void grpc_resolver_dns_ares_init(void) {}
+
 void grpc_resolver_dns_ares_shutdown(void) {}
+
+#endif /* GRPC_ARES == 1 */

src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver.h

@@ -34,6 +34,8 @@
 #ifndef GRPC_CORE_EXT_RESOLVER_DNS_C_ARES_GRPC_ARES_EV_DRIVER_H
 #define GRPC_CORE_EXT_RESOLVER_DNS_C_ARES_GRPC_ARES_EV_DRIVER_H
 
+#include <ares.h>
+
 #include "src/core/lib/iomgr/exec_ctx.h"
 #include "src/core/lib/iomgr/pollset_set.h"
 
@@ -48,7 +50,7 @@ void grpc_ares_ev_driver_start(grpc_exec_ctx *exec_ctx,
 /* Returns the ares_channel owned by \a ev_driver. To bind a c-ares query to
    \a ev_driver, use the ares_channel owned by \a ev_driver as the arg of the
    query. */
-void *grpc_ares_ev_driver_get_channel(grpc_ares_ev_driver *ev_driver);
+ares_channel *grpc_ares_ev_driver_get_channel(grpc_ares_ev_driver *ev_driver);
 
 /* Creates a new grpc_ares_ev_driver. Returns GRPC_ERROR_NONE if \a ev_driver is
    created successfully. */

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

@@ -32,11 +32,10 @@
  */
 #include <grpc/support/port_platform.h>
 #include "src/core/lib/iomgr/port.h"
-#if !defined(GRPC_NATIVE_ADDRESS_RESOLVE) && defined(GRPC_POSIX_SOCKET)
+#if GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET)
 
 #include "src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver.h"
 
-#include <ares.h>
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
 #include <grpc/support/string_util.h>
@@ -236,7 +235,7 @@ static void on_writable_cb(grpc_exec_ctx *exec_ctx, void *arg,
   grpc_ares_ev_driver_unref(ev_driver);
 }
 
-void *grpc_ares_ev_driver_get_channel(grpc_ares_ev_driver *ev_driver) {
+ares_channel *grpc_ares_ev_driver_get_channel(grpc_ares_ev_driver *ev_driver) {
   return &ev_driver->channel;
 }
 
@@ -327,4 +326,4 @@ void grpc_ares_ev_driver_start(grpc_exec_ctx *exec_ctx,
   grpc_ares_ev_driver_unref(ev_driver);
 }
 
-#endif /* !GRPC_NATIVE_ADDRESS_RESOLVE && GRPC_POSIX_SOCKET */
+#endif /* GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET) */

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

@@ -32,7 +32,7 @@
  */
 
 #include <grpc/support/port_platform.h>
-#ifndef GRPC_NATIVE_ADDRESS_RESOLVE
+#if GRPC_ARES == 1
 
 #include "src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.h"
 #include "src/core/lib/iomgr/sockaddr.h"
@@ -244,8 +244,7 @@ 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;
-  ares_channel *channel =
-      (ares_channel *)grpc_ares_ev_driver_get_channel(r->ev_driver);
+  ares_channel *channel = grpc_ares_ev_driver_get_channel(r->ev_driver);
   // An extra reference is put here to avoid destroying the request in
   // on_done_cb before calling grpc_ares_ev_driver_start.
   gpr_ref_init(&r->pending_queries, 2);
@@ -292,4 +291,4 @@ void grpc_ares_cleanup(void) {
   gpr_mu_unlock(&g_init_mu);
 }
 
-#endif /* GRPC_NATIVE_ADDRESS_RESOLVE */
+#endif /* GRPC_ARES == 1 */

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

@@ -34,14 +34,13 @@
 #ifndef GRPC_CORE_EXT_RESOLVER_DNS_C_ARES_GRPC_ARES_WRAPPER_H
 #define GRPC_CORE_EXT_RESOLVER_DNS_C_ARES_GRPC_ARES_WRAPPER_H
 
-#include "src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver.h"
 #include "src/core/lib/iomgr/exec_ctx.h"
 #include "src/core/lib/iomgr/iomgr.h"
 #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
+/* Asynchronously resolve addr. Use \a 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 must not try to acquire
    locks that are being held by the caller. */

src/core/ext/resolver/dns/native/dns_resolver.c

@@ -310,6 +310,15 @@ void grpc_resolver_dns_native_init(void) {
   if (resolver != NULL && gpr_stricmp(resolver, "native") == 0) {
     gpr_log(GPR_DEBUG, "Using native dns resolver");
     grpc_register_resolver_type(dns_resolver_factory_create());
+  } else {
+    grpc_resolver_factory *existing_factory =
+        grpc_resolver_factory_lookup("dns");
+    if (existing_factory == NULL) {
+      gpr_log(GPR_DEBUG, "Using native dns resolver");
+      grpc_register_resolver_type(dns_resolver_factory_create());
+    } else {
+      grpc_resolver_factory_unref(existing_factory);
+    }
   }
   gpr_free(resolver);
 }

templates/gRPC-Core.podspec.template

@@ -129,7 +129,7 @@
     }
 
     s.default_subspecs = 'Interface', 'Implementation'
-    s.compiler_flags = '-DGRPC_NATIVE_ADDRESS_RESOLVE'
+    s.compiler_flags = '-DGRPC_ARES=0'
 
     # Like many other C libraries, gRPC-Core has its public headers under `include/<libname>/` and its
     # sources and private headers in other directories outside `include/`. Cocoapods' linter doesn't

test/core/client_channel/resolvers/dns_resolver_connectivity_test.c

@@ -45,24 +45,46 @@
 static gpr_mu g_mu;
 static bool g_fail_resolution = true;
 
-static int my_resolve_address(const char *name, const char *addr,
-                              grpc_resolved_addresses **addrs,
-                              grpc_error **error) {
+// static int my_resolve_address(const char *name, const char *addr,
+//                               grpc_resolved_addresses **addrs,
+//                               grpc_error **error) {
+//   gpr_mu_lock(&g_mu);
+//   GPR_ASSERT(0 == strcmp("test", name));
+//   if (g_fail_resolution) {
+//     g_fail_resolution = false;
+//     gpr_mu_unlock(&g_mu);
+//     *error = GRPC_ERROR_CREATE("Forced Failure");
+//   } else {
+//     gpr_mu_unlock(&g_mu);
+//     *addrs = gpr_malloc(sizeof(**addrs));
+//     (*addrs)->naddrs = 1;
+//     (*addrs)->addrs = gpr_malloc(sizeof(*(*addrs)->addrs));
+//     (*addrs)->addrs[0].len = 123;
+//     *error = GRPC_ERROR_NONE;
+//   }
+//   return 1;
+// }
+
+static void my_resolve_address(grpc_exec_ctx *exec_ctx, const char *addr,
+                               const char *default_port,
+                               grpc_pollset_set *interested_parties,
+                               grpc_closure *on_done,
+                               grpc_resolved_addresses **addrs) {
   gpr_mu_lock(&g_mu);
-  GPR_ASSERT(0 == strcmp("test", name));
+  GPR_ASSERT(0 == strcmp("test", addr));
+  grpc_error *error = GRPC_ERROR_NONE;
   if (g_fail_resolution) {
     g_fail_resolution = false;
     gpr_mu_unlock(&g_mu);
-    *error = GRPC_ERROR_CREATE("Forced Failure");
+    error = GRPC_ERROR_CREATE("Forced Failure");
   } else {
     gpr_mu_unlock(&g_mu);
     *addrs = gpr_malloc(sizeof(**addrs));
     (*addrs)->naddrs = 1;
     (*addrs)->addrs = gpr_malloc(sizeof(*(*addrs)->addrs));
     (*addrs)->addrs[0].len = 123;
-    *error = GRPC_ERROR_NONE;
   }
-  return 1;
+  grpc_exec_ctx_sched(exec_ctx, on_done, error, NULL);
 }
 
 static grpc_resolver *create_resolver(const char *name) {
@@ -102,7 +124,7 @@ int main(int argc, char **argv) {
 
   grpc_init();
   gpr_mu_init(&g_mu);
-  grpc_customized_resolve_address = my_resolve_address;
+  grpc_resolve_address = my_resolve_address;
 
   grpc_resolver *resolver = create_resolver("dns:test");
 

test/core/end2end/fuzzers/api_fuzzer.c

@@ -39,7 +39,6 @@
 #include <grpc/support/log.h>
 #include <grpc/support/string_util.h>
 
-#include "src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.h"
 #include "src/core/ext/transport/chttp2/transport/chttp2_transport.h"
 #include "src/core/lib/channel/channel_args.h"
 #include "src/core/lib/iomgr/resolve_address.h"
@@ -362,7 +361,9 @@ static void finish_resolve(grpc_exec_ctx *exec_ctx, void *arg,
 }
 
 void my_resolve_address(grpc_exec_ctx *exec_ctx, const char *addr,
-                        const char *default_port, grpc_closure *on_done,
+                        const char *default_port,
+                        grpc_pollset_set *interested_parties,
+                        grpc_closure *on_done,
                         grpc_resolved_addresses **addresses) {
   addr_req *r = gpr_malloc(sizeof(*r));
   r->addr = gpr_strdup(addr);
@@ -374,14 +375,6 @@ void my_resolve_address(grpc_exec_ctx *exec_ctx, const char *addr,
                   finish_resolve, r, gpr_now(GPR_CLOCK_MONOTONIC));
 }
 
-void my_resolve_address_async(grpc_exec_ctx *exec_ctx, const char *addr,
-                              const char *default_port,
-                              grpc_pollset_set *interested_parties,
-                              grpc_closure *on_done,
-                              grpc_resolved_addresses **addresses) {
-  my_resolve_address(exec_ctx, addr, default_port, on_done, addresses);
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // client connection
 
@@ -664,8 +657,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
   grpc_test_only_set_metadata_hash_seed(0);
   if (squelch) gpr_set_log_function(dont_log);
   input_stream inp = {data, data + size};
-  grpc_resolve_address = my_resolve_address_async;
-  grpc_resolve_address_ares = my_resolve_address_async;
+  grpc_resolve_address = my_resolve_address;
   grpc_tcp_client_connect_impl = my_tcp_client_connect;
   gpr_now_impl = now_impl;
   grpc_init();

test/core/end2end/goaway_server_test.c

@@ -46,6 +46,11 @@ static void *tag(intptr_t i) { return (void *)i; }
 
 static gpr_mu g_mu;
 static int g_resolve_port = -1;
+static void (*iomgr_resolve_address)(grpc_exec_ctx *exec_ctx, const char *addr,
+                                     const char *default_port,
+                                     grpc_pollset_set *interested_parties,
+                                     grpc_closure *on_done,
+                                     grpc_resolved_addresses **addresses);
 
 static void set_resolve_port(int port) {
   gpr_mu_lock(&g_mu);
@@ -53,17 +58,22 @@ static void set_resolve_port(int port) {
   gpr_mu_unlock(&g_mu);
 }
 
-static int my_resolve_address(const char *name, const char *addr,
-                              grpc_resolved_addresses **addrs,
-                              grpc_error **error) {
-  if (0 != strcmp(name, "test")) {
-    return 0;
+static void my_resolve_address(grpc_exec_ctx *exec_ctx, const char *addr,
+                               const char *default_port,
+                               grpc_pollset_set *interested_parties,
+                               grpc_closure *on_done,
+                               grpc_resolved_addresses **addrs) {
+  if (0 != strcmp(addr, "test")) {
+    iomgr_resolve_address(exec_ctx, addr, default_port, interested_parties,
+                          on_done, addrs);
+    return;
   }
 
+  grpc_error *error = GRPC_ERROR_NONE;
   gpr_mu_lock(&g_mu);
   if (g_resolve_port < 0) {
     gpr_mu_unlock(&g_mu);
-    *error = GRPC_ERROR_CREATE("Forced Failure");
+    error = GRPC_ERROR_CREATE("Forced Failure");
   } else {
     *addrs = gpr_malloc(sizeof(**addrs));
     (*addrs)->naddrs = 1;
@@ -75,9 +85,8 @@ static int my_resolve_address(const char *name, const char *addr,
     sa->sin_port = htons((uint16_t)g_resolve_port);
     (*addrs)->addrs[0].len = sizeof(*sa);
     gpr_mu_unlock(&g_mu);
-    *error = GRPC_ERROR_NONE;
   }
-  return 1;
+  grpc_exec_ctx_sched(exec_ctx, on_done, error, NULL);
 }
 
 int main(int argc, char **argv) {
@@ -89,8 +98,9 @@ int main(int argc, char **argv) {
   grpc_test_init(argc, argv);
 
   gpr_mu_init(&g_mu);
-  grpc_customized_resolve_address = my_resolve_address;
   grpc_init();
+  iomgr_resolve_address = grpc_resolve_address;
+  grpc_resolve_address = my_resolve_address;
 
   int was_cancelled1;
   int was_cancelled2;