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

Jump to bottom

Shutdown the context before context's destructor is invoked in tests #2633

Merged
merged 6 commits into from
Sep 24, 2024
Merged

Shutdown the context before context's destructor is invoked in tests #2633

merged 6 commits into from
Sep 24, 2024

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Sep 18, 2024

No description provided.

@ahcorde
zenoh: Shutdown the node properly in component tests
82d3569 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde self-assigned this Sep 18, 2024
@ahcorde ahcorde mentioned this pull request Sep 18, 2024
Merged
@alsora
Copy link
Collaborator

alsora commented Sep 18, 2024

Why this change? The context is initialized in the SetupTestCase method so it should be shut down there

fujitatomoya
fujitatomoya approved these changes Sep 18, 2024
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm. the fix itself makes sense to me that we call shutdown explicitly once it completes the all test cases with this fixture.

i would also like to know @alsora 's comment above. i think default_context will be destructed when the process space is destroyed, then context will be shutdown. What kind of problem did you have with rmw_zenoh?

@Yadunund
Copy link
Member

@ahcorde any chance you opened this PR because you ran into the panic documented here? ros2/rmw_zenoh#170

If so I wonder if my comment here explains why we need this PR.

Yadunund
Yadunund approved these changes Sep 20, 2024
View reviewed changes
Copy link
Member

@Yadunund Yadunund left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please see ros2/rmw_zenoh#170 (comment) for why this is needed.

TLDR; Leaving the context's destructor to shutdown the context at program termiantion causes Zenoh to panic since Thread local storage gets cleared earlier.

@Yadunund
Copy link
Member

CI started with this repos file.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@Yadunund Yadunund changed the title zenoh: Shutdown the node properly in component tests Shutdown the context before context's destructor is invoked in tests Sep 20, 2024
@Yadunund Yadunund force-pushed the ahcorde/rolling/clean_rmw_zenoh_component_Test branch from 4b50b61 to 2fa9c92 Compare September 20, 2024 22:24
@Yadunund
Copy link
Member

I took the liberty to adopt the change into other tests in rclcpp and rclcpp_lifecyle. Now we're able to run all the tests with rmw_zenoh without anything crashing! 🎉

@fujitatomoya @alsora kindly review this PR whenever time permits!

Re-run CI after pushing changes:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

This was referenced Sep 20, 2024
Closed
Closed
clalancette
clalancette requested changes Sep 21, 2024
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Generally, these look good to me, but there is one that I think we should revamp.

rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp Outdated Show resolved Hide resolved
@ahcorde
Added feedback
9af7a4d 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 23, 2024

Still failing here

44: /home/ahcorde/ros2_rolling/src/ros2/rclcpp/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp:646: Failure
44: Value of: endpoint_gid_is_all_zeros
44:   Actual: true
44: Expected: false

@ahcorde ahcorde requested a review from clalancette September 23, 2024 12:14
clalancette
clalancette approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 23, 2024

Pulls: #2633
Gist: https://gist.githubusercontent.com/ahcorde/8d7c14e539372d6cfc1b4315d0a89483/raw/cd27611aeba4eb0fc8ae1722e70b2caaa82ac72d/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp rclcpp_lifecycle rclcpp_components --packages-above-and-dependencies rclcpp rclcpp_lifecycle rclcpp_components
TEST args: --packages-above rclcpp rclcpp_lifecycle rclcpp_components --packages-above rclcpp rclcpp_lifecycle rclcpp_components
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14596

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@Yadunund
Copy link
Member

Still failing here

44: /home/ahcorde/ros2_rolling/src/ros2/rclcpp/rclcpp/test/rclcpp/node_interfaces/test_node_graph.cpp:646: Failure
44: Value of: endpoint_gid_is_all_zeros
44:   Actual: true
44: Expected: false

This is something we need to fix in rmw_zenoh. The changes here are just to get all the tests to run without crashing.

@ahcorde looks like uncrustify is failing.

@ahcorde
make linters happy
88d9eb4 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@Yadunund
Copy link
Member

Yadunund commented Sep 23, 2024 •
edited by ahcorde
Loading

Jobs:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@Yadunund
Copy link
Member

Merging with green CI!

@Yadunund Yadunund merged commit 1f408e3 into rolling Sep 24, 2024
3 checks passed
@Yadunund Yadunund deleted the ahcorde/rolling/clean_rmw_zenoh_component_Test branch September 24, 2024 16:36
@Yadunund Yadunund mentioned this pull request Sep 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@Yadunund Yadunund Yadunund approved these changes

@clalancette clalancette clalancette approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees

@ahcorde ahcorde

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ahcorde @alsora @Yadunund @clalancette @fujitatomoya