Skip to content

Check rclcpp::ok before sleeping for clean shutdown#3484

Merged
rhaschke merged 2 commits intomoveit:mainfrom
MarqRazz:clean_shutdown
May 28, 2025
Merged

Check rclcpp::ok before sleeping for clean shutdown#3484
rhaschke merged 2 commits intomoveit:mainfrom
MarqRazz:clean_shutdown

Conversation

@MarqRazz
Copy link
Copy Markdown
Contributor

@MarqRazz MarqRazz commented May 28, 2025 •
edited
Loading

Description

In MTC PR#684 I found move_group fails to shut down cleanly when preempting motion. This is because we are asking the node to sleep when the stack has been requested to shut down. To fix this I added a simple guard to make sure rclcpp::ok() before sleeping.

Here is a snippet of the stack trace when shutting down.

[move_group-5] #10   Source "/root/my_ws/src/moveit2/moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp", line 549, in scenePublishingThread [0x7d8db9c1afaf]
[move_group-5]         546:       planning_scene_publisher_->publish(msg);
[move_group-5]         547:       if (is_full)
[move_group-5]         548:         RCLCPP_DEBUG(logger_, "Published full planning scene: '%s'", msg.name.c_str());
[move_group-5]       > 549:       rate.sleep();
[move_group-5]         550:     }
[move_group-5] #9    Object "/opt/ros/jazzy/lib/librclcpp.so", at 0x7d8db914c480, in rclcpp::Rate::sleep()
[move_group-5] #8    Object "/opt/ros/jazzy/lib/librclcpp.so", at 0x7d8db90a0238, in rclcpp::Clock::sleep_for(rclcpp::Duration, std::shared_ptr<rclcpp::Context>)

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@MarqRazz MarqRazz requested a review from rhaschke May 28, 2025 15:17
@codecov
Copy link
Copy Markdown

codecov bot commented May 28, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.24%. Comparing base (7907501) to head (d178050).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3484      +/-   ##
==========================================
+ Coverage   45.84%   46.24%   +0.40%     
==========================================
  Files         718      718              
  Lines       62668    62690      +22     
  Branches     7587     7589       +2     
==========================================
+ Hits        28727    28985     +258     
+ Misses      33774    33538     -236     
  Partials      167      167

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

rhaschke
rhaschke reviewed May 28, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Is this a new issue, i.e. a regression in rclcpp? If so, that should be reported upstream.

@MarqRazz @rhaschke
Update moveit_ros/planning/planning_scene_monitor/src/planning_scene_…
d178050 
…monitor.cpp

Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
@MarqRazz MarqRazz mentioned this pull request May 28, 2025
Closed
@rhaschke rhaschke merged commit f4cf780 into moveit:main May 28, 2025
9 of 10 checks passed
@github-project-automation github-project-automation bot moved this to ✅ Done in MoveIt May 28, 2025
@MarqRazz MarqRazz added backport-humble Mergify label that triggers a PR backport to Humble backport-jazzy Mergify label that triggers a PR backport to Jazzy labels May 28, 2025
mergify bot pushed a commit that referenced this pull request May 28, 2025
@MarqRazz @mergify
PSM: finish thread on rclcpp::shutdown (#3484)
f81636b 
Calling rate.sleep() on shutdown will crash

(cherry picked from commit f4cf780)

# Conflicts:
#	moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp
@mergify mergify bot mentioned this pull request May 28, 2025
Merged
6 tasks
mergify bot pushed a commit that referenced this pull request May 28, 2025
@MarqRazz @mergify
PSM: finish thread on rclcpp::shutdown (#3484)
70ea871 
Calling rate.sleep() on shutdown will crash

(cherry picked from commit f4cf780)
@mergify mergify bot mentioned this pull request May 28, 2025
Merged
6 tasks
@MarqRazz MarqRazz deleted the clean_shutdown branch May 28, 2025 19:20
MarqRazz pushed a commit that referenced this pull request May 28, 2025
@mergify
PSM: finish thread on rclcpp::shutdown (#3484) (#3486)
5450b9a
valmikikothare added a commit to valmikikothare/moveit2 that referenced this pull request May 28, 2025
@valmikikothare
Merge pull request #1 from moveit/jazzy
70af5ea 
PSM: finish thread on rclcpp::shutdown (moveit#3484) (moveit#3486)
MarqRazz pushed a commit that referenced this pull request May 29, 2025
@mergify
Check rclcpp::ok before sleeping for clean shutdown (backport #3484) (#…
3f2d7f5 
…3485)
Markus-Simonsen pushed a commit to Markus-Simonsen/moveit2 that referenced this pull request Aug 12, 2025
@MarqRazz
PSM: finish thread on rclcpp::shutdown (moveit#3484)
1578196 
Calling rate.sleep() on shutdown will crash
helen9975 pushed a commit to personalrobotics/moveit2 that referenced this pull request Feb 17, 2026
@MarqRazz
PSM: finish thread on rclcpp::shutdown (moveit#3484)
624f99c 
Calling rate.sleep() on shutdown will crash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhaschke rhaschke rhaschke approved these changes

Assignees

No one assigned

Labels

backport-humble Mergify label that triggers a PR backport to Humble backport-jazzy Mergify label that triggers a PR backport to Jazzy

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MarqRazz @rhaschke