diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index e2a0fed5f50..3daf1a43e19 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -306,8 +306,8 @@ class CountedService : public ServiceType { private: grpc_core::Mutex mu_; - size_t request_count_ = 0; - size_t response_count_ = 0; + size_t request_count_ ABSL_GUARDED_BY(mu_) = 0; + size_t response_count_ ABSL_GUARDED_BY(mu_) = 0; }; template @@ -360,8 +360,8 @@ class BackendServiceImpl private: grpc_core::Mutex mu_; - std::set clients_; - std::vector last_peer_identity_; + std::set clients_ ABSL_GUARDED_BY(mu_); + std::vector last_peer_identity_ ABSL_GUARDED_BY(mu_); }; class ClientStats { @@ -643,7 +643,7 @@ class AdsServiceImpl : public std::enable_shared_from_this { NotifyDoneWithAdsCallLocked(); } - void NotifyDoneWithAdsCallLocked() { + void NotifyDoneWithAdsCallLocked() ABSL_EXCLUSIVE_LOCKS_REQUIRED(ads_mu_) { if (!ads_done_) { ads_done_ = true; ads_cond_.SignalAll(); @@ -830,6 +830,28 @@ class AdsServiceImpl : public std::enable_shared_from_this { } private: + // NB: clang's annotalysis is confused by the use of inner template + // classes here and *ignores* the exclusive lock annotation on some + // functions. See https://bugs.llvm.org/show_bug.cgi?id=51368. + // + // This class is used for a dual purpose: + // - it convinces clang that the lock is held in a given scope + // - when used in a function that is annotated to require the inner lock it + // will cause compilation to fail if the upstream bug is fixed! + // + // If you arrive here because of a compilation failure, that might mean the + // clang bug is fixed! Please report that on the ticket. + // + // Since the buggy compiler will still need to be supported, consider + // wrapping this class in a compiler version #if and replace its usage + // with a macro whose expansion is conditional on the compiler version. In + // time (years? decades?) this code can be deleted altogether. + class ABSL_SCOPED_LOCKABLE NoopMutexLock { + public: + explicit NoopMutexLock(grpc_core::Mutex& mu) + ABSL_EXCLUSIVE_LOCK_FUNCTION(mu) {} + ~NoopMutexLock() ABSL_UNLOCK_FUNCTION() {} + }; // Processes a response read from the client. // Populates response if needed. void ProcessRequest(const DiscoveryRequest& request, @@ -837,7 +859,9 @@ class AdsServiceImpl : public std::enable_shared_from_this { UpdateQueue* update_queue, SubscriptionMap* subscription_map, SentState* sent_state, - absl::optional* response) { + absl::optional* response) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_->ads_mu_) { + NoopMutexLock mu(parent_->ads_mu_); // Check the nonce sent by the client, if any. // (This will be absent on the first request on a stream.) if (request.response_nonce().empty()) { @@ -936,7 +960,9 @@ class AdsServiceImpl : public std::enable_shared_from_this { void ProcessUpdate(const std::string& resource_type, const std::string& resource_name, SubscriptionMap* subscription_map, SentState* sent_state, - absl::optional* response) { + absl::optional* response) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_->ads_mu_) { + NoopMutexLock mu(parent_->ads_mu_); const std::string v2_resource_type = TypeUrlToV2(resource_type); gpr_log(GPR_INFO, "ADS[%p]: Received update for type=%s name=%s", this, resource_type.c_str(), resource_name.c_str()); @@ -996,7 +1022,9 @@ class AdsServiceImpl : public std::enable_shared_from_this { const std::string& resource_type, const std::string& v2_resource_type, const int version, const SubscriptionNameMap& subscription_name_map, const std::set& resources_added_to_response, - SentState* sent_state, DiscoveryResponse* response) { + SentState* sent_state, DiscoveryResponse* response) + ABSL_EXCLUSIVE_LOCKS_REQUIRED(parent_->ads_mu_) { + NoopMutexLock mu(parent_->ads_mu_); auto& response_state = parent_->resource_type_response_state_[resource_type]; if (response_state.state == ResponseState::NOT_SENT) { @@ -1140,22 +1168,23 @@ class AdsServiceImpl : public std::enable_shared_from_this { std::atomic_bool seen_v3_client_{false}; grpc_core::CondVar ads_cond_; - // Protect the members below. grpc_core::Mutex ads_mu_; - bool ads_done_ = false; + bool ads_done_ ABSL_GUARDED_BY(ads_mu_) = false; std::map - resource_type_response_state_; - std::set resource_types_to_ignore_; - std::map resource_type_min_versions_; + resource_type_response_state_ ABSL_GUARDED_BY(ads_mu_); + std::set resource_types_to_ignore_ + ABSL_GUARDED_BY(ads_mu_); + std::map resource_type_min_versions_ + ABSL_GUARDED_BY(ads_mu_); // An instance data member containing the current state of all resources. // Note that an entry will exist whenever either of the following is true: // - The resource exists (i.e., has been created by SetResource() and has not // yet been destroyed by UnsetResource()). // - There is at least one subscription for the resource. - ResourceMap resource_map_; + ResourceMap resource_map_ ABSL_GUARDED_BY(ads_mu_); grpc_core::Mutex clients_mu_; - std::set clients_; + std::set clients_ ABSL_GUARDED_BY(clients_mu_); }; class LrsServiceImpl : public std::enable_shared_from_this { @@ -1193,9 +1222,15 @@ class LrsServiceImpl : public std::enable_shared_from_this { cluster_names_ = cluster_names; } - void Start() { - lrs_done_ = false; - result_queue_.clear(); + void Start() ABSL_LOCKS_EXCLUDED(lrs_mu_, load_report_mu_) { + { + grpc_core::MutexLock lock(&lrs_mu_); + lrs_done_ = false; + } + { + grpc_core::MutexLock lock(&load_report_mu_); + result_queue_.clear(); + } } void Shutdown() { @@ -1292,7 +1327,7 @@ class LrsServiceImpl : public std::enable_shared_from_this { LrsServiceImpl* parent_; }; - void NotifyDoneWithLrsCallLocked() { + void NotifyDoneWithLrsCallLocked() ABSL_EXCLUSIVE_LOCKS_REQUIRED(lrs_mu_) { if (!lrs_done_) { lrs_done_ = true; lrs_cv_.SignalAll(); @@ -1313,12 +1348,14 @@ class LrsServiceImpl : public std::enable_shared_from_this { std::set cluster_names_; grpc_core::CondVar lrs_cv_; - grpc_core::Mutex lrs_mu_; // Protects lrs_done_. - bool lrs_done_ = false; - - grpc_core::Mutex load_report_mu_; // Protects the members below. - grpc_core::CondVar* load_report_cond_ = nullptr; - std::deque> result_queue_; + grpc_core::Mutex lrs_mu_; + bool lrs_done_ ABSL_GUARDED_BY(lrs_mu_) = false; + + grpc_core::Mutex load_report_mu_; + grpc_core::CondVar* load_report_cond_ ABSL_GUARDED_BY(load_report_mu_) = + nullptr; + std::deque> result_queue_ + ABSL_GUARDED_BY(load_report_mu_); }; class TestType { @@ -2472,7 +2509,7 @@ class XdsEnd2endTest : public ::testing::TestWithParam { private: grpc_core::Mutex mu_; grpc_core::CondVar cond_; - std::map status_map; + std::map status_map ABSL_GUARDED_BY(mu_); }; class ServerThread {