Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Fix "roscpp multithreaded spinners eat up CPU when callbacks take too long"#2377

Merged
sloretz merged 2 commits intoros:noetic-develfrom
meyerj:fix_subscription_busy_wait-noetic
May 5, 2025
Merged

Fix "roscpp multithreaded spinners eat up CPU when callbacks take too long"#2377
sloretz merged 2 commits intoros:noetic-develfrom
meyerj:fix_subscription_busy_wait-noetic

Conversation

@meyerj
Copy link
Copy Markdown
Contributor

@meyerj meyerj commented Oct 7, 2024

Resolves #2341:

Issue #1545 was fixed in ROS melodic, but not noetic.

The fix for the issue took a rather meandering rout to getting in, but PR #1684 and PR #2014 both seem related.

As far as I can tell, the offending functions in noetic are the same as in melodic pre fix, so this might be as easy as cherry picking those changes over.

These two commits were cherry-picked from branch melodic-devel, with some merge conflict resolutions in test/test_roscpp/test/test_callback_queue.cpp due to updates in #2121.

Not sure whether #2121 resolving #1980 is related and would not even have been necessary with these two commits from melodic-devel? I have not checked those two in detail yet.

cwecht and others added 2 commits October 7, 2024 18:14
@cwecht @peci1
@dirk-thomas @meyerj
* Fix a busy-wait loop in subscription queue.
a60cca0 
* Better fix for slow callbacks CPU throttling.

* More versatile callback queue test.

* Finished cherry-pick merge to melodic-devel.

* libros: moved define

* roscpp: implementet ros#1608 without ABI/API breaks

* /test_roscpp: fake_message is in a header now...

* test_roscpp: fixed sign-compare warning

* stabilized test

* CallbackQueue: use SteadyTime instead of WallTime to get independent of system-time changes

* style only

* Update clients/roscpp/include/ros/callback_queue.h

Co-authored-by: Johannes Meyer <johannes@intermodalics.eu>

Co-authored-by: Martin Pecka <peci1@seznam.cz>
Co-authored-by: CTU base <robot@ctu-base>
Co-authored-by: Martin Pecka <peckama2@fel.cvut.cz>
Co-authored-by: Christopher Wecht <christopher.wechtstudent.kit.edu>
Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Co-authored-by: Johannes Meyer <johannes@intermodalics.eu>
@dirk-thomas @meyerj
Fix subscription busy wait melodic (ros#2014)
df10b21 
* roscpp: simplify implementation of CallbackQueue::callOne()

Replace duplicate wait_for() call in CallbackQueue::callOne() with a loop and a call to wait_until().

* roscpp: return TryAgain from CallbackQueue::callOne(timeout) immediately if timeout.isZero()

... and if none of the other result conditions holds (i.e. the queue is Empty or Disabled).

Co-authored-by: Johannes Meyer <johannes@intermodalics.eu>
This was referenced Oct 7, 2024
Closed
Closed
@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Apr 25, 2025

Thank you for the PR!

ROS Noetic will reach end-of-life on May 31st, 2025. Every change comes with a risk of introducing regressions, and there isn't much time left to fix them. To make sure this PR doesn't introduce any regressions please:

  • Describe how you tested this change
  • Recruit at least one more person to review this PR and try it out on their system

@meyerj
Copy link
Copy Markdown
Contributor Author

meyerj commented May 2, 2025

I assume this is an automatic near end-of-life message posted to all pending pull requests?

The patches were reviewed, tested and merged into melodic-devel, see #1684 and #2014, and probably many users were already using them in ROS Melodic. Nothing fundamentally changed in ROS Noetic related to that, the patches just never ended up in noetic-devel for some reason and hence the old issue #1545 re-appeared for Noetic (#2341).

peci1
peci1 approved these changes May 4, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@peci1 peci1 left a comment

Choose a reason for hiding this comment

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

I've tested this PR locally.

All tests from test_roscpp succeed.

I also ran our student autonomous exploration pipeline (Turtlebot 3 in Gazebo Classic + custom control code) and everything seems to run correctly.

As this is a forward-port from Melodic, I guess it should not do any undiscovered harm.

@peci1
Copy link
Copy Markdown
Contributor

peci1 commented May 4, 2025

@meyerj Would you mind having a (reciprocal) look at #2250 and #2325?

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented May 5, 2025

Thanks for trying it out @peci1 !

@sloretz sloretz merged commit 114a142 into ros:noetic-devel May 5, 2025
@meyerj meyerj deleted the fix_subscription_busy_wait-noetic branch May 5, 2025 17:05
@amitsingh21 amitsingh21 mentioned this pull request Feb 19, 2026
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@peci1 peci1 peci1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

roscpp multithreaded spinners eat up CPU when callbacks take too long

5 participants

@meyerj @sloretz @peci1 @cwecht @dirk-thomas