diff --git a/include/behaviortree_cpp/blackboard.h b/include/behaviortree_cpp/blackboard.h index c88924400..aac7592a6 100644 --- a/include/behaviortree_cpp/blackboard.h +++ b/include/behaviortree_cpp/blackboard.h @@ -76,7 +76,7 @@ class Blackboard return std::shared_ptr(new Blackboard(parent)); } - virtual ~Blackboard() = default; + virtual ~Blackboard(); void enableAutoRemapping(bool remapping); @@ -115,6 +115,11 @@ class Blackboard template void set(const std::string& key, const T& value); + /** + * @brief Remove the entry with the given key, if any. This never blocks: + * if an AnyPtrLocked to that entry is still alive, the entry is destroyed + * only after the lock has been released. + */ void unset(const std::string& key); [[nodiscard]] const TypeInfo* entryInfo(const std::string& key); @@ -253,21 +258,6 @@ inline T Blackboard::get(const std::string& key) const throw RuntimeError("Blackboard::get() error. Missing key [", key, "]"); } -inline void Blackboard::unset(const std::string& key) -{ - std::unique_lock storage_lock(storage_mutex_); - - // check local storage - auto it = storage_.find(key); - if(it == storage_.end()) - { - // No entry, nothing to do. - return; - } - - storage_.erase(it); -} - template inline void Blackboard::set(const std::string& key, const T& value) { diff --git a/include/behaviortree_cpp/utils/locked_reference.hpp b/include/behaviortree_cpp/utils/locked_reference.hpp index 674908dfe..2374b493b 100644 --- a/include/behaviortree_cpp/utils/locked_reference.hpp +++ b/include/behaviortree_cpp/utils/locked_reference.hpp @@ -12,6 +12,10 @@ namespace BT * * As long as the object remains in scope, the mutex is locked, therefore * you must destroy this instance as soon as the pointer was used. + * + * LockedPtr does not own the object it points to: the owner is only expected + * to keep it alive while the mutex is locked. In particular, the pointer must + * not be considered valid after calling unlock(). */ template class LockedPtr diff --git a/src/blackboard.cpp b/src/blackboard.cpp index 7858b4655..6bf7889cd 100644 --- a/src/blackboard.cpp +++ b/src/blackboard.cpp @@ -4,6 +4,7 @@ #include #include +#include namespace BT { @@ -14,8 +15,72 @@ bool IsPrivateKey(StringView str) { return str.size() >= 1 && str.data()[0] == '_'; } + +// Entries removed from a Blackboard while an AnyPtrLocked still held their +// entry_mutex. An AnyPtrLocked does not own the entry, and the only signal that +// the holder is done with it is the release of the mutex, so such entries are +// parked here and destroyed by a later removal that finds them unlocked. +struct DeferredEntries +{ + std::mutex mutex; + std::vector> entries; +}; + +DeferredEntries& GetDeferredEntries() +{ + // Never destroyed: a Blackboard with static storage duration may be + // destroyed after this object during program exit. + static auto* const instance = new DeferredEntries(); + return *instance; +} + +// Destroy the entries that were removed from a Blackboard, except those whose +// entry_mutex is still locked by an outstanding AnyPtrLocked: these are parked +// until a later call finds them unlocked. This never blocks, so removing an +// entry while holding a lock on it (from any thread) cannot deadlock. +void ReleaseEntries(std::vector> entries) +{ + std::vector> to_destroy; + auto& deferred = GetDeferredEntries(); + { + const std::scoped_lock lock(deferred.mutex); + entries.insert(entries.end(), std::make_move_iterator(deferred.entries.begin()), + std::make_move_iterator(deferred.entries.end())); + deferred.entries.clear(); + for(auto& entry : entries) + { + // A removed entry can't be found by getAnyLocked() anymore, so once + // try_lock() succeeds nobody holds it. + if(entry->entry_mutex.try_lock()) + { + entry->entry_mutex.unlock(); + to_destroy.push_back(std::move(entry)); + } + else + { + deferred.entries.push_back(std::move(entry)); + } + } + } + // "to_destroy" is released here, outside the lock, because the destructor + // of a stored value may call back into a Blackboard. +} } // namespace +Blackboard::~Blackboard() +{ + // An AnyPtrLocked may still refer to one of the entries: let ReleaseEntries() + // defer the destruction of that entry until the lock is released. + std::vector> entries; + entries.reserve(storage_.size()); + for(auto& [key, entry] : storage_) + { + entries.push_back(std::move(entry)); + } + storage_.clear(); + ReleaseEntries(std::move(entries)); +} + void Blackboard::enableAutoRemapping(bool remapping) { autoremapping_ = remapping; @@ -23,18 +88,31 @@ void Blackboard::enableAutoRemapping(bool remapping) AnyPtrLocked Blackboard::getAnyLocked(const std::string& key) { - if(auto entry = getEntry(key)) + while(auto entry = getEntry(key)) { - return AnyPtrLocked(&entry->value, &entry->entry_mutex); + AnyPtrLocked locked(&entry->value, &entry->entry_mutex); + // Re-check under entry_mutex: the removal paths erase the entry from the + // storage first and destroy it only once its mutex can be acquired (see + // ReleaseEntries). If the key still resolves to this entry, it will stay + // alive as long as the lock is held; otherwise release the lock and look + // the key up again. + if(getEntry(key) == entry) + { + return locked; + } } return {}; } AnyPtrLocked Blackboard::getAnyLocked(const std::string& key) const { - if(auto entry = getEntry(key)) + while(auto entry = getEntry(key)) { - return AnyPtrLocked(&entry->value, const_cast(&entry->entry_mutex)); + AnyPtrLocked locked(&entry->value, const_cast(&entry->entry_mutex)); + if(getEntry(key) == entry) + { + return locked; + } } return {}; } @@ -155,10 +233,38 @@ std::vector Blackboard::getKeys() const return out; } +void Blackboard::unset(const std::string& key) +{ + std::vector> removed; + { + const std::unique_lock storage_lock(storage_mutex_); + + // check local storage + auto it = storage_.find(key); + if(it == storage_.end()) + { + // No entry, nothing to do. + return; + } + removed.push_back(std::move(it->second)); + storage_.erase(it); + } + ReleaseEntries(std::move(removed)); +} + void Blackboard::clear() { - const std::unique_lock storage_lock(storage_mutex_); - storage_.clear(); + std::vector> removed; + { + const std::unique_lock storage_lock(storage_mutex_); + removed.reserve(storage_.size()); + for(auto& [key, entry] : storage_) + { + removed.push_back(std::move(entry)); + } + storage_.clear(); + } + ReleaseEntries(std::move(removed)); } void Blackboard::createEntry(const std::string& key, const TypeInfo& info) @@ -258,15 +364,24 @@ void Blackboard::cloneInto(Blackboard& dst) const // Step 3: insert new entries and remove stale ones under dst.storage_mutex_. if(!new_entries.empty() || !keys_to_remove.empty()) { - const std::unique_lock dst_lock(dst.storage_mutex_); - for(auto& [key, entry] : new_entries) - { - dst.storage_.try_emplace(key, std::move(entry)); - } - for(const auto& key : keys_to_remove) + std::vector> removed_entries; { - dst.storage_.erase(key); + const std::unique_lock dst_lock(dst.storage_mutex_); + for(auto& [key, entry] : new_entries) + { + dst.storage_.try_emplace(key, std::move(entry)); + } + for(const auto& key : keys_to_remove) + { + auto it = dst.storage_.find(key); + if(it != dst.storage_.end()) + { + removed_entries.push_back(std::move(it->second)); + dst.storage_.erase(it); + } + } } + ReleaseEntries(std::move(removed_entries)); } } diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index f4ce45689..c645fe466 100644 --- a/tests/gtest_blackboard.cpp +++ b/tests/gtest_blackboard.cpp @@ -13,6 +13,10 @@ #include "behaviortree_cpp/blackboard.h" #include "behaviortree_cpp/bt_factory.h" +#include +#include +#include + #include #include "../sample_nodes/dummy_nodes.h" @@ -303,6 +307,120 @@ TEST(BlackboardTest, AnyPtrLocked) } #endif +// An entry removed from the blackboard must stay alive as long as an +// AnyPtrLocked refers to it, and removing it must never block, not even +// from the thread holding the lock. + +TEST(BlackboardTest, AnyPtrLockedSurvivesUnset) +{ + auto blackboard = Blackboard::create(); + blackboard->set("value", 42); + + auto locked = blackboard->getAnyLocked("value"); + ASSERT_TRUE(bool(locked)); + + blackboard->unset("value"); + ASSERT_TRUE(blackboard->getKeys().empty()); + + // the entry must stay alive as long as we hold the lock + ASSERT_EQ(locked.get()->cast(), 42); + + locked = {}; + ASSERT_FALSE(bool(blackboard->getAnyLocked("value"))); +} + +TEST(BlackboardTest, AnyPtrLockedSurvivesClear) +{ + auto blackboard = Blackboard::create(); + blackboard->set("value", 42); + + auto locked = blackboard->getAnyLocked("value"); + ASSERT_TRUE(bool(locked)); + + blackboard->clear(); + ASSERT_TRUE(blackboard->getKeys().empty()); + ASSERT_EQ(locked.get()->cast(), 42); +} + +TEST(BlackboardTest, AnyPtrLockedSurvivesCloneInto) +{ + auto src = Blackboard::create(); + auto dst = Blackboard::create(); + dst->set("stale", 42); + + auto locked = dst->getAnyLocked("stale"); + ASSERT_TRUE(bool(locked)); + + // "stale" doesn't exist in src, so cloneInto() removes it from dst + src->cloneInto(*dst); + ASSERT_TRUE(dst->getKeys().empty()); + ASSERT_EQ(locked.get()->cast(), 42); +} + +TEST(BlackboardTest, AnyPtrLockedSurvivesBlackboardDestruction) +{ + auto blackboard = Blackboard::create(); + blackboard->set("value", 42); + + auto locked = blackboard->getAnyLocked("value"); + ASSERT_TRUE(bool(locked)); + + blackboard.reset(); + ASSERT_EQ(locked.get()->cast(), 42); +} + +TEST(BlackboardTest, AnyPtrLockedCrossUnsetDoesNotDeadlock) +{ + auto blackboard = Blackboard::create(); + blackboard->set("A", 1); + blackboard->set("B", 2); + + // Each thread holds a lock on one entry and removes the other one, while + // the other thread does the opposite. + std::atomic ready = 0; + int values[2] = { 0, 0 }; + auto hold_and_unset = [&](const char* held, const char* removed, int& out) { + auto locked = blackboard->getAnyLocked(held); + ready++; + while(ready < 2) + { + std::this_thread::yield(); + } + blackboard->unset(removed); + out = locked ? locked.get()->cast() : 0; + }; + + std::thread t1(hold_and_unset, "A", "B", std::ref(values[0])); + std::thread t2(hold_and_unset, "B", "A", std::ref(values[1])); + t1.join(); + t2.join(); + + ASSERT_EQ(values[0], 1); + ASSERT_EQ(values[1], 2); + ASSERT_TRUE(blackboard->getKeys().empty()); +} + +TEST(BlackboardTest, AnyPtrLockedDeferredEntryIsDestroyed) +{ + auto blackboard = Blackboard::create(); + auto value = std::make_shared(42); + std::weak_ptr weak_value = value; + blackboard->set("value", value); + value.reset(); + + { + auto locked = blackboard->getAnyLocked("value"); + ASSERT_TRUE(bool(locked)); + blackboard->unset("value"); + // still alive, since we hold the lock + ASSERT_FALSE(weak_value.expired()); + } + // the lock was released: the entry is destroyed by the next removal + blackboard->set("other", 1); + blackboard->unset("other"); + ASSERT_TRUE(weak_value.expired()); +} + TEST(BlackboardTest, SetStringView) { auto bb = Blackboard::create();