Reviewer comments

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

@@ -1022,8 +1022,7 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface {
       if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {
         gpr_log(GPR_INFO,
                 "chand=%p: connectivity change for subchannel wrapper %p "
-                "subchannel %p; "
+                "subchannel %p; hopping into work_serializer",
-                "hopping into work_serializer",
                 parent_->chand_, parent_.get(), parent_->subchannel_);
       }
       // Will delete itself.
@@ -1066,16 +1065,16 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface {
                   parent_->parent_->chand_, parent_->parent_.get(),
                   parent_->parent_->subchannel_, parent_->watcher_.get());
         }
-        while (true) {
+        grpc_connectivity_state state;
-          grpc_connectivity_state state;
+        RefCountedPtr<ConnectedSubchannel> connected_subchannel;
-          RefCountedPtr<ConnectedSubchannel> connected_subchannel;
+        if (!parent_->PopConnectivityStateChange(&state,
-          if (!parent_->PopConnectivityStateChange(&state,
+                                                 &connected_subchannel)) {
-                                                   &connected_subchannel)) {
+          // There should be atleast one connectivity change in the queue.
-            break;
+          GPR_DEBUG_ASSERT(false);
-          }
+        }
-          // Ignore update if the parent WatcherWrapper has been replaced
+        // Ignore update if the parent WatcherWrapper has been replaced
-          // since this callback was scheduled.
+        // since this callback was scheduled.
-          if (parent_->watcher_ == nullptr) continue;
+        if (parent_->watcher_ != nullptr) {
           parent_->last_seen_state_ = state;
           parent_->parent_->MaybeUpdateConnectedSubchannel(
               std::move(connected_subchannel));

src/core/ext/filters/client_channel/subchannel.h

@@ -185,17 +185,17 @@ class Subchannel {
     // Will be invoked whenever the subchannel's connectivity state
     // changes.  There will be only one invocation of this method on a
     // given watcher instance at any given time.
-    //
+    // Implementations should call PopConnectivityStateChange to get the next
-    // When the state changes to READY, connected_subchannel will
+    // connectivity state change.
-    // contain a ref to the connected subchannel.  When it changes from
+    virtual void OnConnectivityStateChange() = 0;
-    // READY to some other state, the implementation must release its
-    // ref to the connected subchannel.
-    virtual void OnConnectivityStateChange()  // NOLINT
-        = 0;
 
     virtual grpc_pollset_set* interested_parties() = 0;
 
     // Enqueues connectivity state change notifications.
+    // When the state changes to READY, connected_subchannel will
+    // contain a ref to the connected subchannel.  When it changes from
+    // READY to some other state, the implementation must release its
+    // ref to the connected subchannel.
     // TODO(yashkt): This is currently needed to send the state updates in the
     // right order when asynchronously notifying. This will no longer be
     // necessary when we have access to EventManager.

test/core/client_channel/resolvers/dns_resolver_connectivity_test.cc

@@ -66,8 +66,9 @@ static grpc_ares_request* my_dns_lookup_ares_locked(
     std::unique_ptr<grpc_core::ServerAddressList>* addresses,
     bool /*check_grpclb*/, char** /*service_config_json*/,
     int /*query_timeout_ms*/,
-    std::shared_ptr<grpc_core::WorkSerializer> /* work_serializer */) { /* NOLINT
+    std::shared_ptr<
-                                                                         */
+        grpc_core::WorkSerializer> /* work_serializer */) { /* NOLINT
+                                                             */
   gpr_mu_lock(&g_mu);
   GPR_ASSERT(0 == strcmp("test", addr));
   grpc_error* error = GRPC_ERROR_NONE;