You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by to...@apache.org on 2016/02/18 03:15:58 UTC
[2/4] incubator-kudu git commit: mem_tracker: fix a potential
bad_weak_ptr
mem_tracker: fix a potential bad_weak_ptr
This fixes the following race:
T1 is calling MemTracker::FindOrCreateTracker for a tracker that does
not yet exist:
- T1 registers the new MemTracker as a child in the root tracker's child list.
This was a raw pointer, prior to this patch.
- T1 constructs a shared_ptr, incrementing the ref count to 1.
- T1 returns the shared_ptr to the user.
- The user drops the reference to the shared_ptr back to 0, such that it is
being deleted. The destructor is called, and the thread is switched out
before the raw pointer can be removed from the root tracker's child list.
T2 now runs, where a caller is trying to FindOrCreateTracker() with the
same identifier:
- T2 looks in the child tracker and sees a matching MemTracker instance.
- T2 tries to call tracker->shared_from_this(), but the refcount is 0,
so it throws bad_weak_ptr.
The fix is to use proper std::weak_ptrs for the child list instead of
raw pointers. A new unit test reproduces the bug consistently.
Change-Id: I1e1c225d6670025a9b4729013747652a30dfe608
Reviewed-on: http://gerrit.cloudera.org:8080/2212
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-kudu/commit/e284a93c
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/e284a93c
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/e284a93c
Branch: refs/heads/master
Commit: e284a93c40740ad8bd3b5b95ee31e98c24aa4a6f
Parents: 4440077
Author: Todd Lipcon <to...@apache.org>
Authored: Wed Feb 17 16:13:34 2016 -0800
Committer: Todd Lipcon <to...@apache.org>
Committed: Thu Feb 18 02:12:04 2016 +0000
----------------------------------------------------------------------
src/kudu/util/mem_tracker-test.cc | 20 ++++++++++++++++++++
src/kudu/util/mem_tracker.cc | 28 ++++++++++++++++++----------
src/kudu/util/mem_tracker.h | 8 ++++----
3 files changed, 42 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/e284a93c/src/kudu/util/mem_tracker-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/mem_tracker-test.cc b/src/kudu/util/mem_tracker-test.cc
index 2a10bed..4c1b244 100644
--- a/src/kudu/util/mem_tracker-test.cc
+++ b/src/kudu/util/mem_tracker-test.cc
@@ -17,7 +17,9 @@
#include "kudu/util/mem_tracker.h"
+#include <atomic>
#include <string>
+#include <thread>
#include <unordered_map>
#include <utility>
#include <vector>
@@ -337,4 +339,22 @@ TEST(MemTrackerTest, UnregisterFromParent) {
c->UnregisterFromParent();
}
+TEST(MemTrackerTest, TestMultiThreadedRegisterAndDestroy) {
+ std::atomic<bool> done(false);
+ vector<std::thread> threads;
+ for (int i = 0; i < 10; i++) {
+ threads.emplace_back([&done]{
+ while (!done.load()) {
+ shared_ptr<MemTracker> t = MemTracker::FindOrCreateTracker(1000, "foo");
+ }
+ });
+ }
+
+ SleepFor(MonoDelta::FromSeconds(AllowSlowTests() ? 5 : 1));
+ done.store(true);
+ for (auto& t : threads) {
+ t.join();
+ }
+}
+
} // namespace kudu
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/e284a93c/src/kudu/util/mem_tracker.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/mem_tracker.cc b/src/kudu/util/mem_tracker.cc
index 1f13347..1a73284 100644
--- a/src/kudu/util/mem_tracker.cc
+++ b/src/kudu/util/mem_tracker.cc
@@ -74,6 +74,7 @@ using std::string;
using std::stringstream;
using std::shared_ptr;
using std::vector;
+using std::weak_ptr;
using strings::Substitute;
@@ -153,7 +154,7 @@ shared_ptr<MemTracker> MemTracker::CreateTrackerUnlocked(int64_t byte_limit,
const shared_ptr<MemTracker>& parent) {
DCHECK(parent);
shared_ptr<MemTracker> tracker(new MemTracker(ConsumptionFunction(), byte_limit, id, parent));
- parent->AddChildTrackerUnlocked(tracker.get());
+ parent->AddChildTrackerUnlocked(tracker);
tracker->Init();
return tracker;
@@ -223,9 +224,10 @@ bool MemTracker::FindTrackerUnlocked(const string& id,
const shared_ptr<MemTracker>& parent) {
DCHECK(parent != NULL);
parent->child_trackers_lock_.AssertAcquired();
- for (MemTracker* child : parent->child_trackers_) {
- if (child->id() == id) {
- *tracker = child->shared_from_this();
+ for (const auto& child_weak : parent->child_trackers_) {
+ shared_ptr<MemTracker> child = child_weak.lock();
+ if (child && child->id() == id) {
+ *tracker = std::move(child);
return true;
}
}
@@ -255,8 +257,11 @@ void MemTracker::ListTrackers(vector<shared_ptr<MemTracker>>* trackers) {
trackers->push_back(t);
{
MutexLock l(t->child_trackers_lock_);
- for (MemTracker* child : t->child_trackers_) {
- to_process.push_back(child->shared_from_this());
+ for (const auto& child_weak : t->child_trackers_) {
+ shared_ptr<MemTracker> child = child_weak.lock();
+ if (child) {
+ to_process.emplace_back(std::move(child));
+ }
}
}
}
@@ -541,7 +546,7 @@ void MemTracker::Init() {
DCHECK_EQ(all_trackers_[0], this);
}
-void MemTracker::AddChildTrackerUnlocked(MemTracker* tracker) {
+void MemTracker::AddChildTrackerUnlocked(const shared_ptr<MemTracker>& tracker) {
child_trackers_lock_.AssertAcquired();
#ifndef NDEBUG
shared_ptr<MemTracker> found;
@@ -563,10 +568,13 @@ void MemTracker::LogUpdate(bool is_consume, int64_t bytes) const {
}
string MemTracker::LogUsage(const string& prefix,
- const list<MemTracker*>& trackers) {
+ const list<weak_ptr<MemTracker>>& trackers) {
vector<string> usage_strings;
- for (const MemTracker* child : trackers) {
- usage_strings.push_back(child->LogUsage(prefix));
+ for (const auto& child_weak : trackers) {
+ shared_ptr<MemTracker> child = child_weak.lock();
+ if (child) {
+ usage_strings.push_back(child->LogUsage(prefix));
+ }
}
return JoinStrings(usage_strings, "\n");
}
http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/e284a93c/src/kudu/util/mem_tracker.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/mem_tracker.h b/src/kudu/util/mem_tracker.h
index 645f6e5..6cd980f 100644
--- a/src/kudu/util/mem_tracker.h
+++ b/src/kudu/util/mem_tracker.h
@@ -267,13 +267,13 @@ class MemTracker : public std::enable_shared_from_this<MemTracker> {
// Adds tracker to child_trackers_.
//
// child_trackers_lock_ must be held.
- void AddChildTrackerUnlocked(MemTracker* tracker);
+ void AddChildTrackerUnlocked(const std::shared_ptr<MemTracker>& tracker);
// Logs the stack of the current consume/release. Used for debugging only.
void LogUpdate(bool is_consume, int64_t bytes) const;
static std::string LogUsage(const std::string& prefix,
- const std::list<MemTracker*>& trackers);
+ const std::list<std::weak_ptr<MemTracker>>& trackers);
// Variant of CreateTracker() that:
// 1. Must be called with a non-NULL parent, and
@@ -322,11 +322,11 @@ class MemTracker : public std::enable_shared_from_this<MemTracker> {
// listing only (i.e. updating the consumption of a parent tracker does not
// update that of its children).
mutable Mutex child_trackers_lock_;
- std::list<MemTracker*> child_trackers_;
+ std::list<std::weak_ptr<MemTracker>> child_trackers_;
// Iterator into parent_->child_trackers_ for this object. Stored to have O(1)
// remove.
- std::list<MemTracker*>::iterator child_tracker_it_;
+ std::list<std::weak_ptr<MemTracker>>::iterator child_tracker_it_;
// Functions to call after the limit is reached to free memory.
std::vector<GcFunction> gc_functions_;