Fix has_invalid_weak_nodes (#266)

* Fix #264

* Corrected test comment

rclcpp/CMakeLists.txt

@@ -120,6 +120,13 @@ if(BUILD_TESTING)
       ${PROJECT_NAME}
     )
   endif()
+  ament_add_gtest(test_find_weak_nodes test/test_find_weak_nodes.cpp)
+  if(TARGET test_find_weak_nodes)
+    target_include_directories(test_find_weak_nodes PUBLIC
+      ${rcl_INCLUDE_DIRS}
+    )
+    target_link_libraries(test_find_weak_nodes ${PROJECT_NAME})
+  endif()
 endif()
 
 ament_package(

rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp

@@ -144,7 +144,7 @@ public:
     for (auto & weak_node : weak_nodes) {
       auto node = weak_node.lock();
       if (!node) {
-        has_invalid_weak_nodes = false;
+        has_invalid_weak_nodes = true;
         continue;
       }
       for (auto & weak_group : node->get_callback_groups()) {

rclcpp/test/test_find_weak_nodes.cpp

@@ -0,0 +1,76 @@
+// Copyright 2016 Open Source Robotics Foundation, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <gtest/gtest.h>
+#include <memory>
+
+#include "rclcpp/strategies/allocator_memory_strategy.hpp"
+#include "rclcpp/memory_strategy.hpp"
+#include "rclcpp/node.hpp"
+#include "rclcpp/rclcpp.hpp"
+
+class TestFindWeakNodes : public ::testing::Test
+{
+protected:
+  static void SetUpTestCase()
+  {
+    rclcpp::init(0, nullptr);
+  }
+};
+
+TEST_F(TestFindWeakNodes, allocator_strategy_with_weak_nodes) {
+  // GIVEN
+  // A vector of weak pointers to nodes
+  auto memory_strategy = std::make_shared<
+    rclcpp::memory_strategies::allocator_memory_strategy::AllocatorMemoryStrategy<>>();
+  auto existing_node = rclcpp::node::Node::make_shared("existing_node");
+  auto dead_node = rclcpp::node::Node::make_shared("dead_node");
+  rclcpp::memory_strategy::MemoryStrategy::WeakNodeVector weak_nodes;
+  weak_nodes.push_back(existing_node);
+  weak_nodes.push_back(dead_node);
+
+  // AND
+  // Delete dead_node, creating a dangling pointer in weak_nodes
+  dead_node.reset();
+  ASSERT_FALSE(weak_nodes[0].expired());
+  ASSERT_TRUE(weak_nodes[1].expired());
+
+  // WHEN
+  bool has_invalid_weak_nodes = memory_strategy->collect_entities(weak_nodes);
+
+  // THEN
+  // The result of finding dangling node pointers should be true
+  ASSERT_TRUE(has_invalid_weak_nodes);
+}
+
+TEST_F(TestFindWeakNodes, allocator_strategy_no_weak_nodes) {
+  // GIVEN
+  // A vector of weak pointers to nodes, all valid
+  auto memory_strategy = std::make_shared<
+    rclcpp::memory_strategies::allocator_memory_strategy::AllocatorMemoryStrategy<>>();
+  auto existing_node1 = rclcpp::node::Node::make_shared("existing_node1");
+  auto existing_node2 = rclcpp::node::Node::make_shared("existing_node2");
+  rclcpp::memory_strategy::MemoryStrategy::WeakNodeVector weak_nodes;
+  weak_nodes.push_back(existing_node1);
+  weak_nodes.push_back(existing_node2);
+  ASSERT_FALSE(weak_nodes[0].expired());
+  ASSERT_FALSE(weak_nodes[1].expired());
+
+  // WHEN
+  bool has_invalid_weak_nodes = memory_strategy->collect_entities(weak_nodes);
+
+  // THEN
+  // The result of finding dangling node pointers should be false
+  ASSERT_FALSE(has_invalid_weak_nodes);
+}