EventBaseLocal cleanup

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

diff --git a/folly/io/async/EventBase.cpp b/folly/io/async/EventBase.cpp
index 8d6c03f211d0c2ddad2a95def05e1152b99ac6ff..d0d58def06e20e4d5411155b9bff918680612c4d 100644 (file)
--- a/folly/io/async/EventBase.cpp
+++ b/folly/io/async/EventBase.cpp
@@ -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.";
 }
 

diff --git a/folly/io/async/EventBase.h b/folly/io/async/EventBase.h
index 8527e559aa039dd5007c31e0a0f98b9b35bf4ef0..fe6dcf1eb8a2d48642ed78c63e4eab2a1df09073 100644 (file)
--- a/folly/io/async/EventBase.h
+++ b/folly/io/async/EventBase.h
@@ -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_;
 

diff --git a/folly/io/async/EventBaseLocal.cpp b/folly/io/async/EventBaseLocal.cpp
index 8dd551856471906abd516e6ae0affcaa17697f6b..999616fef25acde9080cae8677563a82729d60bd 100644 (file)
--- a/folly/io/async/EventBaseLocal.cpp
+++ b/folly/io/async/EventBaseLocal.cpp
@@ -15,50 +15,30 @@
  */
 
 #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);
   }
 }

diff --git a/folly/io/async/EventBaseLocal.h b/folly/io/async/EventBaseLocal.h
index 41f76a75b0617fb7231780cb729b97bc9a0c57dc..f7276375e51ba7696bc05cdca91f03f257027c52 100644 (file)
--- a/folly/io/async/EventBaseLocal.h
+++ b/folly/io/async/EventBaseLocal.h
@@ -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;
   }
 };
 

diff --git a/folly/io/async/test/EventBaseLocalTest.cpp b/folly/io/async/test/EventBaseLocalTest.cpp
index f7b963aaf88324a9337ef88876ff7bf50eb851e0..4e74a8cabec44669799ab7b36b18ce852bbd3cc5 100644 (file)
--- a/folly/io/async/test/EventBaseLocalTest.cpp
+++ b/folly/io/async/test/EventBaseLocalTest.cpp
@@ -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) {