From 434b3b62e585e122e07d34d55d71a6e5a4cba7ad Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Fri, 24 May 2019 10:24:17 -0700 Subject: [PATCH] Applied the best practice using global configuration --- src/core/ext/filters/client_channel/backup_poller.cc | 5 ++--- src/core/ext/filters/client_channel/backup_poller.h | 7 +++++-- .../ext/filters/client_channel/client_channel_plugin.cc | 2 ++ .../resolver/dns/c_ares/dns_resolver_ares.cc | 9 ++++++--- src/core/lib/gprpp/global_config.h | 9 +++++++++ src/core/lib/iomgr/iomgr.cc | 6 +++--- test/cpp/end2end/client_lb_end2end_test.cc | 9 ++++----- test/cpp/end2end/grpclb_end2end_test.cc | 9 ++++----- test/cpp/end2end/service_config_end2end_test.cc | 9 ++++----- test/cpp/end2end/xds_end2end_test.cc | 9 ++++----- 10 files changed, 43 insertions(+), 31 deletions(-) diff --git a/src/core/ext/filters/client_channel/backup_poller.cc b/src/core/ext/filters/client_channel/backup_poller.cc index dd761694414..9e51a83415e 100644 --- a/src/core/ext/filters/client_channel/backup_poller.cc +++ b/src/core/ext/filters/client_channel/backup_poller.cc @@ -65,8 +65,8 @@ GPR_GLOBAL_CONFIG_DEFINE_INT32( "idleness), so that the next RPC on this channel won't fail. Set to 0 to " "turn off the backup polls."); -static void init_globals() { - gpr_mu_init(&g_poller_mu); +void grpc_client_channel_global_init_backup_polling() { + gpr_once_init(&g_once, [] { gpr_mu_init(&g_poller_mu); }); int32_t poll_interval_ms = GPR_GLOBAL_CONFIG_GET(grpc_client_channel_backup_poll_interval_ms); if (poll_interval_ms < 0) { @@ -153,7 +153,6 @@ static void g_poller_init_locked() { void grpc_client_channel_start_backup_polling( grpc_pollset_set* interested_parties) { - gpr_once_init(&g_once, init_globals); if (g_poll_interval_ms == 0) { return; } diff --git a/src/core/ext/filters/client_channel/backup_poller.h b/src/core/ext/filters/client_channel/backup_poller.h index e1bf4f88b2a..b412081b960 100644 --- a/src/core/ext/filters/client_channel/backup_poller.h +++ b/src/core/ext/filters/client_channel/backup_poller.h @@ -27,11 +27,14 @@ GPR_GLOBAL_CONFIG_DECLARE_INT32(grpc_client_channel_backup_poll_interval_ms); -/* Start polling \a interested_parties periodically in the timer thread */ +/* Initializes backup polling. */ +void grpc_client_channel_global_init_backup_polling(); + +/* Starts polling \a interested_parties periodically in the timer thread. */ void grpc_client_channel_start_backup_polling( grpc_pollset_set* interested_parties); -/* Stop polling \a interested_parties */ +/* Stops polling \a interested_parties. */ void grpc_client_channel_stop_backup_polling( grpc_pollset_set* interested_parties); diff --git a/src/core/ext/filters/client_channel/client_channel_plugin.cc b/src/core/ext/filters/client_channel/client_channel_plugin.cc index e564df8be33..3a7492d82d9 100644 --- a/src/core/ext/filters/client_channel/client_channel_plugin.cc +++ b/src/core/ext/filters/client_channel/client_channel_plugin.cc @@ -24,6 +24,7 @@ #include +#include "src/core/ext/filters/client_channel/backup_poller.h" #include "src/core/ext/filters/client_channel/client_channel.h" #include "src/core/ext/filters/client_channel/client_channel_channelz.h" #include "src/core/ext/filters/client_channel/global_subchannel_pool.h" @@ -62,6 +63,7 @@ void grpc_client_channel_init(void) { GRPC_CLIENT_CHANNEL, GRPC_CHANNEL_INIT_BUILTIN_PRIORITY, append_filter, (void*)&grpc_client_channel_filter); grpc_http_connect_register_handshaker_factory(); + grpc_client_channel_global_init_backup_polling(); } void grpc_client_channel_shutdown(void) { 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 00358736a94..32a339af359 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 @@ -473,10 +473,13 @@ static bool should_use_ares(const char* resolver_env) { } #endif /* GRPC_UV */ +static bool g_use_ares_dns_resolver; + void grpc_resolver_dns_ares_init() { grpc_core::UniquePtr resolver = GPR_GLOBAL_CONFIG_GET(grpc_dns_resolver); if (should_use_ares(resolver.get())) { + g_use_ares_dns_resolver = true; gpr_log(GPR_DEBUG, "Using ares dns resolver"); address_sorting_init(); grpc_error* error = grpc_ares_init(); @@ -491,13 +494,13 @@ void grpc_resolver_dns_ares_init() { grpc_core::ResolverRegistry::Builder::RegisterResolverFactory( grpc_core::UniquePtr( grpc_core::New())); + } else { + g_use_ares_dns_resolver = false; } } void grpc_resolver_dns_ares_shutdown() { - grpc_core::UniquePtr resolver = - GPR_GLOBAL_CONFIG_GET(grpc_dns_resolver); - if (should_use_ares(resolver.get())) { + if (g_use_ares_dns_resolver) { address_sorting_shutdown(); grpc_ares_cleanup(); } diff --git a/src/core/lib/gprpp/global_config.h b/src/core/lib/gprpp/global_config.h index a1bbf07564c..ed986b8e04d 100644 --- a/src/core/lib/gprpp/global_config.h +++ b/src/core/lib/gprpp/global_config.h @@ -59,6 +59,15 @@ // // Declaring config variables for other modules to access: // GPR_GLOBAL_CONFIG_DECLARE_*TYPE*(name) +// +// * Caveat for setting global configs at runtime +// +// Setting global configs at runtime multiple times is safe but it doesn't +// mean that it will have a valid effect on the module depending configs. +// In unit tests, it may be unpredictable to set different global configs +// between test cases because grpc init and shutdown can ignore changes. +// It's considered safe to set global configs before the first call to +// grpc_init(). // -------------------------------------------------------------------- // How to customize the global configuration system: diff --git a/src/core/lib/iomgr/iomgr.cc b/src/core/lib/iomgr/iomgr.cc index b09d19fa759..802e3bdcb4d 100644 --- a/src/core/lib/iomgr/iomgr.cc +++ b/src/core/lib/iomgr/iomgr.cc @@ -49,6 +49,7 @@ static gpr_mu g_mu; static gpr_cv g_rcv; static int g_shutdown; static grpc_iomgr_object g_root_object; +static bool g_grpc_abort_on_leaks; void grpc_iomgr_init() { grpc_core::ExecCtx exec_ctx; @@ -62,6 +63,7 @@ void grpc_iomgr_init() { grpc_iomgr_platform_init(); grpc_timer_list_init(); grpc_core::grpc_errqueue_init(); + g_grpc_abort_on_leaks = GPR_GLOBAL_CONFIG_GET(grpc_abort_on_leaks); } void grpc_iomgr_start() { grpc_timer_manager_init(); } @@ -189,6 +191,4 @@ void grpc_iomgr_unregister_object(grpc_iomgr_object* obj) { gpr_free(obj->name); } -bool grpc_iomgr_abort_on_leaks(void) { - return GPR_GLOBAL_CONFIG_GET(grpc_abort_on_leaks); -} +bool grpc_iomgr_abort_on_leaks(void) { return g_grpc_abort_on_leaks; } diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index b623348e13f..f6d4a48b6f0 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -139,11 +139,7 @@ class ClientLbEnd2endTest : public ::testing::Test { : server_host_("localhost"), kRequestMessage_("Live long and prosper."), creds_(new SecureChannelCredentials( - grpc_fake_transport_security_credentials_create())) { - // Make the backup poller poll very frequently in order to pick up - // updates from all the subchannels's FDs. - GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1); - } + grpc_fake_transport_security_credentials_create())) {} void SetUp() override { grpc_init(); @@ -1485,6 +1481,9 @@ TEST_F(ClientLbInterceptTrailingMetadataTest, InterceptsRetriesEnabled) { } // namespace grpc int main(int argc, char** argv) { + // Make the backup poller poll very frequently in order to pick up + // updates from all the subchannels's FDs. + GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1); ::testing::InitGoogleTest(&argc, argv); grpc::testing::TestEnvironment env(argc, argv); const auto result = RUN_ALL_TESTS(); diff --git a/test/cpp/end2end/grpclb_end2end_test.cc b/test/cpp/end2end/grpclb_end2end_test.cc index 50b1383e7e1..70c443412aa 100644 --- a/test/cpp/end2end/grpclb_end2end_test.cc +++ b/test/cpp/end2end/grpclb_end2end_test.cc @@ -372,11 +372,7 @@ class GrpclbEnd2endTest : public ::testing::Test { num_backends_(num_backends), num_balancers_(num_balancers), client_load_reporting_interval_seconds_( - client_load_reporting_interval_seconds) { - // Make the backup poller poll very frequently in order to pick up - // updates from all the subchannels's FDs. - GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1); - } + client_load_reporting_interval_seconds) {} void SetUp() override { response_generator_ = @@ -1994,6 +1990,9 @@ TEST_F(SingleBalancerWithClientLoadReportingTest, Drop) { } // namespace grpc int main(int argc, char** argv) { + // Make the backup poller poll very frequently in order to pick up + // updates from all the subchannels's FDs. + GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1); grpc_init(); grpc::testing::TestEnvironment env(argc, argv); ::testing::InitGoogleTest(&argc, argv); diff --git a/test/cpp/end2end/service_config_end2end_test.cc b/test/cpp/end2end/service_config_end2end_test.cc index 14786b98f85..8bf4a7c22ff 100644 --- a/test/cpp/end2end/service_config_end2end_test.cc +++ b/test/cpp/end2end/service_config_end2end_test.cc @@ -117,11 +117,7 @@ class ServiceConfigEnd2endTest : public ::testing::Test { : server_host_("localhost"), kRequestMessage_("Live long and prosper."), creds_(new SecureChannelCredentials( - grpc_fake_transport_security_credentials_create())) { - // Make the backup poller poll very frequently in order to pick up - // updates from all the subchannels's FDs. - GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1); - } + grpc_fake_transport_security_credentials_create())) {} void SetUp() override { grpc_init(); @@ -615,6 +611,9 @@ TEST_F(ServiceConfigEnd2endTest, } // namespace grpc int main(int argc, char** argv) { + // Make the backup poller poll very frequently in order to pick up + // updates from all the subchannels's FDs. + GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1); ::testing::InitGoogleTest(&argc, argv); grpc::testing::TestEnvironment env(argc, argv); const auto result = RUN_ALL_TESTS(); diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index 87a231c588d..eac6f32a538 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -368,11 +368,7 @@ class XdsEnd2endTest : public ::testing::Test { num_backends_(num_backends), num_balancers_(num_balancers), client_load_reporting_interval_seconds_( - client_load_reporting_interval_seconds) { - // Make the backup poller poll very frequently in order to pick up - // updates from all the subchannels's FDs. - GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1); - } + client_load_reporting_interval_seconds) {} void SetUp() override { response_generator_ = @@ -1405,6 +1401,9 @@ class SingleBalancerWithClientLoadReportingTest : public XdsEnd2endTest { } // namespace grpc int main(int argc, char** argv) { + // Make the backup poller poll very frequently in order to pick up + // updates from all the subchannels's FDs. + GPR_GLOBAL_CONFIG_SET(grpc_client_channel_backup_poll_interval_ms, 1); grpc_init(); grpc::testing::TestEnvironment env(argc, argv); ::testing::InitGoogleTest(&argc, argv);