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.

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

Adjust for new rclcpp::Rate API #516

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Conversation

AlexeyMerzlyakov
Copy link
Contributor

This is a adjustement fix for the test regressions appeared in test_communication and test_security after applying new rclcpp::Rate API (ros2/rclcpp#2123).

The new rclcpp::Rate API after the change implies that RCLCPP context should exist at the rate.sleep() call time. However, internal test publisher's logic does not destroy the Rate objects after RCLCPP was stopped, which causes a runtime exceptions. Moreover, there are some tests (named **AfterShutdown), that intentionally runs the subscriber and publisher nodes (with rclcpp::Rate working inside) after the context was stopped and checks that everything is just closed correctly. This in our case is also causing the same run-time exception.

Thus, without large test modification, it is seems to be impossible to fix it by just changing the rclcpp::Rate usage logic. Therefore, it was chosen to handle the context cannot be slept with because it's invalid exceptions appears during the rclcpp::Rate usage w/o context. This exception raises in two cases described above, when test was already finished. So, this should be a simpler way to handle this, I suppose.

Copy link
Contributor

@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

Signed-off-by: Alexey Merzlyakov <alexey.merzlyakov@samsung.com>
@clalancette
Copy link
Contributor

CI for this is in ros2/rclcpp#2123 (comment)

@clalancette clalancette merged commit f803a3d into ros2:rolling Aug 31, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants