Fix dns_resolver_cooldown_test and fake_resolver_test.

2 files changed, 48 insertions, 124 deletions

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 b1f3a1c08a..27de7cac95 100644
--- a/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc
+++ b/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc
@@ -28,12 +28,14 @@
 #include "src/core/lib/iomgr/sockaddr_utils.h"
 #include "test/core/util/test_config.h"
 
+constexpr int kMinResolutionPeriodMs = 1000;
+
 extern grpc_address_resolver_vtable* grpc_resolve_address_impl;
 static grpc_address_resolver_vtable* default_resolve_address;
 
 static grpc_combiner* g_combiner;
 
-grpc_ares_request* (*g_default_dns_lookup_ares_locked)(
+static grpc_ares_request* (*g_default_dns_lookup_ares_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,
@@ -43,7 +45,7 @@ grpc_ares_request* (*g_default_dns_lookup_ares_locked)(
 // times a system-level resolution has happened.
 static int g_resolution_count;
 
-struct iomgr_args {
+static struct iomgr_args {
   gpr_event ev;
   gpr_atm done_atm;
   gpr_mu* mu;
@@ -61,6 +63,16 @@ static void test_resolve_address_impl(const char* name,
   default_resolve_address->resolve_address(
       name, default_port, g_iomgr_args.pollset_set, on_done, addrs);
   ++g_resolution_count;
+  static grpc_millis last_resolution_time = 0;
+  if (last_resolution_time == 0) {
+    last_resolution_time =
+        grpc_timespec_to_millis_round_up(gpr_now(GPR_CLOCK_MONOTONIC));
+  } else {
+    grpc_millis now =
+        grpc_timespec_to_millis_round_up(gpr_now(GPR_CLOCK_MONOTONIC));
+    GPR_ASSERT(now - last_resolution_time >= kMinResolutionPeriodMs);
+    last_resolution_time = now;
+  }
 }
 
 static grpc_error* test_blocking_resolve_address_impl(
@@ -73,7 +85,7 @@ static grpc_error* test_blocking_resolve_address_impl(
 static grpc_address_resolver_vtable test_resolver = {
     test_resolve_address_impl, test_blocking_resolve_address_impl};
 
-grpc_ares_request* test_dns_lookup_ares_locked(
+static grpc_ares_request* test_dns_lookup_ares_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,
@@ -82,6 +94,15 @@ grpc_ares_request* test_dns_lookup_ares_locked(
       dns_server, name, default_port, g_iomgr_args.pollset_set, on_done, addrs,
       check_grpclb, service_config_json, combiner);
   ++g_resolution_count;
+  static grpc_millis last_resolution_time = 0;
+  if (last_resolution_time == 0) {
+        grpc_timespec_to_millis_round_up(gpr_now(GPR_CLOCK_MONOTONIC));
+  } else {
+    grpc_millis now =
+        grpc_timespec_to_millis_round_up(gpr_now(GPR_CLOCK_MONOTONIC));
+    GPR_ASSERT(now - last_resolution_time >= kMinResolutionPeriodMs);
+    last_resolution_time = now;
+  }
   return result;
 }
 
@@ -91,7 +112,7 @@ static gpr_timespec test_deadline(void) {
 
 static void do_nothing(void* arg, grpc_error* error) {}
 
-void iomgr_args_init(iomgr_args* args) {
+static 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);
@@ -100,7 +121,7 @@ void iomgr_args_init(iomgr_args* args) {
   gpr_atm_rel_store(&args->done_atm, 0);
 }
 
-void iomgr_args_finish(iomgr_args* args) {
+static void iomgr_args_finish(iomgr_args* args) {
   GPR_ASSERT(gpr_event_wait(&args->ev, test_deadline()));
   grpc_pollset_set_del_pollset(args->pollset_set, args->pollset);
   grpc_pollset_set_destroy(args->pollset_set);
@@ -146,29 +167,19 @@ struct OnResolutionCallbackArg {
   const char* uri_str = nullptr;
   grpc_core::OrphanablePtr<grpc_core::Resolver> resolver;
   grpc_channel_args* result = nullptr;
-  grpc_millis delay_before_second_resolution = 0;
 };
 
-// Counter for the number of times a resolution notification callback has been
-// invoked.
-static int g_on_resolution_invocations_count;
-
 // Set to true by the last callback in the resolution chain.
-bool g_all_callbacks_invoked;
+static bool g_all_callbacks_invoked;
 
-void on_fourth_resolution(void* arg, grpc_error* error) {
+static void on_second_resolution(void* arg, grpc_error* error) {
   OnResolutionCallbackArg* cb_arg = static_cast<OnResolutionCallbackArg*>(arg);
   grpc_channel_args_destroy(cb_arg->result);
   GPR_ASSERT(error == GRPC_ERROR_NONE);
-  ++g_on_resolution_invocations_count;
-  gpr_log(GPR_INFO,
-          "4th: g_on_resolution_invocations_count: %d, g_resolution_count: %d",
-          g_on_resolution_invocations_count, g_resolution_count);
-  // In this case we expect to have incurred in another system-level resolution
-  // because on_third_resolution slept for longer than the min resolution
-  // period.
-  GPR_ASSERT(g_on_resolution_invocations_count == 4);
-  GPR_ASSERT(g_resolution_count == 3);
+  gpr_log(GPR_INFO, "2nd: g_resolution_count: %d", g_resolution_count);
+  // The resolution callback was not invoked until new data was
+  // available, which was delayed until after the cooldown period.
+  GPR_ASSERT(g_resolution_count == 2);
   cb_arg->resolver.reset();
   gpr_atm_rel_store(&g_iomgr_args.done_atm, 1);
   gpr_mu_lock(g_iomgr_args.mu);
@@ -179,67 +190,13 @@ void on_fourth_resolution(void* arg, grpc_error* error) {
   g_all_callbacks_invoked = true;
 }
 
-void on_third_resolution(void* arg, grpc_error* error) {
-  OnResolutionCallbackArg* cb_arg = static_cast<OnResolutionCallbackArg*>(arg);
-  grpc_channel_args_destroy(cb_arg->result);
-  GPR_ASSERT(error == GRPC_ERROR_NONE);
-  ++g_on_resolution_invocations_count;
-  gpr_log(GPR_INFO,
-          "3rd: g_on_resolution_invocations_count: %d, g_resolution_count: %d",
-          g_on_resolution_invocations_count, g_resolution_count);
-  // The timer set because of the previous re-resolution request fires, so a new
-  // system-level resolution happened.
-  GPR_ASSERT(g_on_resolution_invocations_count == 3);
-  GPR_ASSERT(g_resolution_count == 2);
-  grpc_core::ExecCtx::Get()->TestOnlySetNow(
-      cb_arg->delay_before_second_resolution * 2);
-  cb_arg->resolver->NextLocked(
-      &cb_arg->result,
-      GRPC_CLOSURE_CREATE(on_fourth_resolution, arg,
-                          grpc_combiner_scheduler(g_combiner)));
-  cb_arg->resolver->RequestReresolutionLocked();
-  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_second_resolution(void* arg, grpc_error* error) {
-  OnResolutionCallbackArg* cb_arg = static_cast<OnResolutionCallbackArg*>(arg);
-  grpc_channel_args_destroy(cb_arg->result);
-  GPR_ASSERT(error == GRPC_ERROR_NONE);
-  ++g_on_resolution_invocations_count;
-  gpr_log(GPR_INFO,
-          "2nd: g_on_resolution_invocations_count: %d, g_resolution_count: %d",
-          g_on_resolution_invocations_count, g_resolution_count);
-  // The resolution request for which this function is the callback happened
-  // before the min resolution period. Therefore, no new system-level
-  // resolutions happened, as indicated by g_resolution_count. But a resolution
-  // timer was set to fire when the cooldown finishes.
-  GPR_ASSERT(g_on_resolution_invocations_count == 2);
-  GPR_ASSERT(g_resolution_count == 1);
-  // Register a new callback to capture the timer firing.
-  cb_arg->resolver->NextLocked(
-      &cb_arg->result,
-      GRPC_CLOSURE_CREATE(on_third_resolution, arg,
-                          grpc_combiner_scheduler(g_combiner)));
-  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) {
+static void on_first_resolution(void* arg, grpc_error* error) {
   OnResolutionCallbackArg* cb_arg = static_cast<OnResolutionCallbackArg*>(arg);
   grpc_channel_args_destroy(cb_arg->result);
   GPR_ASSERT(error == GRPC_ERROR_NONE);
-  ++g_on_resolution_invocations_count;
-  gpr_log(GPR_INFO,
-          "1st: g_on_resolution_invocations_count: %d, g_resolution_count: %d",
-          g_on_resolution_invocations_count, g_resolution_count);
+  gpr_log(GPR_INFO, "1st: g_resolution_count: %d", g_resolution_count);
   // There's one initial system-level resolution and one invocation of a
   // notification callback (the current function).
-  GPR_ASSERT(g_on_resolution_invocations_count == 1);
   GPR_ASSERT(g_resolution_count == 1);
   cb_arg->resolver->NextLocked(
       &cb_arg->result,
@@ -265,9 +222,7 @@ static void start_test_under_combiner(void* arg, grpc_error* error) {
   grpc_core::ResolverArgs args;
   args.uri = uri;
   args.combiner = g_combiner;
-  g_on_resolution_invocations_count = 0;
   g_resolution_count = 0;
-  constexpr int kMinResolutionPeriodMs = 1000;
 
   grpc_arg cooldown_arg;
   cooldown_arg.key =
@@ -280,7 +235,6 @@ static void start_test_under_combiner(void* arg, grpc_error* error) {
   res_cb_arg->resolver = factory->CreateResolver(args);
   grpc_channel_args_destroy(cooldown_channel_args);
   GPR_ASSERT(res_cb_arg->resolver != nullptr);
-  res_cb_arg->delay_before_second_resolution = kMinResolutionPeriodMs;
   // First resolution, would incur in system-level resolution.
   res_cb_arg->resolver->NextLocked(
       &res_cb_arg->result,
diff --git a/test/core/client_channel/resolvers/fake_resolver_test.cc b/test/core/client_channel/resolvers/fake_resolver_test.cc
index 14caa3ea5d..f6696bf127 100644
--- a/test/core/client_channel/resolvers/fake_resolver_test.cc
+++ b/test/core/client_channel/resolvers/fake_resolver_test.cc
@@ -124,8 +124,8 @@ static void test_fake_resolver() {
       build_fake_resolver(combiner, response_generator.get());
   GPR_ASSERT(resolver.get() != nullptr);
   // Test 1: normal resolution.
-  // next_results != NULL, reresolution_results == NULL, last_used_results ==
-  // NULL. Expected response is next_results.
+  // next_results != NULL, reresolution_results == NULL.
+  // Expected response is next_results.
   grpc_channel_args* results = create_new_resolver_result();
   on_resolution_arg on_res_arg = create_on_resolution_arg(results);
   grpc_closure* on_resolution = GRPC_CLOSURE_CREATE(
@@ -137,10 +137,9 @@ static void test_fake_resolver() {
   GPR_ASSERT(gpr_event_wait(&on_res_arg.ev,
                             grpc_timeout_seconds_to_deadline(5)) != nullptr);
   // Test 2: update resolution.
-  // next_results != NULL, reresolution_results == NULL, last_used_results !=
-  // NULL. Expected response is next_results.
+  // next_results != NULL, reresolution_results == NULL.
+  // Expected response is next_results.
   results = create_new_resolver_result();
-  grpc_channel_args* last_used_results = grpc_channel_args_copy(results);
   on_res_arg = create_on_resolution_arg(results);
   on_resolution = GRPC_CLOSURE_CREATE(on_resolution_cb, &on_res_arg,
                                       grpc_combiner_scheduler(combiner));
@@ -150,21 +149,9 @@ static void test_fake_resolver() {
   grpc_core::ExecCtx::Get()->Flush();
   GPR_ASSERT(gpr_event_wait(&on_res_arg.ev,
                             grpc_timeout_seconds_to_deadline(5)) != nullptr);
-  // Test 3: fallback re-resolution.
-  // next_results == NULL, reresolution_results == NULL, last_used_results !=
-  // NULL. Expected response is last_used_results.
-  on_res_arg = create_on_resolution_arg(last_used_results);
-  on_resolution = GRPC_CLOSURE_CREATE(on_resolution_cb, &on_res_arg,
-                                      grpc_combiner_scheduler(combiner));
-  resolver->NextLocked(&on_res_arg.resolver_result, on_resolution);
-  // Trigger a re-resolution.
-  resolver->RequestReresolutionLocked();
-  grpc_core::ExecCtx::Get()->Flush();
-  GPR_ASSERT(gpr_event_wait(&on_res_arg.ev,
-                            grpc_timeout_seconds_to_deadline(5)) != nullptr);
-  // Test 4: normal re-resolution.
-  // next_results == NULL, reresolution_results != NULL, last_used_results !=
-  // NULL. Expected response is reresolution_results.
+  // Test 3: normal re-resolution.
+  // next_results == NULL, reresolution_results != NULL.
+  // Expected response is reresolution_results.
   grpc_channel_args* reresolution_results = create_new_resolver_result();
   on_res_arg =
       create_on_resolution_arg(grpc_channel_args_copy(reresolution_results));
@@ -180,9 +167,9 @@ static void test_fake_resolver() {
   grpc_core::ExecCtx::Get()->Flush();
   GPR_ASSERT(gpr_event_wait(&on_res_arg.ev,
                             grpc_timeout_seconds_to_deadline(5)) != nullptr);
-  // Test 5: repeat re-resolution.
-  // next_results == NULL, reresolution_results != NULL, last_used_results !=
-  // NULL. Expected response is reresolution_results.
+  // Test 4: repeat re-resolution.
+  // next_results == NULL, reresolution_results != NULL.
+  // Expected response is reresolution_results.
   on_res_arg = create_on_resolution_arg(reresolution_results);
   on_resolution = GRPC_CLOSURE_CREATE(on_resolution_cb, &on_res_arg,
                                       grpc_combiner_scheduler(combiner));
@@ -192,11 +179,10 @@ static void test_fake_resolver() {
   grpc_core::ExecCtx::Get()->Flush();
   GPR_ASSERT(gpr_event_wait(&on_res_arg.ev,
                             grpc_timeout_seconds_to_deadline(5)) != nullptr);
-  // Test 6: normal resolution.
-  // next_results != NULL, reresolution_results != NULL, last_used_results !=
-  // NULL. Expected response is next_results.
+  // Test 5: normal resolution.
+  // next_results != NULL, reresolution_results != NULL.
+  // Expected response is next_results.
   results = create_new_resolver_result();
-  last_used_results = grpc_channel_args_copy(results);
   on_res_arg = create_on_resolution_arg(results);
   on_resolution = GRPC_CLOSURE_CREATE(on_resolution_cb, &on_res_arg,
                                       grpc_combiner_scheduler(combiner));
@@ -206,23 +192,7 @@ static void test_fake_resolver() {
   grpc_core::ExecCtx::Get()->Flush();
   GPR_ASSERT(gpr_event_wait(&on_res_arg.ev,
                             grpc_timeout_seconds_to_deadline(5)) != nullptr);
-  // Test 7: fallback re-resolution.
-  // next_results == NULL, reresolution_results == NULL, last_used_results !=
-  // NULL. Expected response is last_used_results.
-  on_res_arg = create_on_resolution_arg(last_used_results);
-  on_resolution = GRPC_CLOSURE_CREATE(on_resolution_cb, &on_res_arg,
-                                      grpc_combiner_scheduler(combiner));
-  resolver->NextLocked(&on_res_arg.resolver_result, on_resolution);
-  // Reset reresolution_results.
-  response_generator->SetReresolutionResponse(nullptr);
-  // Flush here to guarantee that reresolution_results has been reset.
-  grpc_core::ExecCtx::Get()->Flush();
-  // Trigger a re-resolution.
-  resolver->RequestReresolutionLocked();
-  grpc_core::ExecCtx::Get()->Flush();
-  GPR_ASSERT(gpr_event_wait(&on_res_arg.ev,
-                            grpc_timeout_seconds_to_deadline(5)) != nullptr);
-  // Test 8: no-op.
+  // Test 6: no-op.
   // Requesting a new resolution without setting the response shouldn't trigger
   // the resolution callback.
   memset(&on_res_arg, 0, sizeof(on_res_arg));