From 1a10a9b9bf8f3ebc096b0349ccadf8f8b047251f Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 14 Aug 2018 15:05:18 -0700 Subject: [PATCH] Fix dns_resolver_cooldown_test and fake_resolver_test. --- .../resolvers/dns_resolver_cooldown_test.cc | 114 ++++++------------ .../resolvers/fake_resolver_test.cc | 58 +++------ 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 b1f3a1c08aa..27de7cac958 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(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 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(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(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(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(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 14caa3ea5df..f6696bf1278 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));