Detect async children reliably in ReactiveSequence XML validation

include/behaviortree_cpp/bt_factory.h

@@ -40,15 +40,31 @@ inline NodeBuilder CreateBuilder(Args... args)
   };
 }
 
+template <typename T>
+inline constexpr bool IsManifestAsync()
+{
+  return std::is_base_of_v<ThreadedAction, T> ||
+         std::is_base_of_v<StatefulActionNode, T> ||
+         std::is_base_of_v<CoroActionNode, T>;
+}
+
 template <typename T>
 inline TreeNodeManifest CreateManifest(const std::string& ID,
                                        PortsList portlist = getProvidedPorts<T>())
 {
+  TreeNodeManifest manifest;
+  manifest.type = getType<T>();
+  manifest.registration_ID = ID;
+  manifest.ports = std::move(portlist);
   if constexpr(has_static_method_metadata<T>::value)
   {
-    return { getType<T>(), ID, portlist, T::metadata() };
+    manifest.metadata = T::metadata();
   }
-  return { getType<T>(), ID, portlist, {} };
+  if constexpr(IsManifestAsync<T>())
+  {
+    SetNodeManifestAsync(manifest);
+  }
+  return manifest;
 }
 
 #ifdef BT_PLUGIN_EXPORT
@@ -472,6 +488,9 @@ class BehaviorTreeFactory
   /// to <TreeNodesModel> with the function writeTreeNodesModelXML()
   void addMetadataToManifest(const std::string& node_id, const KeyValueVector& metadata);
 
+  /// Mark a registered node as asynchronous for XML validation.
+  void markNodeAsAsynchronous(const std::string& node_id, bool is_async = true);
+
   /**
    * @brief Add an Enum to the scripting language.
    * For instance if you do:

include/behaviortree_cpp/tree_node.h

@@ -41,6 +41,63 @@ struct TreeNodeManifest
   KeyValueVector metadata;
 };
 
+[[nodiscard]] inline bool IsReservedNodeMetadataField(StringView key)
+{
+  return key == "__bt_async";
+}
+
+inline void SetNodeManifestAsync(TreeNodeManifest& manifest, bool is_async = true)
+{
+  auto async_it = manifest.metadata.end();
+  for(auto it = manifest.metadata.begin(); it != manifest.metadata.end();)
+  {
+    if(IsReservedNodeMetadataField(it->first))
+    {
+      if(async_it == manifest.metadata.end())
+      {
+        async_it = it;
+        ++it;
+      }
+      else
+      {
+        it = manifest.metadata.erase(it);
+      }
+    }
+    else
+    {
+      ++it;
+    }
+  }
+
+  if(is_async)
+  {
+    if(async_it == manifest.metadata.end())
+    {
+      manifest.metadata.emplace_back("__bt_async", "true");
+    }
+    else
+    {
+      async_it->second = "true";
+    }
+  }
+  else if(async_it != manifest.metadata.end())
+  {
+    manifest.metadata.erase(async_it);
+  }
+}
+
+[[nodiscard]] inline bool IsNodeManifestAsync(const TreeNodeManifest& manifest)
+{
+  for(const auto& [key, value] : manifest.metadata)
+  {
+    if(IsReservedNodeMetadataField(key))
+    {
+      return value == "true";
+    }
+  }
+  return false;
+}
+
 using PortsRemapping = std::unordered_map<std::string, std::string>;
 using NonPortAttributes = std::unordered_map<std::string, std::string>;
 

include/behaviortree_cpp/xml_parsing.h

@@ -44,7 +44,8 @@ class XMLParser : public Parser
 };
 
 void VerifyXML(const std::string& xml_text,
-               const std::unordered_map<std::string, NodeType>& registered_nodes);
+               const std::unordered_map<std::string, TreeNodeManifest>&
+                   registered_nodes);
 
 /**
  * @brief writeTreeNodesModelXML generates an XMl that contains the manifests in the

src/bt_factory.cpp

@@ -119,6 +119,8 @@ BehaviorTreeFactory::BehaviorTreeFactory() : _p(new PImpl)
   registerNodeType<FallbackNode>("AsyncFallback", true);
   registerNodeType<SequenceNode>("Sequence");
   registerNodeType<SequenceNode>("AsyncSequence", true);
+  markNodeAsAsynchronous("AsyncFallback");
+  markNodeAsAsynchronous("AsyncSequence");
   registerNodeType<SequenceWithMemory>("SequenceWithMemory");
 
 #ifdef USE_BTCPP3_OLD_NAMES
@@ -485,7 +487,23 @@ void BehaviorTreeFactory::addMetadataToManifest(const std::string& node_id,
   {
     throw std::runtime_error("addMetadataToManifest: wrong ID");
   }
+  const bool is_async = IsNodeManifestAsync(it->second);
   it->second.metadata = metadata;
+  if(is_async)
+  {
+    SetNodeManifestAsync(it->second);
+  }
+}
+
+void BehaviorTreeFactory::markNodeAsAsynchronous(const std::string& node_id,
+                                                 bool is_async)
+{
+  auto it = _p->manifests.find(node_id);
+  if(it == _p->manifests.end())
+  {
+    throw std::runtime_error("markNodeAsAsynchronous: wrong ID");
+  }
+  SetNodeManifestAsync(it->second, is_async);
 }
 
 void BehaviorTreeFactory::registerScriptingEnum(StringView name, int value)

src/xml_parsing.cpp

@@ -439,18 +439,12 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes)
   }
 
   // Collect the names of all nodes registered with the behavior tree factory
-  std::unordered_map<std::string, BT::NodeType> registered_nodes;
-  for(const auto& it : factory->manifests())
-  {
-    registered_nodes.insert({ it.first, it.second.type });
-  }
-
   XMLPrinter printer;
   doc->Print(&printer);
   auto xml_text = std::string(printer.CStr(), size_t(printer.CStrSize()));
 
   // Verify the validity of the XML before adding any behavior trees to the parser's list of registered trees
-  VerifyXML(xml_text, registered_nodes);
+  VerifyXML(xml_text, factory->manifests());
 
   loadSubtreeModel(xml_root);
 
@@ -473,7 +467,8 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes)
 }
 
 void VerifyXML(const std::string& xml_text,
-               const std::unordered_map<std::string, BT::NodeType>& registered_nodes)
+               const std::unordered_map<std::string, BT::TreeNodeManifest>&
+                   registered_nodes)
 {
   XMLDocument doc;
   auto xml_error = doc.Parse(xml_text.c_str(), xml_text.size());
@@ -629,7 +624,7 @@ void VerifyXML(const std::string& xml_text,
         ThrowError(line_number, std::string("Node not recognized: ") + lookup_name);
       }
 
-      const auto node_type = search->second;
+      const auto node_type = search->second.type;
       const std::string& registered_name = search->first;
 
       if(node_type == NodeType::DECORATOR)
@@ -665,11 +660,12 @@ void VerifyXML(const std::string& xml_text,
               ThrowError(child->GetLineNum(),
                          std::string("Unknown node type: ") + child_name);
             }
-            const auto child_type = child_search->second;
-            if(child_type == NodeType::CONTROL &&
-               ((child_name == "ThreadedAction") ||
-                (child_name == "StatefulActionNode") ||
-                (child_name == "CoroActionNode") || (child_name == "AsyncSequence")))
+            const auto& child_manifest = child_search->second;
+            const bool is_async_child =
+                IsNodeManifestAsync(child_manifest) &&
+                (child_manifest.type == NodeType::ACTION ||
+                 child_manifest.type == NodeType::CONTROL);
+            if(is_async_child)
             {
               ++async_count;
               if(async_count > 1)
@@ -1340,12 +1336,19 @@ void addNodeModelToXML(const TreeNodeManifest& model, XMLDocument& doc,
 
     for(const auto& [name, value] : model.metadata)
     {
+      if(IsReservedNodeMetadataField(name))
+      {
+        continue;
+      }
       auto metadata_element = doc.NewElement("Metadata");
       metadata_element->SetAttribute(name.c_str(), value.c_str());
       metadata_root->InsertEndChild(metadata_element);
     }
 
-    element->InsertEndChild(metadata_root);
+    if(metadata_root->FirstChildElement() != nullptr)
+    {
+      element->InsertEndChild(metadata_root);
+    }
   }
 
   model_root->InsertEndChild(element);

tests/CMakeLists.txt

@@ -12,6 +12,14 @@ set_target_properties(plugin_issue953 PROPERTIES
 )
 target_link_libraries(plugin_issue953 ${BTCPP_LIBRARY})
 
+add_library(plugin_issue1184 SHARED plugin_issue1184/plugin_issue1184.cpp)
+target_compile_definitions(plugin_issue1184 PRIVATE BT_PLUGIN_EXPORT)
+set_target_properties(plugin_issue1184 PROPERTIES
+  PREFIX ""
+  LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+)
+target_link_libraries(plugin_issue1184 ${BTCPP_LIBRARY})
+
 ######################################################
 
 set(BT_TESTS
@@ -55,6 +63,7 @@ set(BT_TESTS
   gtest_simple_string.cpp
   gtest_polymorphic_ports.cpp
   gtest_plugin_issue953.cpp
+  gtest_plugin_issue1184.cpp
   gtest_blackboard_thread_safety.cpp
   gtest_xml_null_subtree_id.cpp
 
@@ -123,7 +132,8 @@ endif()
 target_compile_definitions(behaviortree_cpp_test PRIVATE BT_TEST_FOLDER="${CMAKE_CURRENT_SOURCE_DIR}")
 
 # Ensure plugin is built before tests run, and tests can find it
-add_dependencies(behaviortree_cpp_test plugin_issue953)
+add_dependencies(behaviortree_cpp_test plugin_issue953 plugin_issue1184)
 target_compile_definitions(behaviortree_cpp_test PRIVATE
   BT_PLUGIN_ISSUE953_PATH="$<TARGET_FILE:plugin_issue953>"
+  BT_PLUGIN_ISSUE1184_PATH="$<TARGET_FILE:plugin_issue1184>"
 )

tests/gtest_basic_types.cpp

@@ -408,6 +408,19 @@ TEST(BasicTypes, TreeNodeManifest)
   ASSERT_EQ(manifest.ports.size(), 2u);
 }
 
+TEST(BasicTypes, TreeNodeManifestAsyncMetadata)
+{
+  TreeNodeManifest manifest;
+  EXPECT_FALSE(IsNodeManifestAsync(manifest));
+
+  SetNodeManifestAsync(manifest);
+  EXPECT_TRUE(IsNodeManifestAsync(manifest));
+  EXPECT_TRUE(IsReservedNodeMetadataField("__bt_async"));
+
+  SetNodeManifestAsync(manifest, false);
+  EXPECT_FALSE(IsNodeManifestAsync(manifest));
+}
+
 // ============ Result type tests ============
 
 TEST(BasicTypes, Result_Success)

tests/gtest_factory.cpp

@@ -1,5 +1,7 @@
 #include "behaviortree_cpp/xml_parsing.h"
 
+#include "action_test_node.h"
+
 #include <filesystem>
 #include <string>
 #include <utility>
@@ -495,6 +497,19 @@ TEST(BehaviorTreeFactory, addMetadataToManifest)
   EXPECT_EQ(modified_manifest.metadata, makeTestMetadata());
 }
 
+TEST(BehaviorTreeFactory, addMetadataToManifestPreservesAsyncMarker)
+{
+  BehaviorTreeFactory factory;
+  factory.registerNodeType<BT::AsyncActionTest>("AsyncActionTest");
+
+  factory.addMetadataToManifest("AsyncActionTest", makeTestMetadata());
+
+  const auto& manifest = factory.manifests().at("AsyncActionTest");
+  EXPECT_TRUE(IsNodeManifestAsync(manifest));
+  EXPECT_EQ(manifest.metadata[0], makeTestMetadata()[0]);
+  EXPECT_EQ(manifest.metadata[1], makeTestMetadata()[1]);
+}
+
 // Action node used to reproduce issue #1046 (use-after-free on
 // manifest pointer). It calls getInput() for a port name that is
 // NOT in the XML, so getInputStamped falls through to the
@@ -777,3 +792,42 @@ TEST(BehaviorTreeFactory, MalformedXML_UnknownNodeType)
   BehaviorTreeFactory factory;
   EXPECT_THROW((void)factory.createTreeFromText(xml), RuntimeError);
 }
+
+TEST(BehaviorTreeFactory, VerifyXMLRejectsManualAsyncControlInReactiveSequence)
+{
+  const char* xml_text_issue = R"(
+<root BTCPP_format="4">
+  <BehaviorTree ID="MainTree">
+    <ReactiveSequence>
+      <ManualAsyncFallback>
+        <AlwaysFailure/>
+        <AlwaysSuccess/>
+      </ManualAsyncFallback>
+      <AsyncSequence>
+        <AlwaysSuccess/>
+      </AsyncSequence>
+    </ReactiveSequence>
+  </BehaviorTree>
+</root> )";
+
+  BehaviorTreeFactory factory;
+
+  TreeNodeManifest manifest{ NodeType::CONTROL, "ManualAsyncFallback", {}, {} };
+  SetNodeManifestAsync(manifest);
+  factory.registerBuilder(
+      manifest, [](const std::string& name, const NodeConfig&) -> std::unique_ptr<TreeNode> {
+        return std::make_unique<FallbackNode>(name, true);
+      });
+
+  EXPECT_THROW((void)factory.createTreeFromText(xml_text_issue), RuntimeError);
+}
+
+TEST(BehaviorTreeFactory, WriteTreeNodesModelXMLSkipsInternalAsyncMetadata)
+{
+  BehaviorTreeFactory factory;
+  factory.registerNodeType<BT::AsyncActionTest>("AsyncActionTest");
+
+  const auto xml = writeTreeNodesModelXML(factory, false);
+
+  EXPECT_EQ(xml.find("__bt_async"), std::string::npos);
+}