diff options
7 files changed, 70 insertions, 43 deletions
diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc index c2a4d42203..01796ca08f 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc @@ -201,7 +201,7 @@ void AresDnsResolver::ShutdownLocked() { grpc_timer_cancel(&next_resolution_timer_); } if (pending_request_ != nullptr) { - grpc_cancel_ares_request(pending_request_); + grpc_cancel_ares_request_locked(pending_request_); } if (next_completion_ != nullptr) { *target_result_ = nullptr; @@ -298,6 +298,7 @@ void AresDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) { grpc_channel_args* result = nullptr; GPR_ASSERT(r->resolving_); r->resolving_ = false; + gpr_free(r->pending_request_); r->pending_request_ = nullptr; if (r->lb_addresses_ != nullptr) { static const char* args_to_remove[2]; diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc index 4c795c34c8..582e2203fc 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc @@ -144,12 +144,12 @@ static void grpc_ares_request_unref_locked(grpc_ares_request* r) { void grpc_ares_complete_request_locked(grpc_ares_request* r) { /* Invoke on_done callback and destroy the request */ + r->ev_driver = nullptr; grpc_lb_addresses* lb_addrs = *(r->lb_addrs_out); if (lb_addrs != nullptr) { grpc_cares_wrapper_address_sorting_sort(lb_addrs); } GRPC_CLOSURE_SCHED(r->on_done, r->error); - gpr_free(r); } static grpc_ares_hostbyname_request* create_hostbyname_request_locked( @@ -356,15 +356,12 @@ done: grpc_ares_request_unref_locked(r); } -static grpc_ares_request* -grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( - const char* dns_server, const char* name, const char* default_port, - grpc_pollset_set* interested_parties, grpc_closure* on_done, - grpc_lb_addresses** addrs, bool check_grpclb, char** service_config_json, - grpc_combiner* combiner) { +void grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( + grpc_ares_request* r, const char* dns_server, const char* name, + const char* default_port, grpc_pollset_set* interested_parties, + bool check_grpclb, grpc_combiner* combiner) { grpc_error* error = GRPC_ERROR_NONE; grpc_ares_hostbyname_request* hr = nullptr; - grpc_ares_request* r = nullptr; ares_channel* channel = nullptr; /* TODO(zyc): Enable tracing after #9603 is checked in */ /* if (grpc_dns_trace) { @@ -390,14 +387,6 @@ grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( } port = gpr_strdup(default_port); } - r = static_cast<grpc_ares_request*>(gpr_zalloc(sizeof(grpc_ares_request))); - r->ev_driver = nullptr; - r->on_done = on_done; - r->lb_addrs_out = addrs; - r->service_config_json_out = service_config_json; - r->success = false; - r->error = GRPC_ERROR_NONE; - r->pending_queries = 0; error = grpc_ares_ev_driver_create_locked(&r->ev_driver, interested_parties, combiner, r); if (error != GRPC_ERROR_NONE) goto error_cleanup; @@ -458,7 +447,7 @@ grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( on_srv_query_done_locked, r); gpr_free(service_name); } - if (service_config_json != nullptr) { + if (r->service_config_json_out != nullptr) { grpc_ares_request_ref_locked(r); char* config_name; gpr_asprintf(&config_name, "_grpc_config.%s", host); @@ -470,14 +459,12 @@ grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( grpc_ares_request_unref_locked(r); gpr_free(host); gpr_free(port); - return r; + return; error_cleanup: - GRPC_CLOSURE_SCHED(on_done, error); - gpr_free(r); + GRPC_CLOSURE_SCHED(r->on_done, error); gpr_free(host); gpr_free(port); - return nullptr; } static bool inner_resolve_as_ip_literal_locked(const char* name, @@ -536,21 +523,31 @@ static grpc_ares_request* grpc_dns_lookup_ares_locked_impl( grpc_pollset_set* interested_parties, grpc_closure* on_done, grpc_lb_addresses** addrs, bool check_grpclb, char** service_config_json, grpc_combiner* combiner) { + grpc_ares_request* r = + static_cast<grpc_ares_request*>(gpr_zalloc(sizeof(grpc_ares_request))); + r->ev_driver = nullptr; + r->on_done = on_done; + r->lb_addrs_out = addrs; + r->service_config_json_out = service_config_json; + r->success = false; + r->error = GRPC_ERROR_NONE; + r->pending_queries = 0; // Early out if the target is an ipv4 or ipv6 literal. if (resolve_as_ip_literal_locked(name, default_port, addrs)) { GRPC_CLOSURE_SCHED(on_done, GRPC_ERROR_NONE); - return nullptr; + return r; } // Early out if the target is localhost and we're on Windows. if (grpc_ares_maybe_resolve_localhost_manually_locked(name, default_port, addrs)) { GRPC_CLOSURE_SCHED(on_done, GRPC_ERROR_NONE); - return nullptr; + return r; } // Look up name using c-ares lib. - return grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( - dns_server, name, default_port, interested_parties, on_done, addrs, - check_grpclb, service_config_json, combiner); + grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked( + r, dns_server, name, default_port, interested_parties, check_grpclb, + combiner); + return r; } grpc_ares_request* (*grpc_dns_lookup_ares_locked)( @@ -559,14 +556,16 @@ grpc_ares_request* (*grpc_dns_lookup_ares_locked)( grpc_lb_addresses** addrs, bool check_grpclb, char** service_config_json, grpc_combiner* combiner) = grpc_dns_lookup_ares_locked_impl; -void grpc_cancel_ares_request(grpc_ares_request* r) { - if (grpc_dns_lookup_ares_locked == grpc_dns_lookup_ares_locked_impl) { - if (r != nullptr) { - grpc_ares_ev_driver_shutdown_locked(r->ev_driver); - } +static void grpc_cancel_ares_request_locked_impl(grpc_ares_request* r) { + GPR_ASSERT(r != nullptr); + if (r->ev_driver != nullptr) { + grpc_ares_ev_driver_shutdown_locked(r->ev_driver); } } +void (*grpc_cancel_ares_request_locked)(grpc_ares_request* r) = + grpc_cancel_ares_request_locked_impl; + grpc_error* grpc_ares_init(void) { gpr_once_init(&g_basic_init, do_basic_init); gpr_mu_lock(&g_init_mu); @@ -603,20 +602,23 @@ typedef struct grpc_resolve_address_ares_request { grpc_lb_addresses* lb_addrs; /** closure to call when the resolve_address_ares request completes */ grpc_closure* on_resolve_address_done; - /** a closure wrapping on_dns_lookup_done_cb, which should be invoked when the - grpc_dns_lookup_ares_locked operation is done. */ - grpc_closure on_dns_lookup_done; + /** a closure wrapping on_resolve_address_done, which should be invoked when + the grpc_dns_lookup_ares_locked operation is done. */ + grpc_closure on_dns_lookup_done_locked; /* target name */ const char* name; /* default port to use if none is specified */ const char* default_port; /* pollset_set to be driven by */ grpc_pollset_set* interested_parties; + /* underlying ares_request that the query is performed on */ + grpc_ares_request* ares_request; } grpc_resolve_address_ares_request; -static void on_dns_lookup_done_cb(void* arg, grpc_error* error) { +static void on_dns_lookup_done_locked(void* arg, grpc_error* error) { grpc_resolve_address_ares_request* r = static_cast<grpc_resolve_address_ares_request*>(arg); + gpr_free(r->ares_request); grpc_resolved_addresses** resolved_addresses = r->addrs_out; if (r->lb_addrs == nullptr || r->lb_addrs->num_addresses == 0) { *resolved_addresses = nullptr; @@ -643,9 +645,9 @@ static void grpc_resolve_address_invoke_dns_lookup_ares_locked( void* arg, grpc_error* unused_error) { grpc_resolve_address_ares_request* r = static_cast<grpc_resolve_address_ares_request*>(arg); - grpc_dns_lookup_ares_locked( + r->ares_request = grpc_dns_lookup_ares_locked( nullptr /* dns_server */, r->name, r->default_port, r->interested_parties, - &r->on_dns_lookup_done, &r->lb_addrs, false /* check_grpclb */, + &r->on_dns_lookup_done_locked, &r->lb_addrs, false /* check_grpclb */, nullptr /* service_config_json */, r->combiner); } @@ -660,8 +662,8 @@ static void grpc_resolve_address_ares_impl(const char* name, r->combiner = grpc_combiner_create(); r->addrs_out = addrs; r->on_resolve_address_done = on_done; - GRPC_CLOSURE_INIT(&r->on_dns_lookup_done, on_dns_lookup_done_cb, r, - grpc_schedule_on_exec_ctx); + GRPC_CLOSURE_INIT(&r->on_dns_lookup_done_locked, on_dns_lookup_done_locked, r, + grpc_combiner_scheduler(r->combiner)); r->name = name; r->default_port = default_port; r->interested_parties = interested_parties; diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h index 1bc457d4cf..a1231cc4e0 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h @@ -54,7 +54,8 @@ extern void (*grpc_resolve_address_ares)(const char* name, port in \a name. 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, so it must not try to acquire locks that are - being held by the caller. */ + being held by the caller. The returned grpc_ares_request object is owned + by the caller and it is safe to free after on_done is called back. */ extern grpc_ares_request* (*grpc_dns_lookup_ares_locked)( const char* dns_server, const char* name, const char* default_port, grpc_pollset_set* interested_parties, grpc_closure* on_done, @@ -62,7 +63,7 @@ extern grpc_ares_request* (*grpc_dns_lookup_ares_locked)( char** service_config_json, grpc_combiner* combiner); /* Cancel the pending grpc_ares_request \a request */ -void grpc_cancel_ares_request(grpc_ares_request* request); +extern void (*grpc_cancel_ares_request_locked)(grpc_ares_request* request); /* Initialize gRPC ares wrapper. Must be called at least once before grpc_resolve_address_ares(). */ diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc index d6a76fc8b6..9f293c1ac0 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper_fallback.cc @@ -40,7 +40,10 @@ grpc_ares_request* (*grpc_dns_lookup_ares_locked)( grpc_lb_addresses** addrs, bool check_grpclb, char** service_config_json, grpc_combiner* combiner) = grpc_dns_lookup_ares_locked_impl; -void grpc_cancel_ares_request(grpc_ares_request* r) {} +static void grpc_cancel_ares_request_locked_impl(grpc_ares_request* r) {} + +void (*grpc_cancel_ares_request_locked)(grpc_ares_request* r) = + grpc_cancel_ares_request_locked_impl; grpc_error* grpc_ares_init(void) { return GRPC_ERROR_NONE; } diff --git a/test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc b/test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc index 1c8d0775ab..eb5a911748 100644 --- a/test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc +++ b/test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc @@ -82,6 +82,10 @@ static grpc_ares_request* my_dns_lookup_ares_locked( return nullptr; } +static void my_cancel_ares_request_locked(grpc_ares_request* request) { + GPR_ASSERT(request == nullptr); +} + static grpc_core::OrphanablePtr<grpc_core::Resolver> create_resolver( const char* name) { grpc_core::ResolverFactory* factory = @@ -148,6 +152,7 @@ int main(int argc, char** argv) { g_combiner = grpc_combiner_create(); grpc_set_resolver_impl(&test_resolver); grpc_dns_lookup_ares_locked = my_dns_lookup_ares_locked; + grpc_cancel_ares_request_locked = my_cancel_ares_request_locked; grpc_channel_args* result = (grpc_channel_args*)1; { diff --git a/test/core/end2end/fuzzers/api_fuzzer.cc b/test/core/end2end/fuzzers/api_fuzzer.cc index 9544adb912..e97a544e12 100644 --- a/test/core/end2end/fuzzers/api_fuzzer.cc +++ b/test/core/end2end/fuzzers/api_fuzzer.cc @@ -390,6 +390,10 @@ grpc_ares_request* my_dns_lookup_ares_locked( return nullptr; } +static void my_cancel_ares_request_locked(grpc_ares_request* request) { + GPR_ASSERT(request == nullptr); +} + //////////////////////////////////////////////////////////////////////////////// // client connection @@ -705,6 +709,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { } grpc_set_resolver_impl(&fuzzer_resolver); grpc_dns_lookup_ares_locked = my_dns_lookup_ares_locked; + grpc_cancel_ares_request_locked = my_cancel_ares_request_locked; GPR_ASSERT(g_channel == nullptr); GPR_ASSERT(g_server == nullptr); diff --git a/test/core/end2end/goaway_server_test.cc b/test/core/end2end/goaway_server_test.cc index 8904c3d325..3f1c5596ad 100644 --- a/test/core/end2end/goaway_server_test.cc +++ b/test/core/end2end/goaway_server_test.cc @@ -50,6 +50,8 @@ static grpc_ares_request* (*iomgr_dns_lookup_ares_locked)( grpc_lb_addresses** addresses, bool check_grpclb, char** service_config_json, grpc_combiner* combiner); +static void (*iomgr_cancel_ares_request_locked)(grpc_ares_request* request); + static void set_resolve_port(int port) { gpr_mu_lock(&g_mu); g_resolve_port = port; @@ -130,6 +132,12 @@ static grpc_ares_request* my_dns_lookup_ares_locked( return nullptr; } +static void my_cancel_ares_request_locked(grpc_ares_request* request) { + if (request != nullptr) { + iomgr_cancel_ares_request_locked(request); + } +} + int main(int argc, char** argv) { grpc_completion_queue* cq; cq_verifier* cqv; @@ -143,7 +151,9 @@ int main(int argc, char** argv) { default_resolver = grpc_resolve_address_impl; grpc_set_resolver_impl(&test_resolver); iomgr_dns_lookup_ares_locked = grpc_dns_lookup_ares_locked; + iomgr_cancel_ares_request_locked = grpc_cancel_ares_request_locked; grpc_dns_lookup_ares_locked = my_dns_lookup_ares_locked; + grpc_cancel_ares_request_locked = my_cancel_ares_request_locked; int was_cancelled1; int was_cancelled2; |