Merge pull request #19136 from veblush/global-config-best

Applied the best practice using global configuration

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;
   }

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);
 

src/core/ext/filters/client_channel/client_channel_plugin.cc

@@ -24,6 +24,7 @@
 
 #include <grpc/support/alloc.h>
 
+#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) {

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<char> 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::ResolverFactory>(
             grpc_core::New<grpc_core::AresDnsResolverFactory>()));
+  } else {
+    g_use_ares_dns_resolver = false;
   }
 }
 
 void grpc_resolver_dns_ares_shutdown() {
-  grpc_core::UniquePtr<char> 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();
   }

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:

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; }

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();

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);

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();

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);