Chiebot-Mirror
/
grpc
mirror of https://github.com/grpc/grpc.git
8
0
Fork 0
Code Issues Projects Releases Wiki Activity

Add thread safety annotations (#26871)

Browse Source 
This exposes a bug in clang, reported upstream as
https://bugs.llvm.org/show_bug.cgi?id=51368.

The clang bug is mitigated using a fake scoped lock; that allows the
current code to compile while also serving as a change detector to
prevent it from going stale; if the compiler bug is fixed, the compiler
will see an overlapping locking requirement, and reject this code, which
will prompt a human being to remove this workaround.
pull/26936/head
Tamir Duberstein 4 years ago committed by GitHub
parent 3d4b89616d
commit 3fd01fabae
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
1 changed files with 63 additions and 26 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 89
      test/cpp/end2end/xds_end2end_test.cc

89
test/cpp/end2end/xds_end2end_test.cc
Unescape View File

@ -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 <typename RpcService>
@ -360,8 +360,8 @@ class BackendServiceImpl
private:
grpc_core::Mutex mu_;
std::set<std::string> clients_;
std::vector<std::string> last_peer_identity_;
std::set<std::string> clients_ ABSL_GUARDED_BY(mu_);
std::vector<std::string> last_peer_identity_ ABSL_GUARDED_BY(mu_);
};
class ClientStats {
@ -643,7 +643,7 @@ class AdsServiceImpl : public std::enable_shared_from_this<AdsServiceImpl> {
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<AdsServiceImpl> {
}
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<AdsServiceImpl> {
UpdateQueue* update_queue,
SubscriptionMap* subscription_map,
SentState* sent_state,
absl::optional<DiscoveryResponse>* response) {
absl::optional<DiscoveryResponse>* 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<AdsServiceImpl> {
void ProcessUpdate(const std::string& resource_type,
const std::string& resource_name,
SubscriptionMap* subscription_map, SentState* sent_state,
absl::optional<DiscoveryResponse>* response) {
absl::optional<DiscoveryResponse>* 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<AdsServiceImpl> {
const std::string& resource_type, const std::string& v2_resource_type,
const int version, const SubscriptionNameMap& subscription_name_map,
const std::set<std::string>& 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<AdsServiceImpl> {
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<std::string /* type_url */, ResponseState>
resource_type_response_state_;
std::set<std::string /*resource_type*/> resource_types_to_ignore_;
std::map<std::string /*resource_type*/, int> resource_type_min_versions_;
resource_type_response_state_ ABSL_GUARDED_BY(ads_mu_);
std::set<std::string /*resource_type*/> resource_types_to_ignore_
ABSL_GUARDED_BY(ads_mu_);
std::map<std::string /*resource_type*/, int> 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<std::string> clients_;
std::set<std::string> clients_ ABSL_GUARDED_BY(clients_mu_);
};
class LrsServiceImpl : public std::enable_shared_from_this<LrsServiceImpl> {
@ -1193,9 +1222,15 @@ class LrsServiceImpl : public std::enable_shared_from_this<LrsServiceImpl> {
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> {
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<LrsServiceImpl> {
std::set<std::string> 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<std::vector<ClientStats>> 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<std::vector<ClientStats>> result_queue_
ABSL_GUARDED_BY(load_report_mu_);
};
class TestType {
@ -2472,7 +2509,7 @@ class XdsEnd2endTest : public ::testing::TestWithParam<TestType> {
private:
grpc_core::Mutex mu_;
grpc_core::CondVar cond_;
std::map<std::string, grpc::Status> status_map;
std::map<std::string, grpc::Status> status_map ABSL_GUARDED_BY(mu_);
};
class ServerThread {

Write Preview
Loading…
Cancel
Save