Make sure to wait for graph change events in test_node_graph.

[clalancette]

While debugging something else, I came across this fairly rare
flake in the test_node_graph tests. Essentially, it may take
some time for node changes to propagate into the graph. If
the NodeGraph client asks for data before the changes are
in the graph, they may get something they don't expect. The
fix is to wait for an event on the NodeGraph to happen, and
then look for the data you are interested in.

Before this change, I could get the flake to happen on a loaded
aarch64 system fairly regularly. After this change, I can no
longer make the flake happen on a loaded system.

Signed-off-by: Chris Lalancette [email protected]

@eboasson FYI, this should fix the aarch64 failures we are seeing in the tests in ros2/rmw#293 .

[hidmic]

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

[clalancette]

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.

[hidmic]

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.

[wjwwood]

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?

[wjwwood]

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

[clalancette]

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.

[wjwwood]

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?

[clalancette]

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.

[wjwwood]

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?

[clalancette]

All right, a6f6a74 should fix the problems pointed out by @wjwwood , and also uses a predicate system as suggested by @hidmic. Please take another look.

[clalancette]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

lgtm, except I'd get rid of the notify_graph_change() and instead change the "count users" check to be less fragile.