Skip to content

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

Merged
Yadunund merged 6 commits intorollingfrom
ahcorde/rolling/clean_rmw_zenoh_component_Test
Sep 24, 2024
Merged

Shutdown the context before context's destructor is invoked in tests#2633
Yadunund merged 6 commits intorollingfrom
ahcorde/rolling/clean_rmw_zenoh_component_Test

Conversation

@ahcorde
Copy link
Copy Markdown
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 <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Sep 18, 2024
@ahcorde ahcorde mentioned this pull request Sep 18, 2024
Merged
@alsora
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Yadunund commented Sep 20, 2024

@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
Copy Markdown
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
Copy Markdown
Member

Yadunund commented Sep 20, 2024

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
Call rclcpp::shutdown when tearing down tests in rclcpp
2fa9c92 
Signed-off-by: Yadunund <yadunund@intrinsic.ai>
@Yadunund Yadunund force-pushed the ahcorde/rolling/clean_rmw_zenoh_component_Test branch from 4b50b61 to 2fa9c92 Compare September 20, 2024 22:24
@Yadunund
Call rclcpp::shutdown when tearing down tests in rclcpp_lifecycle
6d9741e 
Signed-off-by: Yadunund <yadunund@intrinsic.ai>
@Yadunund
Copy link
Copy Markdown
Member

Yadunund commented Sep 20, 2024

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
@Yadunund
Ensure context is initialized for all tests in test_publisher
f462ac1 
Signed-off-by: Yadunund <yadunund@intrinsic.ai>
clalancette
clalancette requested changes Sep 21, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@ahcorde
Added feedback
9af7a4d 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Yadunund 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

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 <ahcorde@gmail.com>
@Yadunund
Copy link
Copy Markdown
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
Copy Markdown
Member

Yadunund commented Sep 24, 2024

Merging with green CI!

@Yadunund Yadunund merged commit 1f408e3 into rolling Sep 24, 2024
@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

@clalancette clalancette clalancette approved these changes

@Yadunund Yadunund Yadunund approved these changes

@fujitatomoya fujitatomoya fujitatomoya 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