EventBaseLocal cleanup
authorAndrii Grynenko <andrii@fb.com>
Thu, 20 Apr 2017 21:54:37 +0000 (14:54 -0700)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Thu, 20 Apr 2017 22:10:16 +0000 (15:10 -0700)
Summary:
1. Restrict EventBaseLocal API to only be used from EventBase thread to avoid extra locking.
2. Make sure objects stored in EventBaseLocal are destroyed in EventBase thread.

Reviewed By: yfeldblum

Differential Revision: D4918282

fbshipit-source-id: b7cb4c2b62fef85a9b1d796fa71af8af9087479d

folly/io/async/EventBase.cpp
folly/io/async/EventBase.h
folly/io/async/EventBaseLocal.cpp
folly/io/async/EventBaseLocal.h
folly/io/async/test/EventBaseLocalTest.cpp

index 8d6c03f..d0d58de 100644 (file)
@@ -183,12 +183,10 @@ EventBase::~EventBase() {
     event_base_free(evb_);
   }
 
-  {
-    std::lock_guard<std::mutex> lock(localStorageMutex_);
-    for (auto storage : localStorageToDtor_) {
-      storage->onEventBaseDestruction(*this);
-    }
+  for (auto storage : localStorageToDtor_) {
+    storage->onEventBaseDestruction(*this);
   }
+
   VLOG(5) << "EventBase(): Destroyed.";
 }
 
index 8527e55..fe6dcf1 100644 (file)
@@ -743,7 +743,6 @@ class EventBase : private boost::noncopyable,
   // see EventBaseLocal
   friend class detail::EventBaseLocalBase;
   template <typename T> friend class EventBaseLocal;
-  std::mutex localStorageMutex_;
   std::unordered_map<uint64_t, std::shared_ptr<void>> localStorage_;
   std::unordered_set<detail::EventBaseLocalBaseBase*> localStorageToDtor_;
 
index 8dd5518..999616f 100644 (file)
  */
 
 #include <folly/io/async/EventBaseLocal.h>
+#include <folly/MapUtil.h>
 #include <atomic>
 #include <thread>
 
 namespace folly { namespace detail {
 
 EventBaseLocalBase::~EventBaseLocalBase() {
-  // There's a race condition if an EventBase and an EventBaseLocal destruct
-  // at the same time (each will lock eventBases_ and localStorageMutex_
-  // in the opposite order), so we dance around it with a loop and try_lock.
-  while (true) {
-    SYNCHRONIZED(eventBases_) {
-      auto it = eventBases_.begin();
-      while (it != eventBases_.end()) {
-        auto evb = *it;
-        if (evb->localStorageMutex_.try_lock()) {
-          evb->localStorage_.erase(key_);
-          evb->localStorageToDtor_.erase(this);
-          it = eventBases_.erase(it);
-          evb->localStorageMutex_.unlock();
-        } else {
-          ++it;
-        }
-      }
-
-      if (eventBases_.empty()) {
-        return;
-      }
-    }
-    std::this_thread::yield(); // let the other thread take the eventBases_ lock
+  for (auto* evb : *eventBases_.rlock()) {
+    evb->runInEventBaseThread([ this, evb, key = key_ ] {
+      evb->localStorage_.erase(key);
+      evb->localStorageToDtor_.erase(this);
+    });
   }
 }
 
 void* EventBaseLocalBase::getVoid(EventBase& evb) {
-  std::lock_guard<std::mutex> lg(evb.localStorageMutex_);
-  auto it2 = evb.localStorage_.find(key_);
-  if (UNLIKELY(it2 != evb.localStorage_.end())) {
-    return it2->second.get();
-  }
+  DCHECK(evb.isInEventBaseThread());
 
-  return nullptr;
+  return folly::get_default(evb.localStorage_, key_, {}).get();
 }
 
 void EventBaseLocalBase::erase(EventBase& evb) {
-  std::lock_guard<std::mutex> lg(evb.localStorageMutex_);
+  DCHECK(evb.isInEventBaseThread());
+
   evb.localStorage_.erase(key_);
   evb.localStorageToDtor_.erase(this);
 
@@ -68,18 +48,15 @@ void EventBaseLocalBase::erase(EventBase& evb) {
 }
 
 void EventBaseLocalBase::onEventBaseDestruction(EventBase& evb) {
+  DCHECK(evb.isInEventBaseThread());
+
   SYNCHRONIZED(eventBases_) {
     eventBases_.erase(&evb);
   }
 }
 
 void EventBaseLocalBase::setVoid(EventBase& evb, std::shared_ptr<void>&& ptr) {
-  std::lock_guard<std::mutex> lg(evb.localStorageMutex_);
-  setVoidUnlocked(evb, std::move(ptr));
-}
-
-void EventBaseLocalBase::setVoidUnlocked(
-    EventBase& evb, std::shared_ptr<void>&& ptr) {
+  DCHECK(evb.isInEventBaseThread());
 
   auto alreadyExists =
     evb.localStorage_.find(key_) != evb.localStorage_.end();
@@ -87,9 +64,7 @@ void EventBaseLocalBase::setVoidUnlocked(
   evb.localStorage_.emplace(key_, std::move(ptr));
 
   if (!alreadyExists) {
-    SYNCHRONIZED(eventBases_) {
-      eventBases_.insert(&evb);
-    }
+    eventBases_.wlock()->insert(&evb);
     evb.localStorageToDtor_.insert(this);
   }
 }
index 41f76a7..f727637 100644 (file)
@@ -37,7 +37,6 @@ class EventBaseLocalBase : public EventBaseLocalBaseBase, boost::noncopyable {
 
  protected:
   void setVoid(EventBase& evb, std::shared_ptr<void>&& ptr);
-  void setVoidUnlocked(EventBase& evb, std::shared_ptr<void>&& ptr);
   void* getVoid(EventBase& evb);
 
   folly::Synchronized<std::unordered_set<EventBase*>> eventBases_;
@@ -92,17 +91,13 @@ class EventBaseLocal : public detail::EventBaseLocalBase {
 
   template <typename... Args>
   T& getOrCreate(EventBase& evb, Args&&... args) {
-    std::lock_guard<std::mutex> lg(evb.localStorageMutex_);
-
-    auto it2 = evb.localStorage_.find(key_);
-    if (LIKELY(it2 != evb.localStorage_.end())) {
-      return *static_cast<T*>(it2->second.get());
-    } else {
-      auto smartPtr = std::make_shared<T>(std::forward<Args>(args)...);
-      auto ptr = smartPtr.get();
-      setVoidUnlocked(evb, std::move(smartPtr));
-      return *ptr;
+    if (auto ptr = getVoid(evb)) {
+      return *static_cast<T*>(ptr);
     }
+    auto smartPtr = std::make_shared<T>(std::forward<Args>(args)...);
+    auto& ref = *smartPtr;
+    setVoid(evb, std::move(smartPtr));
+    return ref;
   }
 
   template <typename Func>
@@ -110,17 +105,13 @@ class EventBaseLocal : public detail::EventBaseLocalBase {
     // If this looks like it's copy/pasted from above, that's because it is.
     // gcc has a bug (fixed in 4.9) that doesn't allow capturing variadic
     // params in a lambda.
-    std::lock_guard<std::mutex> lg(evb.localStorageMutex_);
-
-    auto it2 = evb.localStorage_.find(key_);
-    if (LIKELY(it2 != evb.localStorage_.end())) {
-      return *static_cast<T*>(it2->second.get());
-    } else {
-      std::shared_ptr<T> smartPtr(fn());
-      auto ptr = smartPtr.get();
-      setVoidUnlocked(evb, std::move(smartPtr));
-      return *ptr;
+    if (auto ptr = getVoid(evb)) {
+      return *static_cast<T*>(ptr);
     }
+    std::shared_ptr<T> smartPtr(fn());
+    auto& ref = *smartPtr;
+    setVoid(evb, std::move(smartPtr));
+    return ref;
   }
 };
 
index f7b963a..4e74a8c 100644 (file)
@@ -71,7 +71,9 @@ TEST(EventBaseLocalTest, Basic) {
     EXPECT_EQ(dtorCnt, 2); // should dtor a Foo when evb2 destructs
 
   }
-  EXPECT_EQ(dtorCnt, 3); // should dtor a Foo when foo destructs
+  EXPECT_EQ(dtorCnt, 2); // should schedule Foo destructor, when foo destructs
+  evb1.loop();
+  EXPECT_EQ(dtorCnt, 3); // Foo will be destroyed in EventBase loop
 }
 
 TEST(EventBaseLocalTest, getOrCreate) {