Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure to wait for graph change events in test_node_graph. #1503

Merged
merged 3 commits into from
Jan 6, 2021
Merged

Make sure to wait for graph change events in test_node_graph. #1503

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79d7061
Make sure to wait for graph change events in test_node_graph.
clalancette Jan 5, 2021
71fc062
cpplint fixes.
clalancette Jan 5, 2021
a6f6a74
Introduce get_num_graph_things().
clalancette Jan 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 54 additions & 10 deletions rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@

#include <gtest/gtest.h>

#include <functional>
#include <memory>
#include <string>
#include <utility>
#include <vector>

#include "rcl/graph.h"
#include "rcl/node_options.h"
Expand Down Expand Up @@ -66,18 +68,57 @@ class TestNodeGraph : public ::testing::Test

const rclcpp::node_interfaces::NodeGraph * node_graph() const {return node_graph_;}

size_t get_num_graph_things(std::function<size_t()> predicate)
{
constexpr std::chrono::milliseconds timeout(100);

size_t tries = 0;
size_t num_things = 0;
while (tries++ < 5) {
num_things = predicate();
if (num_things >= 1) {
break;
}

auto event = node()->get_graph_event();
EXPECT_NO_THROW(node()->wait_for_graph_change(event, timeout));
}

// Run notify_graph_change() here to remove the graph_event users we created
// in the loop above.
node_graph_->notify_graph_change();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clalancette I'm not sure I follow this bit -- or at least I find it counter-intuitive.

Copy link
Contributor Author

@clalancette clalancette Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow this bit -- or at least I find it counter-intuitive.

I agree that it is counter-intuitive, though it is the way current NodeGraph API is setup.

A call to node->get_graph_event ends up calling node_graph->get_graph_event. There, a new shared pointer to rclcpp::Event is created, a weak reference to that Event is stored, the graph_users_count_ is incremented, and the shared pointer is returned. However, there is no explicit decrement of graph_users_count_. Instead, when notify_graph_change is called, it loops through the weak pointers, attempting to lock them. If it finds one that it cannot lock anymore (because it has been destroyed), then it removes that weak pointer from the list and recalculates the size of graph_users_count. That way you'll get the "correct" number of graph users when you later call count_graph_users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It's out of scope for this PR, but it'd be nice to have a way to explicitly drop the event instead of doing this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also seems like an abuse of this function to me. If you need to prune old events then maybe we should have a function for that, but why does that matter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be specific, I confused why clearing the old events is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be specific, I confused why clearing the old events is important.

Mostly because of this check:

rclcpp/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp

Line 94 in b9ef1fe

EXPECT_EQ(1u, node_graph()->count_graph_users());
. At that point, we can't really know how many graph events it took to get the previous data, so we really need to clean up. Alternatively, we could remove that check, but I feel like it is a good thing to test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even understand what that check is doing? Maybe just getting coverage on that function? The value it returns isn't really important I think.

I mean if we ever added something to the node that happen to use the graph events, this would break because there would be more users. I think that change could just be changed to >= 1, if needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if we ever added something to the node that happen to use the graph events, this would break because there would be more users. I think that change could just be changed to >= 1, if needed?

That's true, we could do that.

Here's what I'm going to do. To take advantage of the CI that already successfully completed, I'm going to merge this PR as-is. That fixes one of our current problems in ros2/rmw#293 . I'll do an immediate follow-up to change that fragile count_graph_users test and remove the notify_graph_change, which should satisfy @wjwwood 's concerns.


return num_things;
}

size_t get_num_topics()
{
return get_num_graph_things(
[this]() -> size_t {
auto topic_names_and_types = node_graph()->get_topic_names_and_types();
return topic_names_and_types.size();
});
}

size_t get_num_services()
{
return get_num_graph_things(
[this]() -> size_t {
auto service_names_and_types = node_graph()->get_service_names_and_types();
return service_names_and_types.size();
});
}

private:
std::shared_ptr<rclcpp::Node> node_;
rclcpp::node_interfaces::NodeGraph * node_graph_;
};

TEST_F(TestNodeGraph, construct_from_node)
{
auto topic_names_and_types = node_graph()->get_topic_names_and_types(false);
EXPECT_LT(0u, topic_names_and_types.size());
EXPECT_LT(0u, get_num_topics());

auto service_names_and_types = node_graph()->get_service_names_and_types();
EXPECT_LT(0u, service_names_and_types.size());
EXPECT_LT(0u, get_num_services());

auto names = node_graph()->get_node_names();
EXPECT_EQ(1u, names.size());
Expand All @@ -96,8 +137,7 @@ TEST_F(TestNodeGraph, construct_from_node)

TEST_F(TestNodeGraph, get_topic_names_and_types)
{
auto topic_names_and_types = node_graph()->get_topic_names_and_types();
EXPECT_LT(0u, topic_names_and_types.size());
ASSERT_LT(0u, get_num_topics());
}

TEST_F(TestNodeGraph, get_topic_names_and_types_rcl_error)
Expand Down Expand Up @@ -126,8 +166,7 @@ TEST_F(TestNodeGraph, get_topic_names_and_types_rcl_names_and_types_fini_error)

TEST_F(TestNodeGraph, get_service_names_and_types)
{
auto service_names_and_types = node_graph()->get_service_names_and_types();
EXPECT_LT(0u, service_names_and_types.size());
ASSERT_LT(0u, get_num_services());
}

TEST_F(TestNodeGraph, get_service_names_and_types_rcl_error)
Expand Down Expand Up @@ -309,8 +348,13 @@ TEST_F(TestNodeGraph, get_info_by_topic)

EXPECT_EQ(0u, node_graph()->get_publishers_info_by_topic("topic", true).size());

auto publishers = node_graph()->get_publishers_info_by_topic("topic", false);
ASSERT_EQ(1u, publishers.size());
std::vector<rclcpp::TopicEndpointInfo> publishers;
size_t num_publishers = get_num_graph_things(
[this, &publishers]() {
publishers = node_graph()->get_publishers_info_by_topic("topic", false);
return publishers.size();
});
ASSERT_EQ(1u, num_publishers);

auto publisher_endpoint_info = publishers[0];
const auto const_publisher_endpoint_info = publisher_endpoint_info;
Expand Down