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

wait_for_message Accumulating CPU Load After Repeated Calls #1179

Closed
KKSTB opened this issue Oct 3, 2023 · 11 comments
Closed

wait_for_message Accumulating CPU Load After Repeated Calls #1179

KKSTB opened this issue Oct 3, 2023 · 11 comments
Labels
help wanted Extra attention is needed

Comments

@KKSTB
Copy link
Contributor

KKSTB commented Oct 3, 2023

Bug report

Required Info:

  • Operating System:
    • Ubuntu 22.04
  • Installation type:
    • Binaries
  • Version or commit hash:
    • Iron
  • DDS implementation:
    • Fast-RTPS
  • Client library (if applicable):
    • rclpy

Steps to reproduce issue

  1. Setup and spin single threaded node executor in a new thread
  2. Call wait_for_message in main thread or another thread:
for i in range(100):
    is_success, msg = rclpy.wait_for_message.wait_for_message(msg_type, rclpy_node, topic, time_to_wait=time_to_wait)

Expected behavior

CPU% stays the same after the for loop

Actual behavior

CPU% increased and does not drop

Additional information

py-spy record -n -r 10 -p [PID] indicates high CPU load at _take_subscription (rclpy/executors.py 338 https://github.com/ros2/rclpy/blob/e9bd428ed562019596e8d3964489bab90a85b203/rclpy/rclpy/executors.py#L338C25-L338C43).

KKSTB added a commit to KKSTB/rclpy that referenced this issue Oct 3, 2023
@KKSTB
Copy link
Contributor Author

KKSTB commented Oct 3, 2023

Fix is simple: just unsubscribe before return (tested). This should work assuming that there is no exception in between.

@clalancette
Copy link
Contributor

clalancette commented Oct 3, 2023

Fix is simple: just unsubscribe before return (tested). This should work assuming that there is no exception in between.

Yes, we should definitely destroy the subscription. I think we should put it into a try..finally block, so we make sure it is always destroyed. Would you mind submittting a pull request to do that?

@fujitatomoya fujitatomoya added the help wanted Extra attention is needed label Oct 3, 2023
KKSTB added a commit to KKSTB/rclpy that referenced this issue Oct 4, 2023
@KKSTB
Copy link
Contributor Author

KKSTB commented Oct 4, 2023

Added try finally block and I tested it can destroy_subscription when:

  1. returning normally
  2. there is exception in the middle of the code, without catching the exception

KKSTB added a commit to KKSTB/rclpy that referenced this issue Oct 9, 2023
KKSTB added a commit to KKSTB/rclpy that referenced this issue Oct 9, 2023
clalancette pushed a commit that referenced this issue Oct 9, 2023
* Fix to issue #1179

Signed-off-by: KKSTB <[email protected]>
@KKSTB KKSTB closed this as completed Oct 10, 2023
@fujitatomoya
Copy link
Collaborator

@KKSTB i was thinking probably we want to backport this to iron and humble, since this affects user platform. what do you think? besides, the issue was originally created with iron.

CC: @Yadunund @clalancette

KKSTB added a commit to KKSTB/rclpy that referenced this issue Oct 11, 2023
@KKSTB
Copy link
Contributor Author

KKSTB commented Oct 11, 2023

Ah you are right. Thank you for reminding me.

fujitatomoya pushed a commit that referenced this issue Oct 11, 2023
@FelixLittleFei
Copy link

@KKSTB @fujitatomoya @clalancette
Dear ROS2 developers,
Could you please backport to ROS2 Humble for this wait_for_message function?
Screenshot from 2023-12-20 15-49-31

I noticed that it already exists in Rolling and Iron.
Thank you!

@KKSTB
Copy link
Contributor Author

KKSTB commented Dec 21, 2023

Hi @FelixLittleFei

Actually I tested that the wait_for_message function is also affected by this issue:
#1206

If wait_for_message is called many times the executor may sometimes throw exception saying it's using a destroyed subscription.

So I actually gave up in using this function in my application. I changed to permanently subscribe to topic instead.

@tiesus
Copy link

tiesus commented Apr 19, 2024

Hi, we would like to use this in humble as well. Is there any progress regarding the backport?

@fujitatomoya
Copy link
Collaborator

@tiesus currently nobody is working on that, but i think that is acceptable. CC: @clalancette

fujitatomoya pushed a commit to fujitatomoya/rclpy that referenced this issue Apr 21, 2024
* Fix to issue ros2#1179

Signed-off-by: KKSTB <[email protected]>
fujitatomoya pushed a commit to fujitatomoya/rclpy that referenced this issue Apr 21, 2024
* Fix to issue ros2#1179

Signed-off-by: KKSTB <[email protected]>
Signed-off-by: Tomoya Fujita <[email protected]>
@fujitatomoya
Copy link
Collaborator

humble backport: #1272

@tiesus
Copy link

tiesus commented Apr 22, 2024

Great. That helps us a lot to get rid of ros1.

fujitatomoya added a commit that referenced this issue Jun 25, 2024
* Add feature of wait for message (#953). (#960)

* Add feature of wait for message.

Signed-off-by: Lei Liu <[email protected]>
Signed-off-by: Tomoya Fujita <[email protected]>

* Fix to issue #1179 (#1180)

* Fix to issue #1179

Signed-off-by: KKSTB <[email protected]>
Signed-off-by: Tomoya Fujita <[email protected]>

* Allow specifying qos (#1225)

* Allow specifying qos

Signed-off-by: Tim Clephas <[email protected]>

---------

Signed-off-by: Lei Liu <[email protected]>
Signed-off-by: Tomoya Fujita <[email protected]>
Signed-off-by: KKSTB <[email protected]>
Signed-off-by: Tim Clephas <[email protected]>
Co-authored-by: Lei Liu <[email protected]>
Co-authored-by: KKSTB <[email protected]>
Co-authored-by: Tim Clephas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants