diff --git a/include/behaviortree_cpp/utils/locked_reference.hpp b/include/behaviortree_cpp/utils/locked_reference.hpp index 674908dfe..9c57c71f8 100644 --- a/include/behaviortree_cpp/utils/locked_reference.hpp +++ b/include/behaviortree_cpp/utils/locked_reference.hpp @@ -2,6 +2,7 @@ #include "behaviortree_cpp/utils/safe_any.hpp" +#include #include namespace BT @@ -19,7 +20,15 @@ class LockedPtr public: LockedPtr() = default; - LockedPtr(T* obj, std::mutex* obj_mutex) : ref_(obj), mutex_(obj_mutex) + /** + * @param obj the object to be protected + * @param obj_mutex the mutex protecting obj + * @param owner optional shared ownership of whatever holds obj and obj_mutex. + * It is kept alive until this instance is destroyed, so that + * neither the object nor the mutex can be freed while locked. + */ + LockedPtr(T* obj, std::mutex* obj_mutex, std::shared_ptr owner = {}) + : ref_(obj), mutex_(obj_mutex), owner_(std::move(owner)) { mutex_->lock(); } @@ -39,12 +48,14 @@ class LockedPtr { std::swap(ref_, other.ref_); std::swap(mutex_, other.mutex_); + std::swap(owner_, other.owner_); } LockedPtr& operator=(LockedPtr&& other) noexcept { std::swap(ref_, other.ref_); std::swap(mutex_, other.mutex_); + std::swap(owner_, other.owner_); return *this; } @@ -108,6 +119,7 @@ class LockedPtr private: T* ref_ = nullptr; std::mutex* mutex_ = nullptr; + std::shared_ptr owner_ = nullptr; }; } // namespace BT diff --git a/src/blackboard.cpp b/src/blackboard.cpp index 7858b4655..b5730ae27 100644 --- a/src/blackboard.cpp +++ b/src/blackboard.cpp @@ -25,7 +25,9 @@ AnyPtrLocked Blackboard::getAnyLocked(const std::string& key) { if(auto entry = getEntry(key)) { - return AnyPtrLocked(&entry->value, &entry->entry_mutex); + // hand over the shared_ptr: unset()/clear() only take storage_mutex_, so the + // entry can be erased while entry_mutex is still locked here. + return AnyPtrLocked(&entry->value, &entry->entry_mutex, entry); } return {}; } @@ -34,7 +36,8 @@ AnyPtrLocked Blackboard::getAnyLocked(const std::string& key) const { if(auto entry = getEntry(key)) { - return AnyPtrLocked(&entry->value, const_cast(&entry->entry_mutex)); + return AnyPtrLocked(&entry->value, const_cast(&entry->entry_mutex), + entry); } return {}; } diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index f4ce45689..208ea248a 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -303,6 +303,21 @@ TEST(BlackboardTest, AnyPtrLocked) } #endif +TEST(BlackboardTest, AnyPtrLockedSurvivesUnset) +{ + auto blackboard = Blackboard::create(); + blackboard->set("value", 42); + + auto locked = blackboard->getAnyLocked("value"); + ASSERT_TRUE(bool(locked)); + + // unset() takes storage_mutex_ but not entry_mutex, so it can drop the last + // reference to the entry we are holding the lock on. + blackboard->unset("value"); + + ASSERT_EQ(locked.get()->cast(), 42); +} + TEST(BlackboardTest, SetStringView) { auto bb = Blackboard::create();