diff --git a/include/behaviortree_cpp/blackboard.h b/include/behaviortree_cpp/blackboard.h index c88924400..651c05a5d 100644 --- a/include/behaviortree_cpp/blackboard.h +++ b/include/behaviortree_cpp/blackboard.h @@ -255,6 +255,8 @@ inline T Blackboard::get(const std::string& key) const inline void Blackboard::unset(const std::string& key) { + // the entry (i.e. the stored value) is destroyed outside the lock + std::shared_ptr removed; std::unique_lock storage_lock(storage_mutex_); // check local storage @@ -265,6 +267,7 @@ inline void Blackboard::unset(const std::string& key) return; } + removed = std::move(it->second); storage_.erase(it); } 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 03d2f6e25..412e263f7 100644 --- a/src/blackboard.cpp +++ b/src/blackboard.cpp @@ -2,8 +2,12 @@ #include "behaviortree_cpp/json_export.h" +#include +#include +#include #include #include +#include namespace BT { @@ -14,6 +18,94 @@ bool IsPrivateKey(StringView str) { return str.size() >= 1 && str.data()[0] == '_'; } + +// Blackboard::getAnyLocked() returns an AnyPtrLocked: raw pointers to the value +// and to the (locked) entry_mutex of an Entry. It does not own the Entry and its +// layout can not change (ABI), so the only sign that the holder is done is the +// release of the mutex. Therefore an Entry whose last shared_ptr is dropped +// (unset(), clear(), cloneInto(), ~Blackboard) while its mutex is still locked +// is parked here, and destroyed later by the first RetireEntry() that finds it +// unlocked. +struct RetiredEntries +{ + std::mutex mutex; + std::vector entries; + std::atomic_size_t count = 0; +}; + +RetiredEntries& GetRetiredEntries() +{ + // Never destroyed: a Blackboard with static storage duration may be + // destroyed after this object during program exit. + // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) + static auto* const instance = new RetiredEntries(); + return *instance; +} + +// No shared_ptr is left when this is called, so nobody can lock the entry +// anymore: if try_lock() succeeds, the entry is unreachable. +// Note: the calling thread may be the one holding the AnyPtrLocked. A +// try_lock() on a std::mutex owned by the caller is formally undefined, but +// it simply fails on all the supported platforms (pthread: EBUSY). +bool IsUnlocked(Blackboard::Entry* entry) +{ + if(entry->entry_mutex.try_lock()) + { + entry->entry_mutex.unlock(); + return true; + } + return false; +} + +// Destroy the parked entries that have been unlocked in the meantime. +// If "retired" is not null, it is parked first. +void SweepRetiredEntries(Blackboard::Entry* retired = nullptr) noexcept +{ + auto& parked = GetRetiredEntries(); + std::vector unlocked; + try + { + const std::scoped_lock lock(parked.mutex); + if(retired != nullptr) + { + parked.entries.push_back(retired); + } + const auto it = std::partition(parked.entries.begin(), parked.entries.end(), + [](auto* entry) { return !IsUnlocked(entry); }); + unlocked.assign(it, parked.entries.end()); + parked.entries.erase(it, parked.entries.end()); + parked.count = parked.entries.size(); + } + catch(...) // NOLINT(bugprone-empty-catch) + { + // Out of memory. Whatever could not be moved to "unlocked" remains parked + // (or leaks, in the case of "retired"), which is safe. + } + // Destroyed outside the lock: the destructor of a stored value may remove + // other entries, i.e. call this function again. + for(auto* entry : unlocked) + { + delete entry; // NOLINT(cppcoreguidelines-owning-memory) + } +} + +// Deleter of every shared_ptr. Being attached at allocation time, it +// covers all the removal paths, including those inlined in the user's binary. +void RetireEntry(Blackboard::Entry* retired) noexcept +{ + // Common case: nothing is parked and nobody holds this entry + if(GetRetiredEntries().count == 0 && IsUnlocked(retired)) + { + delete retired; // NOLINT(cppcoreguidelines-owning-memory) + return; + } + SweepRetiredEntries(retired); +} + +std::shared_ptr MakeEntry(const TypeInfo& info) +{ + return { new Blackboard::Entry(info), &RetireEntry }; +} } // namespace void Blackboard::enableAutoRemapping(bool remapping) @@ -157,8 +249,10 @@ std::vector Blackboard::getKeys() const void Blackboard::clear() { + // the entries (i.e. the stored values) are destroyed outside the lock + decltype(storage_) removed; const std::unique_lock storage_lock(storage_mutex_); - storage_.clear(); + removed.swap(storage_); } void Blackboard::createEntry(const std::string& key, const TypeInfo& info) @@ -248,7 +342,7 @@ void Blackboard::cloneInto(Blackboard& dst) const { // create new entry from src std::scoped_lock src_lock(task.src->entry_mutex); - auto new_entry = std::make_shared(task.src->info); + auto new_entry = MakeEntry(task.src->info); new_entry->value = task.src->value; new_entry->string_converter = task.src->string_converter; new_entries.emplace_back(task.key, std::move(new_entry)); @@ -258,6 +352,8 @@ 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()) { + // the stale entries are destroyed outside the lock + std::vector> removed; const std::unique_lock dst_lock(dst.storage_mutex_); for(auto& [key, entry] : new_entries) { @@ -265,7 +361,11 @@ void Blackboard::cloneInto(Blackboard& dst) const } for(const auto& key : keys_to_remove) { - dst.storage_.erase(key); + if(auto it = dst.storage_.find(key); it != dst.storage_.end()) + { + removed.push_back(std::move(it->second)); + dst.storage_.erase(it); + } } } } @@ -291,6 +391,12 @@ std::shared_ptr Blackboard::createEntryImpl(const std::string return rootBlackboard()->createEntryImpl(key.substr(1, key.size() - 1), info); } + // Bound the lifetime of the entries that were removed while locked + if(GetRetiredEntries().count != 0) + { + SweepRetiredEntries(); + } + const std::unique_lock storage_lock(storage_mutex_); // This function might be called recursively, when we do remapping, because we move // to the top scope to find already existing entries @@ -336,7 +442,7 @@ std::shared_ptr Blackboard::createEntryImpl(const std::string } // not remapped, not found. Create locally. - auto entry = std::make_shared(info); + auto entry = MakeEntry(info); // even if empty, let's assign to it a default type entry->value = Any(info.type()); storage_.insert({ key, entry }); diff --git a/tests/gtest_blackboard.cpp b/tests/gtest_blackboard.cpp index b5bf614b8..fd2c2769e 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,162 @@ 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 when the next one is + // created (or removed) + blackboard->set("other", 1); + ASSERT_TRUE(weak_value.expired()); +} + +// Stress test, meaningful with ASan/TSan: readers hold an AnyPtrLocked while +// another thread keeps removing and re-creating the same entry. +TEST(BlackboardTest, AnyPtrLockedConcurrentUnsetStress) +{ + auto blackboard = Blackboard::create(); + std::atomic_bool stop = false; + std::atomic_int reads = 0; + + auto reader = [&]() { + while(!stop) + { + if(auto locked = blackboard->getAnyLocked("value")) + { + // set() inserts the entry before assigning the value: it may be empty. + // Otherwise, read it while the writer may unset the key. + if(!locked->empty()) + { + ASSERT_EQ(locked->cast(), 42); + reads++; + } + } + } + }; + std::thread reader_a(reader); + std::thread reader_b(reader); + + for(int i = 0; i < 5000; i++) + { + blackboard->set("value", 42); + blackboard->unset("value"); + } + // make sure that the readers had the chance to run + blackboard->set("value", 42); + while(reads == 0) + { + std::this_thread::yield(); + } + stop = true; + reader_a.join(); + reader_b.join(); +} + TEST(BlackboardTest, SetStringView) { auto bb = Blackboard::create();