fixes and simpler getAnyLocked

Author: facontidavide

examples/CMakeLists.txt

@@ -27,7 +27,10 @@ CompileExample("t08_additional_node_args")
 CompileExample("t09_scripting")
 CompileExample("t10_observer")
 CompileExample("t11_replace_rules")
+
+if(BTCPP_GROOT_INTERFACE AND BTCPP_SQLITE_LOGGING)
 CompileExample("t12_groot_howto")
+endif()
 
 CompileExample("ex01_wrap_legacy")
 CompileExample("ex02_runtime_ports")

include/behaviortree_cpp/blackboard.h

@@ -6,7 +6,6 @@
 #include <stdint.h>
 #include <unordered_map>
 #include <mutex>
-#include <shared_mutex>
 #include <sstream>
 
 #include "behaviortree_cpp/basic_types.h"
@@ -16,13 +15,10 @@
 
 namespace BT
 {
-/// This type contains pointer to Any.
-/// Protected with a read-only lock as long as the object is in scope
-using AnyPtrReadLock = LockedPtrConst<Any>;
 
-/// This type contains pointer to Any.
-/// Protected with a read-write lock as long as the object is in scope
-using AnyPtrWriteLock = LockedPtr<Any>;
+/// This type contains a pointer to Any, protected
+/// with a locked mutex as long as the object is in scope
+using AnyPtrLocked = LockedPtr<Any>;
 
 /**
  * @brief The Blackboard is the mechanism used by BehaviorTrees to exchange
@@ -45,7 +41,7 @@ class Blackboard
   {
     Any value;
     PortInfo port_info;
-    std::shared_mutex entry_mutex;
+    mutable std::mutex entry_mutex;
 
     Entry(const PortInfo& info) : port_info(info)
     {}
@@ -91,43 +87,43 @@ class Blackboard
     return const_cast<Entry*>( static_cast<const Blackboard &>(*this).getEntry(key));
   }
 
-  [[nodiscard]] AnyPtrReadLock getAnyRead(const std::string& key) const
+  [[nodiscard]] AnyPtrLocked getAnyLocked(const std::string& key)
   {
     if(auto entry = getEntry(key))
     {
-      return AnyPtrReadLock(&entry->value, const_cast<std::shared_mutex*>(&entry->entry_mutex));
+      return AnyPtrLocked(&entry->value, &entry->entry_mutex);
     }
     return {};
   }
 
-  [[nodiscard]] AnyPtrWriteLock getAnyWrite(const std::string& key)
+  [[nodiscard]] AnyPtrLocked getAnyLocked(const std::string& key) const
   {
     if(auto entry = getEntry(key))
     {
-      return AnyPtrWriteLock(&entry->value, &entry->entry_mutex);
+      return AnyPtrLocked(&entry->value,  const_cast<std::mutex*>(&entry->entry_mutex));
     }
     return {};
   }
 
-  [[deprecated("Use getAnyRead instead")]]
+  [[deprecated("Use getAnyLocked instead")]]
   const Any* getAny(const std::string& key) const
   {
-    return getAnyRead(key).get();
+    return getAnyLocked(key).get();
   }
 
-  [[deprecated("Use getAnyWrite instead")]]
+  [[deprecated("Use getAnyLocked instead")]]
   Any* getAny(const std::string& key)
   {
-    return getAnyWrite(key).get();
+    return const_cast<Any*>(getAnyLocked(key).get());
   }
 
   /** Return true if the entry with the given key was found.
-     *  Note that this method may throw an exception if the cast to T failed.
-     */
+   *  Note that this method may throw an exception if the cast to T failed.
+   */
   template <typename T> [[nodiscard]]
   bool get(const std::string& key, T& value) const
   {
-    if (auto any_ref = getAnyRead(key))
+    if (auto any_ref = getAnyLocked(key))
     {
       value = any_ref.get()->cast<T>();
       return true;
@@ -136,12 +132,12 @@ class Blackboard
   }
 
   /**
-     * Version of get() that throws if it fails.
-    */
+   * Version of get() that throws if it fails.
+   */
   template <typename T> [[nodiscard]]
   T get(const std::string& key) const
   {
-    if (auto any_ref = getAnyRead(key))
+    if (auto any_ref = getAnyLocked(key))
     {
       return any_ref.get()->cast<T>();
     }

include/behaviortree_cpp/decorators/loop_node.h

@@ -13,7 +13,6 @@
 #pragma once
 
 #include <deque>
-#include <shared_mutex>
 #include "behaviortree_cpp/decorator_node.h"
 
 namespace BT
@@ -70,9 +69,9 @@ class LoopNode : public DecoratorNode
     {
       // if the port is static, any_ref is empty, otherwise it will keep access to
       // port locked for thread-safety
-      AnyPtrWriteLock any_ref = static_queue_ ?
-                                    AnyPtrWriteLock() :
-                                getLockedPortContent("queue");
+      AnyPtrLocked any_ref = static_queue_ ?
+                                 AnyPtrLocked() :
+                                 getLockedPortContent("queue");
       if(any_ref)
       {
         current_queue_ = any_ref.get()->cast<SharedQueue<T>>();

include/behaviortree_cpp/scripting/operators.hpp

@@ -68,7 +68,7 @@ struct ExprName : ExprBase
             }
         }
         // search now in the variables table
-        auto any_ref = env.vars->getAnyRead(name);
+        auto any_ref = env.vars->getAnyLocked(name);
         if( !any_ref )
         {
           throw std::runtime_error("Variable not found");

include/behaviortree_cpp/tree_node.h

@@ -270,7 +270,7 @@ class TreeNode
    * @return     empty AnyWriteRef if the blackboard entry doesn't exist or the content
    *             of the port was a static string.
    */
-  [[nodiscard]] AnyPtrWriteLock getLockedPortContent(const std::string& key);
+  [[nodiscard]] AnyPtrLocked getLockedPortContent(const std::string& key);
 
   // function provided mostly for debugging purpose to see the raw value
   // in the port (no remapping and no conversion to a type)
@@ -426,7 +426,7 @@ inline Result TreeNode::getInput(const std::string& key, T& destination) const
       return nonstd::make_unexpected("getInput(): trying to access an invalid Blackboard");
     }
 
-    if (auto any_ref = config_.blackboard->getAnyRead(std::string(remapped_key)))
+    if (auto any_ref = config_.blackboard->getAnyLocked(std::string(remapped_key)))
     {
       auto val = any_ref.get();
       if(!val->empty())

include/behaviortree_cpp/utils/locked_reference.hpp

@@ -2,33 +2,46 @@
 
 #include <memory>
 #include <mutex>
-#include <shared_mutex>
 
 namespace BT
 {
 /**
  * @brief The LockedPtr class is used to share a pointer to an object
- * and a mutex that protects the write access to that object.
+ * and a mutex that protects the read/write access to that object.
  *
- * As long as the object remains in scope, the mutex is locked
+ * As long as the object remains in scope, the mutex is locked, therefore
+ * you must destroy this object as soon as the pointer was used.
  */
 template <typename T>
 class LockedPtr {
   public:
 
   LockedPtr() = default;
 
-      LockedPtr(T* obj, std::shared_mutex* obj_mutex):
+  LockedPtr(const T* obj, std::mutex* obj_mutex):
         ref_(obj), mutex_(obj_mutex) {
     mutex_->lock();
   }
 
-      ~LockedPtr() {
+  ~LockedPtr() {
     if(mutex_) {
       mutex_->unlock();
     }
   }
 
+  LockedPtr(LockedPtr const&) = delete;
+  LockedPtr& operator=(LockedPtr const&) = delete;
+
+  LockedPtr(LockedPtr && other) {
+    std::swap(ref_, other.ref_);
+    std::swap(mutex_, other.mutex_);
+  }
+
+  LockedPtr& operator=(LockedPtr&& other) {
+    std::swap(ref_, other.ref_);
+    std::swap(mutex_, other.mutex_);
+  }
+
   operator bool() const {
     return ref_ != nullptr;
   }
@@ -53,60 +66,10 @@ class LockedPtr {
     return ref_;
   }
 
-  T* get() {
-    return ref_;
-  }
-
-  private:
-  T* ref_ = nullptr;
-  std::shared_mutex* mutex_ = nullptr;
-};
-
-/**
- * @brief The LockedPtrConst class is used to share a pointer to an object
- * and a mutex that protects the access to that object.
- * It has a read-only interface, the object can not be modified.
- * Multiple instances of LockedPtrConst can exist without a dead-lock.
- *
- * As long as the object remains in scope, the shared mutex is locked
- */
-template <typename T>
-class LockedPtrConst {
-  public:
-
-  LockedPtrConst() = default;
-
-      LockedPtrConst(LockedPtrConst const&) = delete;
-  LockedPtrConst& operator=(LockedPtrConst const&) = delete;
-
-      LockedPtrConst(const T* obj, std::shared_mutex* obj_mutex):
-        ref_(obj), mutex_(obj_mutex) {
-    mutex_->lock_shared();
-  }
-
-      ~LockedPtrConst() {
-    if(mutex_) {
-      mutex_->unlock_shared();
-    }
-  }
-
-  operator bool() const {
-    return ref_ != nullptr;
-  }
-
-  bool empty() const {
-    return ref_ == nullptr;
-  }
-
-  const T * get() const {
-    return ref_;
-  }
-
   private:
   const T* ref_ = nullptr;
-  std::shared_mutex* mutex_ = nullptr;
+  std::mutex* mutex_ = nullptr;
 };
 
 
-
 }

src/json_export.cpp

@@ -44,7 +44,7 @@ nlohmann::json ExportBlackboardToJSON(Blackboard &blackboard)
   for(auto entry_name: blackboard.getKeys())
   {
     std::string name(entry_name);
-    if(auto any_ref = blackboard.getAnyRead(name))
+    if(auto any_ref = blackboard.getAnyLocked(name))
     {
       if(auto any_ptr = any_ref.get())
       {

src/tree_node.cpp

@@ -411,11 +411,11 @@ std::string toStr<PostCond>(PostCond pre)
   }
 }
 
-AnyPtrWriteLock BT::TreeNode::getLockedPortContent(const std::string &key)
+AnyPtrLocked BT::TreeNode::getLockedPortContent(const std::string &key)
 {
   if(auto remapped_key = getRemappedKey(key, getRawPortValue(key)))
   {
-    return config_.blackboard->getAnyWrite(std::string(*remapped_key));
+    return config_.blackboard->getAnyLocked(std::string(*remapped_key));
   }
   return {};
 }

tests/gtest_blackboard.cpp

@@ -296,28 +296,57 @@ TEST(BlackboardTest, CheckTypeSafety)
   ASSERT_TRUE(is);
 }
 
-TEST(BlackboardTest, AnyPtr)
+TEST(BlackboardTest, AnyPtrLocked)
 {
   auto blackboard = Blackboard::create();
-  auto test_obj = std::make_shared<int>(42);
+  long value = 0;
+  long* test_obj = &value;
 
   blackboard->set("testmove", test_obj);
 
-  // no deadlock if both are read-only
+  auto const timeout = std::chrono::milliseconds(250);
+
+  // Safe way to access a pointer
   {
-    auto r1 = blackboard->getAnyRead("testmove");
-    auto r2 = blackboard->getAnyRead("testmove");
+    std::atomic_llong cycles = 0;
+    auto func = [&]()
+    {
+      auto start = std::chrono::system_clock::now();
+      while( (std::chrono::system_clock::now() - start) < timeout)
+      {
+        auto r1 = blackboard->getAnyLocked("testmove");
+        auto value_ptr = (r1.get()->cast<long*>());
+        (*value_ptr)++;
+        cycles++;
+      }
+    };
+
+    auto t1 = std::thread(func); // other thread
+    func(); // this thread
+    t1.join();
+
+    // number of increments and cycles is expected to be the same
+    ASSERT_EQ(cycles, value);
+  }
+  //------------------
+  // UNSAFE way to access a pointer
+  {
+    std::atomic_llong cycles = 0;
+    auto func = [&]()
+    {
+      auto start = std::chrono::system_clock::now();
+      while( (std::chrono::system_clock::now() - start) < timeout)
+      {
+        auto value_ptr = blackboard->get<long*>("testmove");
+        (*value_ptr)++;
+        cycles++;
+      }
+    };
+
+    auto t1 = std::thread(func);
+    func();
+    t1.join();
+    // since the operation value_ptr++ is not thread safe, cycle and value will unlikely be the same
+    ASSERT_NE(cycles, value);
   }
-
-  auto double_write = [&]() {
-    auto w1 = blackboard->getAnyWrite("testmove");
-    auto w2 = blackboard->getAnyWrite("testmove");
-  };
-  EXPECT_ANY_THROW(double_write());
-
-  auto write_read = [&]() {
-    auto w1 = blackboard->getAnyWrite("testmove");
-    auto r1 = blackboard->getAnyRead("testmove");
-  };
-  EXPECT_ANY_THROW(write_read());
 }

tests/gtest_factory.cpp

@@ -253,9 +253,9 @@ TEST(BehaviorTreeFactory, SubTreeWithRemapping)
   ASSERT_EQ(main_bb->get<std::string>("talk_out"), "done!");
 
   // these ports should not be present in the subtree TalkToMe
-  ASSERT_FALSE(talk_bb->getAnyRead("talk_hello"));
-  ASSERT_FALSE(talk_bb->getAnyRead("talk_bye"));
-  ASSERT_FALSE(talk_bb->getAnyRead("talk_out"));
+  ASSERT_FALSE(talk_bb->getAnyLocked("talk_hello"));
+  ASSERT_FALSE(talk_bb->getAnyLocked("talk_bye"));
+  ASSERT_FALSE(talk_bb->getAnyLocked("talk_out"));
 }
 
 std::string FilePath(const std::filesystem::path& relative_path)