Merge remote-tracking branch 'upstream/master' into release

pull/24259/head
Donna Dionne 4 years ago
commit a484be74c8
  1. 2
      .github/ISSUE_TEMPLATE/bug_report.md
  2. 2
      .github/ISSUE_TEMPLATE/cleanup_request.md
  3. 2
      .github/ISSUE_TEMPLATE/feature_request.md
  4. 2
      .github/ISSUE_TEMPLATE/question.md
  5. 2
      .github/pull_request_template.md
  6. 3
      src/core/ext/filters/client_channel/client_channel.cc
  7. 4
      src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc
  8. 23
      src/core/lib/channel/channelz_registry.cc
  9. 14
      src/core/lib/gprpp/ref_counted.h
  10. 34
      test/core/gprpp/ref_counted_test.cc

@ -2,7 +2,7 @@
name: Report a bug
about: Create a report to help us improve
labels: kind/bug, priority/P2
assignees: yashykt
assignees: veblush
---

@ -2,7 +2,7 @@
name: Request a cleanup
about: Suggest a cleanup in our repository
labels: kind/internal cleanup, priority/P2
assignees: yashykt
assignees: veblush
---

@ -2,7 +2,7 @@
name: Request a feature
about: Suggest an idea for this project
labels: kind/enhancement, priority/P2
assignees: yashykt
assignees: veblush
---

@ -2,7 +2,7 @@
name: Ask a question
about: Ask a question
labels: kind/question, priority/P3
assignees: yashykt
assignees: veblush
---

@ -8,4 +8,4 @@ If you know who should review your pull request, please remove the mentioning be
-->
@yashykt
@veblush

@ -2211,7 +2211,8 @@ void CallData::Destroy(grpc_call_element* elem,
if (GPR_LIKELY(subchannel_call != nullptr)) {
subchannel_call->SetAfterCallStackDestroy(then_schedule_closure);
} else {
Closure::Run(DEBUG_LOCATION, then_schedule_closure, GRPC_ERROR_NONE);
// TODO(yashkt) : This can potentially be a Closure::Run
ExecCtx::Run(DEBUG_LOCATION, then_schedule_closure, GRPC_ERROR_NONE);
}
}

@ -681,8 +681,8 @@ void XdsResolver::GenerateResult() {
void XdsResolver::MaybeRemoveUnusedClusters() {
bool update_needed = false;
for (auto it = cluster_state_map_.begin(); it != cluster_state_map_.end();) {
if (it->second->RefIfNonZero()) {
it->second->Unref();
RefCountedPtr<ClusterState> cluster_state = it->second->RefIfNonZero();
if (cluster_state != nullptr) {
++it;
} else {
update_needed = true;

@ -78,8 +78,7 @@ RefCountedPtr<BaseNode> ChannelzRegistry::InternalGet(intptr_t uuid) {
// Found node. Return only if its refcount is not zero (i.e., when we
// know that there is no other thread about to destroy it).
BaseNode* node = it->second;
if (!node->RefIfNonZero()) return nullptr;
return RefCountedPtr<BaseNode>(node);
return node->RefIfNonZero();
}
std::string ChannelzRegistry::InternalGetTopChannels(
@ -91,8 +90,9 @@ std::string ChannelzRegistry::InternalGetTopChannels(
for (auto it = node_map_.lower_bound(start_channel_id);
it != node_map_.end(); ++it) {
BaseNode* node = it->second;
RefCountedPtr<BaseNode> node_ref;
if (node->type() == BaseNode::EntityType::kTopLevelChannel &&
node->RefIfNonZero()) {
(node_ref = node->RefIfNonZero()) != nullptr) {
// Check if we are over pagination limit to determine if we need to set
// the "end" element. If we don't go through this block, we know that
// when the loop terminates, we have <= to kPaginationLimit.
@ -100,10 +100,10 @@ std::string ChannelzRegistry::InternalGetTopChannels(
// refcount, we need to decrease it, but we can't unref while
// holding the lock, because this may lead to a deadlock.
if (top_level_channels.size() == kPaginationLimit) {
node_after_pagination_limit.reset(node);
node_after_pagination_limit = std::move(node_ref);
break;
}
top_level_channels.emplace_back(node);
top_level_channels.emplace_back(std::move(node_ref));
}
}
}
@ -129,8 +129,9 @@ std::string ChannelzRegistry::InternalGetServers(intptr_t start_server_id) {
for (auto it = node_map_.lower_bound(start_server_id);
it != node_map_.end(); ++it) {
BaseNode* node = it->second;
RefCountedPtr<BaseNode> node_ref;
if (node->type() == BaseNode::EntityType::kServer &&
node->RefIfNonZero()) {
(node_ref = node->RefIfNonZero()) != nullptr) {
// Check if we are over pagination limit to determine if we need to set
// the "end" element. If we don't go through this block, we know that
// when the loop terminates, we have <= to kPaginationLimit.
@ -138,10 +139,10 @@ std::string ChannelzRegistry::InternalGetServers(intptr_t start_server_id) {
// refcount, we need to decrease it, but we can't unref while
// holding the lock, because this may lead to a deadlock.
if (servers.size() == kPaginationLimit) {
node_after_pagination_limit.reset(node);
node_after_pagination_limit = std::move(node_ref);
break;
}
servers.emplace_back(node);
servers.emplace_back(std::move(node_ref));
}
}
}
@ -164,9 +165,9 @@ void ChannelzRegistry::InternalLogAllEntities() {
{
MutexLock lock(&mu_);
for (auto& p : node_map_) {
BaseNode* node = p.second;
if (node->RefIfNonZero()) {
nodes.emplace_back(node);
RefCountedPtr<BaseNode> node = p.second->RefIfNonZero();
if (node != nullptr) {
nodes.emplace_back(std::move(node));
}
}
}

@ -242,7 +242,7 @@ class Delete<T, false> {
// must be tracked in a registry but the object's entry in the registry
// cannot be removed from the object's dtor due to synchronization issues.
// In this case, the registry can be cleaned up later by identifying
// entries for which RefIfNonZero() returns false.
// entries for which RefIfNonZero() returns null.
//
// This will commonly be used by CRTP (curiously-recurring template pattern)
// e.g., class MyClass : public RefCounted<MyClass>
@ -299,9 +299,15 @@ class RefCounted : public Impl {
}
}
bool RefIfNonZero() { return refs_.RefIfNonZero(); }
bool RefIfNonZero(const DebugLocation& location, const char* reason) {
return refs_.RefIfNonZero(location, reason);
RefCountedPtr<Child> RefIfNonZero() GRPC_MUST_USE_RESULT {
return RefCountedPtr<Child>(refs_.RefIfNonZero() ? static_cast<Child*>(this)
: nullptr);
}
RefCountedPtr<Child> RefIfNonZero(const DebugLocation& location,
const char* reason) GRPC_MUST_USE_RESULT {
return RefCountedPtr<Child>(refs_.RefIfNonZero(location, reason)
? static_cast<Child*>(this)
: nullptr);
}
// Not copyable nor movable.

@ -53,8 +53,8 @@ TEST(RefCounted, ExtraRef) {
class Value : public RefCounted<Value, PolymorphicRefCount, false> {
public:
Value(int value, std::set<Value*>* registry) : value_(value) {
registry->insert(this);
Value(int value, std::set<std::unique_ptr<Value>>* registry) : value_(value) {
registry->emplace(this);
}
int value() const { return value_; }
@ -63,41 +63,41 @@ class Value : public RefCounted<Value, PolymorphicRefCount, false> {
int value_;
};
void GarbageCollectRegistry(std::set<Value*>* registry) {
void GarbageCollectRegistry(std::set<std::unique_ptr<Value>>* registry) {
for (auto it = registry->begin(); it != registry->end();) {
Value* v = *it;
RefCountedPtr<Value> v = (*it)->RefIfNonZero();
// Check if the object has any refs remaining.
if (v->RefIfNonZero()) {
if (v != nullptr) {
// It has refs remaining, so we do not delete it.
v->Unref(); // Remove the ref we just added.
++it;
} else {
// No refs remaining, so delete it and remove from registry.
delete v;
// No refs remaining, so remove it from the registry.
it = registry->erase(it);
}
}
}
TEST(RefCounted, NoDeleteUponUnref) {
std::set<Value*> registry;
std::set<std::unique_ptr<Value>> registry;
// Add two objects to the registry.
auto v1 = MakeRefCounted<Value>(1, &registry);
auto v2 = MakeRefCounted<Value>(2, &registry);
EXPECT_THAT(registry, ::testing::UnorderedElementsAre(
::testing::Property(&Value::value, 1),
::testing::Property(&Value::value, 2)));
EXPECT_THAT(registry,
::testing::UnorderedElementsAre(
::testing::Pointee(::testing::Property(&Value::value, 1)),
::testing::Pointee(::testing::Property(&Value::value, 2))));
// Running garbage collection should not delete anything, since both
// entries still have refs.
GarbageCollectRegistry(&registry);
EXPECT_THAT(registry, ::testing::UnorderedElementsAre(
::testing::Property(&Value::value, 1),
::testing::Property(&Value::value, 2)));
EXPECT_THAT(registry,
::testing::UnorderedElementsAre(
::testing::Pointee(::testing::Property(&Value::value, 1)),
::testing::Pointee(::testing::Property(&Value::value, 2))));
// Unref v2 and run GC to remove it.
v2.reset();
GarbageCollectRegistry(&registry);
EXPECT_THAT(registry, ::testing::UnorderedElementsAre(
::testing::Property(&Value::value, 1)));
EXPECT_THAT(registry, ::testing::UnorderedElementsAre(::testing::Pointee(
::testing::Property(&Value::value, 1))));
// Now unref v1 and run GC again.
v1.reset();
GarbageCollectRegistry(&registry);

Loading…
Cancel
Save