Commit 033fa8af authored by Misha Shneerson's avatar Misha Shneerson Committed by Facebook GitHub Bot

fix race between EventBase and EventBaseLocal dtors

Summary:
EventBase keeps a registry of EventBaseLocal instances.EventBaseLocal keeps a registry of EventBase instances.
At destruction time, both are trying to remove themselves from the other's registry, and it is possible that dtors are racing each other.

There are two changes to address the race:
1. remove virtual method in EventBaseLocal because calling through vptr makes TSAN unhappy - the underlying vtbl is being mutated during destruction.
2.  Since deregistration involves acquiring two locks, a lock inversion must be avoided. This is achieved by retrying if inner lock acquisition has failed.

Reviewed By: andriigrynenko

Differential Revision: D29120201

fbshipit-source-id: 93c93c8d7cc7126e3432ac06562d55a838008e4a
parent 76c832bd
......@@ -31,6 +31,7 @@
#include <folly/String.h>
#include <folly/io/async/EventBaseAtomicNotificationQueue.h>
#include <folly/io/async/EventBaseBackendBase.h>
#include <folly/io/async/EventBaseLocal.h>
#include <folly/io/async/VirtualEventBase.h>
#include <folly/portability/Unistd.h>
#include <folly/synchronization/Baton.h>
......@@ -204,8 +205,20 @@ EventBase::~EventBase() {
// Stop consumer before deleting NotificationQueue
queue_->stopConsuming();
for (auto storage : localStorageToDtor_) {
storage->onEventBaseDestruction(*this);
// Remove self from all registered EventBaseLocal instances.
// Notice that we could be racing with EventBaseLocal dtor similarly
// deregistering itself from all registered EventBase instances. Because
// both sides need to acquire two locks, but in inverse order, we retry if
// inner lock acquisition fails to prevent lock inversion deadlock.
while (true) {
auto locked = localStorageToDtor_.wlock();
if (locked->empty()) {
break;
}
auto evbl = *locked->begin();
if (evbl->tryDeregister(*this)) {
locked->erase(evbl);
}
}
localStorage_.clear();
......@@ -214,6 +227,15 @@ EventBase::~EventBase() {
VLOG(5) << "EventBase(): Destroyed.";
}
bool EventBase::tryDeregister(detail::EventBaseLocalBase& evbl) {
if (auto locked = localStorageToDtor_.tryWLock()) {
locked->erase(&evbl);
runInEventBaseThread([this, key = evbl.key_] { localStorage_.erase(key); });
return true;
}
return false;
}
std::unique_ptr<EventBaseBackendBase> EventBase::getDefaultBackend() {
return std::make_unique<EventBaseBackend>();
}
......
......@@ -64,11 +64,6 @@ class NotificationQueue;
namespace detail {
class EventBaseLocalBase;
class EventBaseLocalBaseBase {
public:
virtual void onEventBaseDestruction(EventBase& evb) = 0;
virtual ~EventBaseLocalBaseBase() = default;
};
} // namespace detail
template <typename T>
class EventBaseLocal;
......@@ -955,7 +950,9 @@ class EventBase : public TimeoutManager,
template <typename T>
friend class EventBaseLocal;
std::unordered_map<std::size_t, erased_unique_ptr> localStorage_;
std::unordered_set<detail::EventBaseLocalBaseBase*> localStorageToDtor_;
folly::Synchronized<std::unordered_set<detail::EventBaseLocalBase*>>
localStorageToDtor_;
bool tryDeregister(detail::EventBaseLocalBase&);
folly::once_flag virtualEventBaseInitFlag_;
std::unique_ptr<VirtualEventBase> virtualEventBase_;
......
......@@ -26,12 +26,20 @@ namespace folly {
namespace detail {
EventBaseLocalBase::~EventBaseLocalBase() {
auto locked = eventBases_.rlock();
for (auto* evb : *locked) {
evb->runInEventBaseThread([this, evb, key = key_] {
evb->localStorage_.erase(key);
evb->localStorageToDtor_.erase(this);
});
// Remove self from all registered EventBase instances.
// Notice that we could be racing with EventBase dtor similarly
// deregistering itself from all registered EventBaseLocal instances. Because
// both sides need to acquire two locks, but in inverse order, we retry if
// inner lock acquisition fails to prevent lock inversion deadlock.
while (true) {
auto locked = eventBases_.wlock();
if (locked->empty()) {
break;
}
auto* evb = *locked->begin();
if (evb->tryDeregister(*this)) {
locked->erase(evb);
}
}
}
......@@ -46,15 +54,19 @@ void EventBaseLocalBase::erase(EventBase& evb) {
evb.dcheckIsInEventBaseThread();
evb.localStorage_.erase(key_);
evb.localStorageToDtor_.erase(this);
evb.localStorageToDtor_.wlock()->erase(this);
eventBases_.wlock()->erase(&evb);
}
void EventBaseLocalBase::onEventBaseDestruction(EventBase& evb) {
bool EventBaseLocalBase::tryDeregister(EventBase& evb) {
evb.dcheckIsInEventBaseThread();
eventBases_.wlock()->erase(&evb);
if (auto locked = eventBases_.tryWLock()) {
locked->erase(&evb);
return true;
}
return false;
}
void EventBaseLocalBase::setVoid(
......@@ -71,7 +83,7 @@ void EventBaseLocalBase::setVoid(
if (!alreadyExists) {
eventBases_.wlock()->insert(&evb);
evb.localStorageToDtor_.insert(this);
evb.localStorageToDtor_.wlock()->insert(this);
}
}
......
......@@ -33,14 +33,17 @@ namespace folly {
namespace detail {
class EventBaseLocalBase : public EventBaseLocalBaseBase {
class EventBaseLocalBase {
public:
friend class folly::EventBase;
EventBaseLocalBase() = default;
EventBaseLocalBase(const EventBaseLocalBase&) = delete;
EventBaseLocalBase& operator=(const EventBaseLocalBase&) = delete;
~EventBaseLocalBase() override;
~EventBaseLocalBase();
void erase(EventBase& evb);
void onEventBaseDestruction(EventBase& evb) override;
private:
bool tryDeregister(EventBase& evb);
protected:
void setVoid(EventBase& evb, void* ptr, void (*dtor)(void*));
......
......@@ -18,6 +18,7 @@
#include <folly/io/async/EventBaseAtomicNotificationQueue.h>
#include <folly/portability/GTest.h>
#include <folly/synchronization/Baton.h>
struct Foo {
Foo(int n_, std::function<void()> dtorFn_)
......@@ -112,3 +113,33 @@ TEST(EventBaseLocalTest, DestructionOrder) {
.get();
}
}
TEST(EventBaseLocalTest, DestructorStressTest) {
std::atomic<folly::EventBase*> currentEvb;
folly::Baton<> baton, baton2;
const int kIterations = 10000;
auto thread1 = std::thread([&] {
for (int i = 0; i < kIterations; i++) {
folly::EventBase evb;
currentEvb = &evb;
baton.post();
evb.loopForever();
baton2.wait();
baton2.reset();
}
});
auto thread2 = std::thread([&] {
for (int i = 0; i < kIterations; i++) {
folly::EventBaseLocal<int> local;
baton.wait();
baton.reset();
auto evb = currentEvb.exchange(nullptr);
evb->runInEventBaseThreadAndWait([&] { local.emplace(*evb, 4); });
evb->terminateLoopSoon();
baton2.post();
}
});
thread1.join();
thread2.join();
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment