From 3483cf586efb4321379aea3a0110796776ea091a Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Thu, 10 Nov 2016 23:09:00 -0800 Subject: Address review comments --- doc/environment_variables.md | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'doc/environment_variables.md') 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 -- cgit v1.2.3 From 9e4c8eb8e84123d275bc72f048c96cb1f8ce5acc Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Fri, 18 Nov 2016 01:06:57 -0800 Subject: Address review comments --- doc/environment_variables.md | 2 +- gRPC-Core.podspec | 2 +- include/grpc/impl/codegen/port_platform.h | 8 +++-- src/core/ext/client_channel/resolver_registry.c | 8 +++-- src/core/ext/client_channel/resolver_registry.h | 1 + .../ext/resolver/dns/c_ares/dns_resolver_ares.c | 35 +++++++++++--------- .../ext/resolver/dns/c_ares/grpc_ares_ev_driver.h | 4 ++- .../dns/c_ares/grpc_ares_ev_driver_posix.c | 7 ++-- .../ext/resolver/dns/c_ares/grpc_ares_wrapper.c | 7 ++-- .../ext/resolver/dns/c_ares/grpc_ares_wrapper.h | 5 ++- src/core/ext/resolver/dns/native/dns_resolver.c | 9 +++++ templates/gRPC-Core.podspec.template | 2 +- .../resolvers/dns_resolver_connectivity_test.c | 38 +++++++++++++++++----- test/core/end2end/fuzzers/api_fuzzer.c | 16 +++------ test/core/end2end/goaway_server_test.c | 28 +++++++++++----- 15 files changed, 109 insertions(+), 63 deletions(-) (limited to 'doc/environment_variables.md') diff --git a/doc/environment_variables.md b/doc/environment_variables.md index 11ac890cd5..6c13015baa 100644 --- a/doc/environment_variables.md +++ b/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 diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index f261481fd4..670df36f1d 100644 --- a/gRPC-Core.podspec +++ b/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//` and its # sources and private headers in other directories outside `include/`. Cocoapods' linter doesn't diff --git a/include/grpc/impl/codegen/port_platform.h b/include/grpc/impl/codegen/port_platform.h index 29b0cb80c4..1db69b08e2 100644 --- a/include/grpc/impl/codegen/port_platform.h +++ b/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 diff --git a/src/core/ext/client_channel/resolver_registry.c b/src/core/ext/client_channel/resolver_registry.c index 94942069a1..dc8be78764 100644 --- a/src/core/ext/client_channel/resolver_registry.c +++ b/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); diff --git a/src/core/ext/client_channel/resolver_registry.h b/src/core/ext/client_channel/resolver_registry.h index 2a95a669f0..c216ae1466 100644 --- a/src/core/ext/client_channel/resolver_registry.h +++ b/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 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 e5691942a4..c0c1efbb86 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 @@ -32,6 +32,8 @@ */ #include +#if GRPC_ARES == 1 + #include #include @@ -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 + +void grpc_resolver_dns_ares_init(void) {} + void grpc_resolver_dns_ares_shutdown(void) {} + +#endif /* GRPC_ARES == 1 */ diff --git a/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver.h b/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver.h index 7165df0afc..334feaa2ab 100644 --- a/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver.h +++ b/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 + #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. */ 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 a4733dcb4b..68c52e43f0 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 @@ -32,11 +32,10 @@ */ #include #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 #include #include #include @@ -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) */ 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 f90222b2e6..c8323e740a 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 @@ -32,7 +32,7 @@ */ #include -#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 */ 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 3968a445ab..ab00a26b36 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 @@ -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. */ diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index 7cbd6ce95c..a3b4d5b9af 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/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); } diff --git a/templates/gRPC-Core.podspec.template b/templates/gRPC-Core.podspec.template index d52b53dd97..029e2c33cb 100644 --- a/templates/gRPC-Core.podspec.template +++ b/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//` and its # sources and private headers in other directories outside `include/`. Cocoapods' linter doesn't diff --git a/test/core/client_channel/resolvers/dns_resolver_connectivity_test.c b/test/core/client_channel/resolvers/dns_resolver_connectivity_test.c index 57dfb5cb12..dd7fa89d69 100644 --- a/test/core/client_channel/resolvers/dns_resolver_connectivity_test.c +++ b/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"); diff --git a/test/core/end2end/fuzzers/api_fuzzer.c b/test/core/end2end/fuzzers/api_fuzzer.c index 531c3980b5..746134c85b 100644 --- a/test/core/end2end/fuzzers/api_fuzzer.c +++ b/test/core/end2end/fuzzers/api_fuzzer.c @@ -39,7 +39,6 @@ #include #include -#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(); diff --git a/test/core/end2end/goaway_server_test.c b/test/core/end2end/goaway_server_test.c index 7f9998f36f..9d9092aa0c 100644 --- a/test/core/end2end/goaway_server_test.c +++ b/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; -- cgit v1.2.3 From f47c9635f0ea406b85f9b0d11549c55baf4c583a Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Fri, 18 Nov 2016 20:41:20 -0800 Subject: Address reivew comments --- doc/environment_variables.md | 4 +++- src/core/ext/resolver/dns/c_ares/dns_resolver_ares.c | 1 - .../ext/resolver/dns/c_ares/grpc_ares_ev_driver_posix.c | 16 ++++------------ src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.c | 14 ++++++-------- 4 files changed, 13 insertions(+), 22 deletions(-) (limited to 'doc/environment_variables.md') diff --git a/doc/environment_variables.md b/doc/environment_variables.md index 6c13015baa..9222cc305c 100644 --- a/doc/environment_variables.md +++ b/doc/environment_variables.md @@ -67,7 +67,9 @@ some configuration as environment variables that can be set. - ERROR - log only errors * GRPC_DNS_RESOLVER - Declares which DNS resolver to use. Available DNS resolver include: + Declares which DNS resolver to use. The default is ares if gRPC is built with + c-ares support. Otherwise, the value of this environment variable is ignored. + Available DNS resolver include: - 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 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 c0c1efbb86..01ed011439 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 @@ -393,7 +393,6 @@ void grpc_resolver_dns_ares_init(void) { void grpc_resolver_dns_ares_shutdown(void) { grpc_ares_cleanup(); } #else /* GRPC_ARES == 1 */ -#include void grpc_resolver_dns_ares_init(void) {} 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 68c52e43f0..301f52d62f 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 @@ -107,7 +107,6 @@ static void grpc_ares_ev_driver_unref(grpc_ares_ev_driver *ev_driver) { gpr_mu_destroy(&ev_driver->mu); ares_destroy(ev_driver->channel); gpr_free(ev_driver); - grpc_ares_cleanup(); } } @@ -125,10 +124,7 @@ static void fd_node_destroy(grpc_exec_ctx *exec_ctx, fd_node *fdn) { grpc_error *grpc_ares_ev_driver_create(grpc_ares_ev_driver **ev_driver, grpc_pollset_set *pollset_set) { int status; - grpc_error *err = grpc_ares_init(); - if (err != GRPC_ERROR_NONE) { - return err; - } + grpc_error *err = GRPC_ERROR_NONE; *ev_driver = gpr_malloc(sizeof(grpc_ares_ev_driver)); status = ares_init(&(*ev_driver)->channel); gpr_log(GPR_DEBUG, "grpc_ares_ev_driver_create"); @@ -150,13 +146,12 @@ grpc_error *grpc_ares_ev_driver_create(grpc_ares_ev_driver **ev_driver, return GRPC_ERROR_NONE; } -void grpc_ares_ev_driver_destroy( // grpc_exec_ctx *exec_ctx, - grpc_ares_ev_driver *ev_driver) { +void grpc_ares_ev_driver_destroy(grpc_ares_ev_driver *ev_driver) { // It's not safe to shut down remaining fds here directly, becauses // ares_host_callback does not provide an exec_ctx. We mark the event driver // as being shut down. If the event driver is working, // grpc_ares_notify_on_event_locked will shut down the fds; if it's not - // working, grpc_ares_ev_driver_unref will release it directly. + // working, there are no fds to shut down. gpr_mu_lock(&ev_driver->mu); ev_driver->shutting_down = true; gpr_mu_unlock(&ev_driver->mu); @@ -283,8 +278,7 @@ static void grpc_ares_notify_on_event_locked(grpc_exec_ctx *exec_ctx, fdn->readable_registered = true; } // Register write_closure if the socket is writable and write_closure - // has - // not been registered with this socket. + // has not been registered with this socket. if (ARES_GETSOCK_WRITABLE(socks_bitmask, i) && !fdn->writable_registered) { gpr_log(GPR_DEBUG, "notify write on: %d", @@ -316,14 +310,12 @@ static void grpc_ares_notify_on_event_locked(grpc_exec_ctx *exec_ctx, void grpc_ares_ev_driver_start(grpc_exec_ctx *exec_ctx, grpc_ares_ev_driver *ev_driver) { - grpc_ares_ev_driver_ref(ev_driver); gpr_mu_lock(&ev_driver->mu); if (!ev_driver->working) { ev_driver->working = true; grpc_ares_notify_on_event_locked(exec_ctx, ev_driver); } gpr_mu_unlock(&ev_driver->mu); - grpc_ares_ev_driver_unref(ev_driver); } #endif /* GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET) */ 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 b63b7ebfe3..c210c40f09 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 @@ -196,7 +196,6 @@ static void on_done_cb(void *arg, int status, int timeouts, void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name, const char *default_port, - // grpc_ares_ev_driver *ev_driver, grpc_pollset_set *interested_parties, grpc_closure *on_done, grpc_resolved_addresses **addrs) { @@ -204,16 +203,16 @@ void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name, char *host; char *port; gpr_split_host_port(name, &host, &port); - grpc_error *err = GRPC_ERROR_NONE; if (host == NULL) { - err = grpc_error_set_str(GRPC_ERROR_CREATE("unparseable host:port"), - GRPC_ERROR_STR_TARGET_ADDRESS, name); + grpc_error *err = + grpc_error_set_str(GRPC_ERROR_CREATE("unparseable host:port"), + GRPC_ERROR_STR_TARGET_ADDRESS, name); grpc_exec_ctx_sched(exec_ctx, on_done, err, NULL); goto error_cleanup; } else if (port == NULL) { if (default_port == NULL) { - err = grpc_error_set_str(GRPC_ERROR_CREATE("no port in name"), - GRPC_ERROR_STR_TARGET_ADDRESS, name); + grpc_error *err = grpc_error_set_str(GRPC_ERROR_CREATE("no port in name"), + GRPC_ERROR_STR_TARGET_ADDRESS, name); grpc_exec_ctx_sched(exec_ctx, on_done, err, NULL); goto error_cleanup; } @@ -221,7 +220,7 @@ void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name, } grpc_ares_ev_driver *ev_driver; - err = grpc_ares_ev_driver_create(&ev_driver, interested_parties); + grpc_error *err = grpc_ares_ev_driver_create(&ev_driver, interested_parties); if (err != GRPC_ERROR_NONE) { GRPC_LOG_IF_ERROR("grpc_ares_ev_driver_create() failed", err); goto error_cleanup; @@ -258,7 +257,6 @@ error_cleanup: void (*grpc_resolve_address_ares)( grpc_exec_ctx *exec_ctx, const char *name, const char *default_port, grpc_pollset_set *interested_parties, grpc_closure *on_done, - // grpc_ares_ev_driver *ev_driver, grpc_closure *on_done, grpc_resolved_addresses **addrs) = grpc_resolve_address_ares_impl; grpc_error *grpc_ares_init(void) { -- cgit v1.2.3 From 15618625ed389405aa6afb717721ed6f84452bdd Mon Sep 17 00:00:00 2001 From: Yuchen Zeng Date: Sun, 19 Mar 2017 22:31:14 -0700 Subject: Turn off c_ares resolver by default --- doc/environment_variables.md | 4 +- .../ext/resolver/dns/c_ares/dns_resolver_ares.c | 4 +- .../dns/c_ares/grpc_ares_ev_driver_fallback.c | 58 --------------------- .../ext/resolver/dns/c_ares/grpc_ares_wrapper.c | 2 +- .../dns/c_ares/grpc_ares_wrapper_fallback.c | 60 ---------------------- src/core/lib/iomgr/exec_ctx.c | 1 - src/core/lib/iomgr/tcp_client_posix.c | 21 -------- tools/run_tests/run_tests_matrix.py | 19 +++---- 8 files changed, 16 insertions(+), 153 deletions(-) delete mode 100644 src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_fallback.c delete mode 100644 src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c (limited to 'doc/environment_variables.md') diff --git a/doc/environment_variables.md b/doc/environment_variables.md index 645ab46ec1..513936d660 100644 --- a/doc/environment_variables.md +++ b/doc/environment_variables.md @@ -74,6 +74,6 @@ some configuration as environment variables that can be set. Declares which DNS resolver to use. The default is ares if gRPC is built with c-ares support. Otherwise, the value of this environment variable is ignored. Available DNS resolver include: - - ares (default) - a DNS resolver based around the c-ares library - - native - a DNS resolver based around getaddrinfo(), creates a new thread to + - native (default)- a DNS resolver based around getaddrinfo(), creates a new thread to perform name resolution + - ares - a DNS resolver based around the c-ares library 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 4ad40f8615..2399b9b80c 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 @@ -369,7 +369,9 @@ 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) { + /* TODO(zyc): Turn on c-ares based resolver by default after the address + sorter is added. */ + 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); diff --git a/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_fallback.c b/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_fallback.c deleted file mode 100644 index 5f2ebd60cb..0000000000 --- a/src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver_fallback.c +++ /dev/null @@ -1,58 +0,0 @@ -/* - * - * Copyright 2016, Google Inc. - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are - * met: - * - * * Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * * Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following disclaimer - * in the documentation and/or other materials provided with the - * distribution. - * * Neither the name of Google Inc. nor the names of its - * contributors may be used to endorse or promote products derived from - * this software without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT - * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT - * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - * - */ - -#include -#include "src/core/lib/iomgr/port.h" -#if !(GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET)) - -#include "src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver.h" - -struct grpc_ares_ev_driver { - char unused_member; // unused member to prevent undefined behaviour -}; - -void grpc_ares_ev_driver_start(grpc_exec_ctx *exec_ctx, - grpc_ares_ev_driver *ev_driver) {} - -ares_channel *grpc_ares_ev_driver_get_channel(grpc_ares_ev_driver *ev_driver) { - return NULL; -} - -grpc_error *grpc_ares_ev_driver_create(grpc_ares_ev_driver **ev_driver, - grpc_pollset_set *pollset_set) { - return GRPC_ERROR_NONE; -} - -void grpc_ares_ev_driver_destroy(grpc_ares_ev_driver *ev_driver) {} - -#endif /* !(GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET)) */ 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 7a104665c1..4c5786dfb4 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 @@ -98,7 +98,7 @@ static void grpc_ares_request_unref(grpc_exec_ctx *exec_ctx, /* If there are no pending queries, invoke on_done callback and destroy the request */ if (gpr_unref(&r->pending_queries)) { - /* TODO(zyc): Sort results with RPC6724 before invoking on_done. */ + /* TODO(zyc): Sort results with RFC6724 before invoking on_done. */ if (exec_ctx == NULL) { /* A new exec_ctx is created here, as the c-ares interface does not provide one in ares_host_callback. It's safe to schedule on_done with diff --git a/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c b/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c deleted file mode 100644 index 8414de472b..0000000000 --- a/src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper_fallback.c +++ /dev/null @@ -1,60 +0,0 @@ -/* - * - * Copyright 2016, Google Inc. - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are - * met: - * - * * Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * * Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following disclaimer - * in the documentation and/or other materials provided with the - * distribution. - * * Neither the name of Google Inc. nor the names of its - * contributors may be used to endorse or promote products derived from - * this software without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT - * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT - * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - * - */ - -/* TODO(zyc): remove this fallback after we can build c-ares on windows */ - -#include -#if !(GRPC_ARES == 1) - -#include "src/core/ext/resolver/dns/c_ares/grpc_ares_ev_driver.h" -#include "src/core/ext/resolver/dns/c_ares/grpc_ares_wrapper.h" - -void grpc_resolve_address_ares_impl(grpc_exec_ctx *exec_ctx, const char *name, - const char *default_port, - grpc_pollset_set *interested_parties, - grpc_closure *on_done, - grpc_resolved_addresses **addrs) { - grpc_resolve_address(exec_ctx, name, default_port, interested_parties, - on_done, addrs); -} - -void (*grpc_resolve_address_ares)( - grpc_exec_ctx *exec_ctx, const char *name, const char *default_port, - grpc_pollset_set *interested_parties, grpc_closure *on_done, - grpc_resolved_addresses **addrs) = grpc_resolve_address_ares_impl; - -grpc_error *grpc_ares_init(void) { return GRPC_ERROR_NONE; } - -void grpc_ares_cleanup(void) {} - -#endif /* GRPC_NATIVE_ADDRESS_RESOLVE */ diff --git a/src/core/lib/iomgr/exec_ctx.c b/src/core/lib/iomgr/exec_ctx.c index 39ab396268..83bb436bd0 100644 --- a/src/core/lib/iomgr/exec_ctx.c +++ b/src/core/lib/iomgr/exec_ctx.c @@ -64,7 +64,6 @@ bool grpc_always_ready_to_finish(grpc_exec_ctx *exec_ctx, void *arg_ignored) { bool grpc_exec_ctx_flush(grpc_exec_ctx *exec_ctx) { bool did_something = 0; - gpr_log(GPR_DEBUG, "grpc_exec_ctx_flush"); GPR_TIMER_BEGIN("grpc_exec_ctx_flush", 0); for (;;) { if (!grpc_closure_list_empty(exec_ctx->closure_list)) { diff --git a/src/core/lib/iomgr/tcp_client_posix.c b/src/core/lib/iomgr/tcp_client_posix.c index daa4eb51ee..0144192b71 100644 --- a/src/core/lib/iomgr/tcp_client_posix.c +++ b/src/core/lib/iomgr/tcp_client_posix.c @@ -35,9 +35,6 @@ #ifdef GRPC_POSIX_SOCKET -#include "src/core/lib/iomgr/sockaddr.h" -#include "src/core/lib/iomgr/socket_utils_posix.h" - #include "src/core/lib/iomgr/tcp_client_posix.h" #include @@ -290,24 +287,6 @@ static void tcp_client_connect_impl(grpc_exec_ctx *exec_ctx, *ep = NULL; - struct sockaddr_in *addr4 = (struct sockaddr_in *)addr->addr; - if (addr4->sin_family == AF_INET) { - char output[INET_ADDRSTRLEN]; - inet_ntop(AF_INET, &addr4->sin_addr, output, INET_ADDRSTRLEN); - gpr_log(GPR_DEBUG, - "native resolver gets a AF_INET result: \n" - " addr: %s\n", - output); - } else { - struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)addr->addr; - char output[INET6_ADDRSTRLEN]; - inet_ntop(AF_INET6, &addr6->sin6_addr, output, INET6_ADDRSTRLEN); - gpr_log(GPR_DEBUG, - "native resolver gets a AF_INET6 result: \n" - " addr: %s\n, sin6_scope_id: %d\n", - output, addr6->sin6_scope_id); - } - /* Use dualstack sockets where available. */ if (grpc_sockaddr_to_v4mapped(addr, &addr6_v4mapped)) { addr = &addr6_v4mapped; diff --git a/tools/run_tests/run_tests_matrix.py b/tools/run_tests/run_tests_matrix.py index d2de125ffb..6c1d4bd15d 100755 --- a/tools/run_tests/run_tests_matrix.py +++ b/tools/run_tests/run_tests_matrix.py @@ -221,19 +221,20 @@ def _create_portability_test_jobs(extra_args=[], inner_jobs=_DEFAULT_INNER_JOBS) extra_args=extra_args, inner_jobs=inner_jobs) - # C and C++ with the native DNS resolver on Linux + # C and C++ with the c-ares DNS resolver on Linux test_jobs += _generate_jobs(languages=['c', 'c++'], configs=['dbg'], platforms=['linux'], labels=['portability'], extra_args=extra_args, - extra_envs={'GRPC_DNS_RESOLVER': 'native'}) - - # C with the native DNS resolver on Windonws - test_jobs += _generate_jobs(languages=['c'], - configs=['dbg'], platforms=['windows'], - labels=['portability'], - extra_args=extra_args, - extra_envs={'GRPC_DNS_RESOLVER': 'native'}) + extra_envs={'GRPC_DNS_RESOLVER': 'ares'}) + + # TODO(zyc): Turn on this test after adding c-ares support on windows. + # C with the c-ares DNS resolver on Windonws + # test_jobs += _generate_jobs(languages=['c'], + # configs=['dbg'], platforms=['windows'], + # labels=['portability'], + # extra_args=extra_args, + # extra_envs={'GRPC_DNS_RESOLVER': 'ares'}) # cmake build for C and C++ # TODO(jtattermusch): some of the tests are failing, so we force --build_only -- cgit v1.2.3