PR comments

4 files changed, 71 insertions, 59 deletions

diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h
index a303ed15ae..d64b23f332 100644
--- a/include/grpc/impl/codegen/grpc_types.h
+++ b/include/grpc/impl/codegen/grpc_types.h
@@ -241,7 +241,7 @@ typedef struct {
   "grpc.initial_reconnect_backoff_ms"
 /** Minimum amount of time between DNS resolutions, in ms */
 #define GRPC_ARG_DNS_MIN_TIME_BETWEEN_RESOLUTIONS_MS \
-  "grpc.dns_min_resolution_period_ms"
+  "grpc.dns_min_time_between_resolutions_ms"
 /** The timeout used on servers for finishing handshaking on an incoming
     connection.  Defaults to 120 seconds. */
 #define GRPC_ARG_SERVER_HANDSHAKE_TIMEOUT_MS "grpc.server_handshake_timeout_ms"
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 d2bf9950ee..82cdb81c8b 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
@@ -357,6 +357,8 @@ static void dns_ares_maybe_start_resolving_locked(ares_dns_resolver* r) {
         grpc_timer_init(&r->next_resolution_timer, ms_until_next_resolution,
                         &r->deferred_resolution_closure);
       }
+      // TODO(dgq): remove the following two lines once Pick First stops
+      // discarding subchannels after selecting.
       ++r->resolved_version;
       dns_ares_maybe_finish_next_locked(r);
       return;
@@ -426,9 +428,8 @@ static grpc_resolver* dns_ares_create(grpc_resolver_args* args,
                     grpc_combiner_scheduler(r->base.combiner));
   const grpc_arg* period_arg = grpc_channel_args_find(
       args->args, GRPC_ARG_DNS_MIN_TIME_BETWEEN_RESOLUTIONS_MS);
-  const grpc_millis min_time_between_resolutions =
+  r->min_time_between_resolutions =
       grpc_channel_arg_get_integer(period_arg, {1000, 0, INT_MAX});
-  r->min_time_between_resolutions = min_time_between_resolutions;
   r->last_resolution_timestamp = -1;
   GRPC_CLOSURE_INIT(&r->deferred_resolution_closure, ares_cooldown_cb, r,
                     grpc_combiner_scheduler(r->base.combiner));
diff --git a/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc b/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc
index b726cc4c6d..68e850a354 100644
--- a/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc
+++ b/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc
@@ -138,10 +138,8 @@ static void dns_next_locked(grpc_resolver* resolver,
 static void dns_on_next_resolution_timer_locked(void* arg, grpc_error* error) {
   dns_resolver* r = (dns_resolver*)arg;
   r->have_next_resolution_timer = false;
-  if (error == GRPC_ERROR_NONE) {
-    if (!r->resolving) {
-      dns_start_resolving_locked(r);
-    }
+  if (error == GRPC_ERROR_NONE && !r->resolving) {
+    dns_start_resolving_locked(r);
   }
   GRPC_RESOLVER_UNREF(&r->base, "next_resolution_timer");
 }
@@ -213,6 +211,8 @@ static void maybe_start_resolving_locked(dns_resolver* r) {
         grpc_timer_init(&r->next_resolution_timer, ms_until_next_resolution,
                         &r->deferred_resolution_closure);
       }
+      // TODO(dgq): remove the following two lines once Pick First stops
+      // discarding subchannels after selecting.
       ++r->resolved_version;
       dns_maybe_finish_next_locked(r);
       return;
@@ -258,15 +258,6 @@ static void dns_destroy(grpc_resolver* gr) {
   gpr_free(r);
 }
 
-static void cooldown_cb(void* arg, grpc_error* error) {
-  dns_resolver* r = static_cast<dns_resolver*>(arg);
-  r->have_next_resolution_timer = false;
-  if (error == GRPC_ERROR_NONE && !r->resolving) {
-    dns_start_resolving_locked(r);
-  }
-  GRPC_RESOLVER_UNREF(&r->base, "next_resolution_timer_cooldown");
-}
-
 static grpc_resolver* dns_create(grpc_resolver_args* args,
                                  const char* default_port) {
   if (0 != strcmp(args->uri->authority, "")) {
@@ -295,11 +286,11 @@ static grpc_resolver* dns_create(grpc_resolver_args* args,
   r->backoff.Init(grpc_core::BackOff(backoff_options));
   const grpc_arg* period_arg = grpc_channel_args_find(
       args->args, GRPC_ARG_DNS_MIN_TIME_BETWEEN_RESOLUTIONS_MS);
-  const grpc_millis min_time_between_resolutions =
+  r->min_time_between_resolutions =
       grpc_channel_arg_get_integer(period_arg, {1000, 0, INT_MAX});
-  r->min_time_between_resolutions = min_time_between_resolutions;
   r->last_resolution_timestamp = -1;
-  GRPC_CLOSURE_INIT(&r->deferred_resolution_closure, cooldown_cb, r,
+  GRPC_CLOSURE_INIT(&r->deferred_resolution_closure,
+                    dns_on_next_resolution_timer_locked, r,
                     grpc_combiner_scheduler(r->base.combiner));
   return &r->base;
 }
diff --git a/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc b/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc
index be153764c3..64342b48c8 100644
--- a/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc
+++ b/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc
@@ -43,16 +43,13 @@ grpc_ares_request* (*g_default_dns_lookup_ares)(
 // times a system-level resolution has happened.
 static int g_resolution_count;
 
-typedef struct args_struct {
+struct iomgr_args {
   gpr_event ev;
-  grpc_resolved_addresses* addrs;
   gpr_atm done_atm;
   gpr_mu* mu;
   grpc_pollset* pollset;
   grpc_pollset_set* pollset_set;
-} args_struct;
-
-args_struct g_iomgr_args;
+} g_iomgr_args;
 
 // Wrapper around g_default_grpc_resolve_address in order to count the number of
 // times we incur in a system-level name resolution.
@@ -83,19 +80,17 @@ static gpr_timespec test_deadline(void) {
 
 static void do_nothing(void* arg, grpc_error* error) {}
 
-void args_init(args_struct* args) {
+void iomgr_args_init(iomgr_args* args) {
   gpr_event_init(&args->ev);
   args->pollset = static_cast<grpc_pollset*>(gpr_zalloc(grpc_pollset_size()));
   grpc_pollset_init(args->pollset, &args->mu);
   args->pollset_set = grpc_pollset_set_create();
   grpc_pollset_set_add_pollset(args->pollset_set, args->pollset);
-  args->addrs = nullptr;
   gpr_atm_rel_store(&args->done_atm, 0);
 }
 
-void args_finish(args_struct* args) {
+void iomgr_args_finish(iomgr_args* args) {
   GPR_ASSERT(gpr_event_wait(&args->ev, test_deadline()));
-  grpc_resolved_addresses_destroy(args->addrs);
   grpc_pollset_set_del_pollset(args->pollset_set, args->pollset);
   grpc_pollset_set_destroy(args->pollset_set);
   grpc_closure do_nothing_cb;
@@ -115,7 +110,7 @@ static grpc_millis n_sec_deadline(int seconds) {
       grpc_timeout_seconds_to_deadline(seconds));
 }
 
-static void poll_pollset_until_request_done(args_struct* args) {
+static void poll_pollset_until_request_done(iomgr_args* args) {
   grpc_core::ExecCtx exec_ctx;
   grpc_millis deadline = n_sec_deadline(10);
   while (true) {
@@ -137,9 +132,11 @@ static void poll_pollset_until_request_done(args_struct* args) {
 }
 
 typedef struct on_resolution_cb_arg {
+  const char* uri_str;
   grpc_resolver* resolver;
   grpc_channel_args* result;
   grpc_millis delay_before_second_resolution;
+  bool using_cares;
 } on_resolution_cb_arg;
 
 // Counter for the number of times a resolution notification callback has been
@@ -151,9 +148,9 @@ bool g_all_callbacks_invoked;
 
 void on_third_resolution(void* arg, grpc_error* error) {
   on_resolution_cb_arg* cb_arg = static_cast<on_resolution_cb_arg*>(arg);
+  GPR_ASSERT(error == GRPC_ERROR_NONE);
   ++g_on_resolution_invocations_count;
   grpc_channel_args_destroy(cb_arg->result);
-
   gpr_log(GPR_INFO,
           "3rd: g_on_resolution_invocations_count: %d, g_resolution_count: %d",
           g_on_resolution_invocations_count, g_resolution_count);
@@ -162,9 +159,15 @@ void on_third_resolution(void* arg, grpc_error* error) {
   // period.
   GPR_ASSERT(g_on_resolution_invocations_count == 3);
   GPR_ASSERT(g_resolution_count == 2);
-
   grpc_resolver_shutdown_locked(cb_arg->resolver);
   GRPC_RESOLVER_UNREF(cb_arg->resolver, "on_third_resolution");
+  if (cb_arg->using_cares) {
+    gpr_atm_rel_store(&g_iomgr_args.done_atm, 1);
+    gpr_mu_lock(g_iomgr_args.mu);
+    GRPC_LOG_IF_ERROR("pollset_kick",
+                      grpc_pollset_kick(g_iomgr_args.pollset, nullptr));
+    gpr_mu_unlock(g_iomgr_args.mu);
+  }
   gpr_free(cb_arg);
   g_all_callbacks_invoked = true;
 }
@@ -182,22 +185,29 @@ void on_second_resolution(void* arg, grpc_error* error) {
   // resolutions happened, as indicated by g_resolution_count.
   GPR_ASSERT(g_on_resolution_invocations_count == 2);
   GPR_ASSERT(g_resolution_count == 1);
-
   grpc_core::ExecCtx::Get()->TestOnlySetNow(
       cb_arg->delay_before_second_resolution * 2);
   grpc_resolver_next_locked(
       cb_arg->resolver, &cb_arg->result,
-      GRPC_CLOSURE_CREATE(on_third_resolution, arg, grpc_schedule_on_exec_ctx));
+      GRPC_CLOSURE_CREATE(on_third_resolution, arg,
+                          grpc_combiner_scheduler(g_combiner)));
   grpc_resolver_channel_saw_error_locked(cb_arg->resolver);
+  if (cb_arg->using_cares) {
+    gpr_mu_lock(g_iomgr_args.mu);
+    GRPC_LOG_IF_ERROR("pollset_kick",
+                      grpc_pollset_kick(g_iomgr_args.pollset, nullptr));
+    gpr_mu_unlock(g_iomgr_args.mu);
+  }
 }
 
 void on_first_resolution(void* arg, grpc_error* error) {
   on_resolution_cb_arg* cb_arg = static_cast<on_resolution_cb_arg*>(arg);
   ++g_on_resolution_invocations_count;
   grpc_channel_args_destroy(cb_arg->result);
-  grpc_resolver_next_locked(cb_arg->resolver, &cb_arg->result,
-                            GRPC_CLOSURE_CREATE(on_second_resolution, arg,
-                                                grpc_schedule_on_exec_ctx));
+  grpc_resolver_next_locked(
+      cb_arg->resolver, &cb_arg->result,
+      GRPC_CLOSURE_CREATE(on_second_resolution, arg,
+                          grpc_combiner_scheduler(g_combiner)));
   grpc_resolver_channel_saw_error_locked(cb_arg->resolver);
   gpr_log(GPR_INFO,
           "1st: g_on_resolution_invocations_count: %d, g_resolution_count: %d",
@@ -206,18 +216,21 @@ void on_first_resolution(void* arg, grpc_error* error) {
   // notification callback (the current function).
   GPR_ASSERT(g_on_resolution_invocations_count == 1);
   GPR_ASSERT(g_resolution_count == 1);
+  if (cb_arg->using_cares) {
+    gpr_mu_lock(g_iomgr_args.mu);
+    GRPC_LOG_IF_ERROR("pollset_kick",
+                      grpc_pollset_kick(g_iomgr_args.pollset, nullptr));
+    gpr_mu_unlock(g_iomgr_args.mu);
+  }
 }
 
-static void test_cooldown(const char* uri_str, bool use_cares) {
-  grpc_core::ExecCtx exec_ctx;
+static void start_test_under_combiner(void* arg, grpc_error* error) {
+  on_resolution_cb_arg* res_cb_arg = static_cast<on_resolution_cb_arg*>(arg);
+  grpc_resolver* resolver;
   grpc_resolver_factory* factory = grpc_resolver_factory_lookup("dns");
-  grpc_uri* uri = grpc_uri_parse(uri_str, 0);
+  grpc_uri* uri = grpc_uri_parse(res_cb_arg->uri_str, 0);
   grpc_resolver_args args;
-
-  if (use_cares) args_init(&g_iomgr_args);
-
-  grpc_resolver* resolver;
-  gpr_log(GPR_DEBUG, "test: '%s' should be valid for '%s'", uri_str,
+  gpr_log(GPR_DEBUG, "test: '%s' should be valid for '%s'", res_cb_arg->uri_str,
           factory->vtable->scheme);
   GPR_ASSERT(uri);
   memset(&args, 0, sizeof(args));
@@ -235,27 +248,36 @@ static void test_cooldown(const char* uri_str, bool use_cares) {
   auto* cooldown_channel_args =
       grpc_channel_args_copy_and_add(nullptr, &cooldown_arg, 1);
   args.args = cooldown_channel_args;
-
   resolver = grpc_resolver_factory_create_resolver(factory, &args);
   grpc_channel_args_destroy(cooldown_channel_args);
   GPR_ASSERT(resolver != nullptr);
-
-  on_resolution_cb_arg* arg = (on_resolution_cb_arg*)gpr_zalloc(sizeof(*arg));
-  arg->resolver = resolver;
-  arg->delay_before_second_resolution = kMinResolutionPeriodMs;
+  res_cb_arg->resolver = resolver;
+  res_cb_arg->delay_before_second_resolution = kMinResolutionPeriodMs;
   // First resolution, would incur in system-level resolution.
   grpc_resolver_next_locked(
-      resolver, &arg->result,
-      GRPC_CLOSURE_CREATE(on_first_resolution, arg, grpc_schedule_on_exec_ctx));
+      resolver, &res_cb_arg->result,
+      GRPC_CLOSURE_CREATE(on_first_resolution, res_cb_arg,
+                          grpc_combiner_scheduler(g_combiner)));
   grpc_uri_destroy(uri);
+  grpc_resolver_factory_unref(factory);
+}
 
-  if (use_cares) {
+static void test_cooldown(bool using_cares) {
+  grpc_core::ExecCtx exec_ctx;
+  if (using_cares) iomgr_args_init(&g_iomgr_args);
+  on_resolution_cb_arg* res_cb_arg =
+      static_cast<on_resolution_cb_arg*>(gpr_zalloc(sizeof(*res_cb_arg)));
+  res_cb_arg->uri_str = "dns:127.0.0.1";
+  res_cb_arg->using_cares = using_cares;
+
+  GRPC_CLOSURE_SCHED(GRPC_CLOSURE_CREATE(start_test_under_combiner, res_cb_arg,
+                                         grpc_combiner_scheduler(g_combiner)),
+                     GRPC_ERROR_NONE);
+  if (using_cares) {
     grpc_core::ExecCtx::Get()->Flush();
     poll_pollset_until_request_done(&g_iomgr_args);
-    args_finish(&g_iomgr_args);
+    iomgr_args_finish(&g_iomgr_args);
   }
-
-  grpc_resolver_factory_unref(factory);
 }
 
 int main(int argc, char** argv) {
@@ -264,14 +286,13 @@ int main(int argc, char** argv) {
 
   g_combiner = grpc_combiner_create();
 
-  const bool use_cares = (grpc_resolve_address == grpc_resolve_address_ares);
-
+  const bool using_cares = (grpc_resolve_address == grpc_resolve_address_ares);
   g_default_grpc_resolve_address = grpc_resolve_address;
   g_default_dns_lookup_ares = grpc_dns_lookup_ares;
   grpc_dns_lookup_ares = test_dns_lookup_ares;
   grpc_resolve_address = test_resolve_address_impl;
 
-  test_cooldown("dns:127.0.0.1", use_cares);
+  test_cooldown(using_cares);
 
   {
     grpc_core::ExecCtx exec_ctx;
@@ -279,6 +300,5 @@ int main(int argc, char** argv) {
   }
   grpc_shutdown();
   GPR_ASSERT(g_all_callbacks_invoked);
-
   return 0;
 }