diff --git a/fb303/CallbackValuesMap-inl.h b/fb303/CallbackValuesMap-inl.h index 799b19a42..caae0b13d 100644 --- a/fb303/CallbackValuesMap-inl.h +++ b/fb303/CallbackValuesMap-inl.h @@ -104,7 +104,7 @@ bool CallbackValuesMap::unregisterCallback(folly::StringPiece name) { if (entry == wlock->map.end()) { return false; } - entry->second->clear(); + auto callbackCopy = std::move(entry->second); // avoid fetch_add() to avoid extra fences, since we hold the lock already uint64_t epoch = wlock->mapEpoch.load(); @@ -112,6 +112,10 @@ bool CallbackValuesMap::unregisterCallback(folly::StringPiece name) { wlock->map.erase(entry); VLOG(5) << "Unregistered callback: " << name; + + // clear the callback after releasing the lock + wlock.unlock(); + callbackCopy->clear(); return true; } diff --git a/fb303/test/CallbackValuesMapTest.cpp b/fb303/test/CallbackValuesMapTest.cpp index 5e0644fcb..848273c71 100644 --- a/fb303/test/CallbackValuesMapTest.cpp +++ b/fb303/test/CallbackValuesMapTest.cpp @@ -20,6 +20,7 @@ #include #include +#include #include using boost::bind; @@ -119,3 +120,30 @@ TEST(CallbackValuesMapTest, GetCallback) { key2Cb->getValue(&val2); EXPECT_EQ(val2, 321); } + +TEST(CallbackValuesMapTest, DoubleDynamicCounterDeadlock) { + TestCallbackValuesMap callbackMap; + callbackMap.registerCallback("a", []() { return 42; }); + callbackMap.registerCallback("b", [&callbackMap]() { + sleep(2); + int val; + callbackMap.getValue("a", &val); + return val; + }); + + std::thread t1([&callbackMap]() { + int val; + callbackMap.getValue("b", &val); + }); + + folly::Baton<> baton; + std::thread t2([&callbackMap, &baton, &t1]() { + sleep(1); + callbackMap.unregisterCallback("b"); + t1.join(); + baton.post(); + }); + ASSERT_TRUE(baton.try_wait_for(std::chrono::seconds(10))); + t2.join(); + SUCCEED(); +}