Merge pull request #16651 from AspirinSJL/improve_force_creation

Change force_creation from atm to bool

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

@@ -42,7 +42,7 @@ struct grpc_subchannel_key {
   grpc_subchannel_args args;
 };
 
-static gpr_atm g_force_creation = false;
+static bool g_force_creation = false;
 
 static grpc_subchannel_key* create_key(
     const grpc_subchannel_args* args,
@@ -74,7 +74,7 @@ static grpc_subchannel_key* subchannel_key_copy(grpc_subchannel_key* k) {
 int grpc_subchannel_key_compare(const grpc_subchannel_key* a,
                                 const grpc_subchannel_key* b) {
   // To pretend the keys are different, return a non-zero value.
-  if (GPR_UNLIKELY(gpr_atm_no_barrier_load(&g_force_creation))) return 1;
+  if (GPR_UNLIKELY(g_force_creation)) return 1;
   int c = GPR_ICMP(a->args.filter_count, b->args.filter_count);
   if (c != 0) return c;
   if (a->args.filter_count > 0) {
@@ -251,5 +251,5 @@ void grpc_subchannel_index_unregister(grpc_subchannel_key* key,
 }
 
 void grpc_subchannel_index_test_only_set_force_creation(bool force_creation) {
-  gpr_atm_no_barrier_store(&g_force_creation, force_creation);
+  g_force_creation = force_creation;
 }

test/cpp/end2end/client_lb_end2end_test.cc

@@ -119,6 +119,7 @@ class ClientLbEnd2endTest : public ::testing::Test {
   }
 
   void SetUp() override {
+    grpc_init();
     response_generator_ =
         grpc_core::MakeRefCounted<grpc_core::FakeResolverResponseGenerator>();
   }
@@ -127,6 +128,7 @@ class ClientLbEnd2endTest : public ::testing::Test {
     for (size_t i = 0; i < servers_.size(); ++i) {
       servers_[i]->Shutdown();
     }
+    grpc_shutdown();
   }
 
   void CreateServers(size_t num_servers,
@@ -560,7 +562,23 @@ TEST_F(ClientLbEnd2endTest, PickFirstUpdateSuperset) {
   EXPECT_EQ("pick_first", channel->GetLoadBalancingPolicyName());
 }
 
-TEST_F(ClientLbEnd2endTest, PickFirstManyUpdates) {
+class ClientLbEnd2endWithParamTest
+    : public ClientLbEnd2endTest,
+      public ::testing::WithParamInterface<bool> {
+ protected:
+  void SetUp() override {
+    grpc_subchannel_index_test_only_set_force_creation(GetParam());
+    ClientLbEnd2endTest::SetUp();
+  }
+
+  void TearDown() override {
+    ClientLbEnd2endTest::TearDown();
+    grpc_subchannel_index_test_only_set_force_creation(false);
+  }
+};
+
+TEST_P(ClientLbEnd2endWithParamTest, PickFirstManyUpdates) {
+  gpr_log(GPR_INFO, "subchannel force creation: %d", GetParam());
   // Start servers and send one RPC per server.
   const int kNumServers = 3;
   StartServers(kNumServers);
@@ -570,20 +588,21 @@ TEST_F(ClientLbEnd2endTest, PickFirstManyUpdates) {
   for (size_t i = 0; i < servers_.size(); ++i) {
     ports.emplace_back(servers_[i]->port_);
   }
-  for (const bool force_creation : {true, false}) {
-    grpc_subchannel_index_test_only_set_force_creation(force_creation);
-    gpr_log(GPR_INFO, "Force subchannel creation: %d", force_creation);
-    for (size_t i = 0; i < 1000; ++i) {
-      std::shuffle(ports.begin(), ports.end(),
-                   std::mt19937(std::random_device()()));
-      SetNextResolution(ports);
-      if (i % 10 == 0) CheckRpcSendOk(stub, DEBUG_LOCATION);
-    }
+  for (size_t i = 0; i < 1000; ++i) {
+    std::shuffle(ports.begin(), ports.end(),
+                 std::mt19937(std::random_device()()));
+    SetNextResolution(ports);
+    // We should re-enter core at the end of the loop to give the resolution
+    // setting closure a chance to run.
+    if ((i + 1) % 10 == 0) CheckRpcSendOk(stub, DEBUG_LOCATION);
   }
   // Check LB policy name for the channel.
   EXPECT_EQ("pick_first", channel->GetLoadBalancingPolicyName());
 }
 
+INSTANTIATE_TEST_CASE_P(SubchannelForceCreation, ClientLbEnd2endWithParamTest,
+                        ::testing::Bool());
+
 TEST_F(ClientLbEnd2endTest, PickFirstReresolutionNoSelected) {
   // Prepare the ports for up servers and down servers.
   const int kNumServers = 3;
@@ -984,8 +1003,6 @@ TEST_F(ClientLbEnd2endTest, RoundRobinSingleReconnect) {
 int main(int argc, char** argv) {
   ::testing::InitGoogleTest(&argc, argv);
   grpc_test_init(argc, argv);
-  grpc_init();
   const auto result = RUN_ALL_TESTS();
-  grpc_shutdown();
   return result;
 }