Reviewer comments

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

@@ -310,7 +310,6 @@ class LoadBalancingPolicy : public InternallyRefCounted<LoadBalancingPolicy> {
   /// Args used to instantiate an LB policy.
   struct Args {
     /// The work_serializer under which all LB policy calls will be run.
-    /// Policy does NOT take ownership of the reference to the work_serializer.
     std::shared_ptr<WorkSerializer> work_serializer;
     /// Channel control helper.
     /// Note: LB policies MUST NOT call any method on the helper from
@@ -396,7 +395,7 @@ class LoadBalancingPolicy : public InternallyRefCounted<LoadBalancingPolicy> {
   virtual void ShutdownLocked() = 0;
 
  private:
-  /// Logical Thread under which LB policy actions take place.
+  /// Work Serializer under which LB policy actions take place.
   std::shared_ptr<WorkSerializer> work_serializer_;
   /// Owned pointer to interested parties in load balancing decisions.
   grpc_pollset_set* interested_parties_;

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

@@ -122,8 +122,8 @@ class Resolver : public InternallyRefCounted<Resolver> {
   }
 
  protected:
-  explicit Resolver(std::shared_ptr<WorkSerializer> work_serializer,
-                    std::unique_ptr<ResultHandler> result_handler);
+  Resolver(std::shared_ptr<WorkSerializer> work_serializer,
+           std::unique_ptr<ResultHandler> result_handler);
 
   /// Shuts down the resolver.
   virtual void ShutdownLocked() = 0;

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc

@@ -372,8 +372,9 @@ void grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
     }
     port.reset(gpr_strdup(default_port));
   }
-  error = grpc_ares_ev_driver_create_locked(
-      &r->ev_driver, interested_parties, query_timeout_ms, work_serializer, r);
+  error = grpc_ares_ev_driver_create_locked(&r->ev_driver, interested_parties,
+                                            query_timeout_ms,
+                                            std::move(work_serializer), r);
   if (error != GRPC_ERROR_NONE) goto error_cleanup;
   channel = grpc_ares_ev_driver_get_channel_locked(r->ev_driver);
   // If dns_server is specified, use it.
@@ -624,7 +625,7 @@ static grpc_ares_request* grpc_dns_lookup_ares_locked_impl(
   // Look up name using c-ares lib.
   grpc_dns_lookup_ares_continue_after_check_localhost_and_ip_literals_locked(
       r, dns_server, name, default_port, interested_parties, check_grpclb,
-      query_timeout_ms, work_serializer);
+      query_timeout_ms, std::move(work_serializer));
   return r;
 }
 

src/core/ext/filters/client_channel/resolver/sockaddr/sockaddr_resolver.cc

@@ -57,7 +57,7 @@ class SockaddrResolver : public Resolver {
 
 SockaddrResolver::SockaddrResolver(ServerAddressList addresses,
                                    ResolverArgs args)
-    : Resolver(args.work_serializer, std::move(args.result_handler)),
+    : Resolver(std::move(args.work_serializer), std::move(args.result_handler)),
       addresses_(std::move(addresses)),
       channel_args_(grpc_channel_args_copy(args.args)) {}
 

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc

@@ -33,7 +33,8 @@ namespace {
 class XdsResolver : public Resolver {
  public:
   explicit XdsResolver(ResolverArgs args)
-      : Resolver(args.work_serializer, std::move(args.result_handler)),
+      : Resolver(std::move(args.work_serializer),
+                 std::move(args.result_handler)),
         args_(grpc_channel_args_copy(args.args)),
         interested_parties_(args.pollset_set) {
     char* path = args.uri->path;

src/core/ext/filters/client_channel/xds/xds_api.h

@@ -151,7 +151,7 @@ class XdsDropConfig : public RefCounted<XdsDropConfig> {
         DropCategory{std::move(name), parts_per_million});
   }
 
-  // The only method invoked from the data plane combiner.
+  // The only method invoked from the data plane mutex.
   bool ShouldDrop(const std::string** category_name) const;
 
   const DropCategoryList& drop_category_list() const {

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

@@ -179,26 +179,27 @@ class XdsClient::ChannelState::AdsCallState
             &msg,
             "timeout obtaining resource {type=%s name=%s} from xds server",
             type_url_.c_str(), name_.c_str());
-        grpc_error* error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg);
+        grpc_error* watcher_error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(msg);
         gpr_free(msg);
         if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) {
           gpr_log(GPR_INFO, "[xds_client %p] %s", ads_calld_->xds_client(),
-                  grpc_error_string(error));
+                  grpc_error_string(watcher_error));
         }
         if (type_url_ == kLdsTypeUrl || type_url_ == kRdsTypeUrl) {
-          ads_calld_->xds_client()->service_config_watcher_->OnError(error);
+          ads_calld_->xds_client()->service_config_watcher_->OnError(
+              watcher_error);
         } else if (type_url_ == kCdsTypeUrl) {
           ClusterState& state = ads_calld_->xds_client()->cluster_map_[name_];
           for (const auto& p : state.watchers) {
-            p.first->OnError(GRPC_ERROR_REF(error));
+            p.first->OnError(GRPC_ERROR_REF(watcher_error));
           }
-          GRPC_ERROR_UNREF(error);
+          GRPC_ERROR_UNREF(watcher_error);
         } else if (type_url_ == kEdsTypeUrl) {
           EndpointState& state = ads_calld_->xds_client()->endpoint_map_[name_];
           for (const auto& p : state.watchers) {
-            p.first->OnError(GRPC_ERROR_REF(error));
+            p.first->OnError(GRPC_ERROR_REF(watcher_error));
           }
-          GRPC_ERROR_UNREF(error);
+          GRPC_ERROR_UNREF(watcher_error);
         } else {
           GPR_UNREACHABLE_CODE(return );
         }
@@ -241,8 +242,8 @@ class XdsClient::ChannelState::AdsCallState
   static void OnRequestSent(void* arg, grpc_error* error);
   void OnRequestSentLocked(grpc_error* error);
   static void OnResponseReceived(void* arg, grpc_error* error);
-  static void OnStatusReceived(void* arg, grpc_error* error);
   void OnResponseReceivedLocked();
+  static void OnStatusReceived(void* arg, grpc_error* error);
   void OnStatusReceivedLocked(grpc_error* error);
 
   bool IsCurrentCallOnChannel() const;
@@ -342,10 +343,10 @@ class XdsClient::ChannelState::LrsCallState
   };
 
   static void OnInitialRequestSent(void* arg, grpc_error* error);
-  static void OnResponseReceived(void* arg, grpc_error* error);
-  static void OnStatusReceived(void* arg, grpc_error* error);
   void OnInitialRequestSentLocked();
+  static void OnResponseReceived(void* arg, grpc_error* error);
   void OnResponseReceivedLocked();
+  static void OnStatusReceived(void* arg, grpc_error* error);
   void OnStatusReceivedLocked(grpc_error* error);
 
   bool IsCurrentCallOnChannel() const;