[pick first] explicitly unset selected subchannel when promoting pending subchannel (#35865)

This change is needed in order to get certain LB-policy-unit-test-framework-based tests to pass, which involve updates on PF policies.

Without this change, in such tests, health watchers on the original subchannel can be left hanging around. The underlying reason is related to 842057d8d5/test/core/client_channel/lb_policy/lb_policy_test_lib.h (L203).

Related: cl/563857636

Closes #35865

COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35865 from apolcyn:update_pf_Test 1a5e6b5591
PiperOrigin-RevId: 607844309

src/core/load_balancing/pick_first/pick_first.cc

@@ -1022,6 +1022,7 @@ void PickFirst::SubchannelList::SubchannelData::ProcessUnselectedReadyLocked() {
               p, p->latest_pending_subchannel_list_.get(),
               p->subchannel_list_.get());
     }
+    p->UnsetSelectedSubchannel();
     p->subchannel_list_ = std::move(p->latest_pending_subchannel_list_);
   }
   // Cases 1 and 2.

test/core/client_channel/lb_policy/lb_policy_test_lib.h

@@ -698,8 +698,10 @@ class LoadBalancingPolicyTest : public ::testing::Test {
     const absl::optional<BackendMetricData> backend_metric_data_;
   };
 
-  explicit LoadBalancingPolicyTest(absl::string_view lb_policy_name)
-      : lb_policy_name_(lb_policy_name) {}
+  explicit LoadBalancingPolicyTest(absl::string_view lb_policy_name,
+                                   ChannelArgs channel_args = ChannelArgs())
+      : lb_policy_name_(lb_policy_name),
+        channel_args_(std::move(channel_args)) {}
 
   void SetUp() override {
     // Order is important here: Fuzzing EE needs to be created before
@@ -715,7 +717,7 @@ class LoadBalancingPolicyTest : public ::testing::Test {
     auto helper = std::make_unique<FakeHelper>(this);
     helper_ = helper.get();
     LoadBalancingPolicy::Args args = {work_serializer_, std::move(helper),
-                                      ChannelArgs()};
+                                      channel_args_};
     lb_policy_ =
         CoreConfiguration::Get().lb_policy_registry().CreateLoadBalancingPolicy(
             lb_policy_name_, std::move(args));
@@ -1491,6 +1493,7 @@ class LoadBalancingPolicyTest : public ::testing::Test {
   std::map<SubchannelKey, SubchannelState> subchannel_pool_;
   OrphanablePtr<LoadBalancingPolicy> lb_policy_;
   const absl::string_view lb_policy_name_;
+  const ChannelArgs channel_args_;
 };
 
 }  // namespace testing

test/core/client_channel/lb_policy/pick_first_test.cc

@@ -14,6 +14,8 @@
 // limitations under the License.
 //
 
+#include "src/core/load_balancing/pick_first/pick_first.h"
+
 #include <stddef.h>
 
 #include <algorithm>
@@ -54,7 +56,8 @@ namespace {
 
 class PickFirstTest : public LoadBalancingPolicyTest {
  protected:
-  PickFirstTest() : LoadBalancingPolicyTest("pick_first") {}
+  explicit PickFirstTest(ChannelArgs channel_args = ChannelArgs())
+      : LoadBalancingPolicyTest("pick_first", channel_args) {}
 
   void SetUp() override {
     LoadBalancingPolicyTest::SetUp();
@@ -1103,6 +1106,49 @@ TEST_F(PickFirstTest, ShufflingDisabled) {
     EXPECT_THAT(address_order, ::testing::ElementsAreArray(kAddresses));
   }
 }
+
+class PickFirstHealthCheckingEnabledTest : public PickFirstTest {
+ protected:
+  PickFirstHealthCheckingEnabledTest()
+      : PickFirstTest(ChannelArgs().Set(
+            GRPC_ARG_INTERNAL_PICK_FIRST_ENABLE_HEALTH_CHECKING, true)) {}
+};
+
+TEST_F(PickFirstHealthCheckingEnabledTest, UpdateWithReadyChannel) {
+  constexpr absl::string_view kAddress = "ipv4:127.0.0.1:443";
+  LoadBalancingPolicy::UpdateArgs update =
+      BuildUpdate({kAddress}, MakePickFirstConfig());
+  absl::Status status = ApplyUpdate(update, lb_policy());
+  EXPECT_TRUE(status.ok()) << status;
+  // LB policy should have created a subchannel for the address.
+  auto* subchannel = FindSubchannel(kAddress);
+  ASSERT_NE(subchannel, nullptr);
+  // When the LB policy receives the first subchannel's initial connectivity
+  // state notification (IDLE), it will request a connection.
+  EXPECT_TRUE(subchannel->ConnectionRequested());
+  // This causes the subchannel to start to connect, so it reports CONNECTING.
+  subchannel->SetConnectivityState(GRPC_CHANNEL_CONNECTING);
+  // LB policy should have reported CONNECTING state.
+  ExpectConnectingUpdate();
+  // When the subchannel becomes connected, it reports READY.
+  subchannel->SetConnectivityState(GRPC_CHANNEL_READY);
+  // The LB policy will report CONNECTING some number of times (doesn't
+  // matter how many) and then report READY.
+  auto picker = WaitForConnected();
+  ASSERT_NE(picker, nullptr);
+  EXPECT_EQ(ExpectPickComplete(picker.get()), kAddress);
+  // Reapply the same update we did before. The the underlying
+  // subchannel will immediately become ready.
+  status =
+      ApplyUpdate(BuildUpdate({kAddress}, MakePickFirstConfig()), lb_policy());
+  EXPECT_TRUE(status.ok()) << status;
+  picker = ExpectState(GRPC_CHANNEL_READY);
+  EXPECT_EQ(ExpectPickComplete(picker.get()), kAddress);
+  // At this point, NumWatchers() should account for our
+  // subchannel connectivity watcher and our health watcher.
+  EXPECT_EQ(subchannel->NumWatchers(), 2);
+}
+
 }  // namespace
 }  // namespace testing
 }  // namespace grpc_core