-
Notifications
You must be signed in to change notification settings - Fork 225
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
executor.spin_once crashes after destroy_subscription called in separate thread #255
Comments
To reproduce the crash it is important that |
It is not, but the client libraries should be. This is definitely a bug in rclpy, but it might be related to using QThread (since it also has C extension code), can this be done with just |
I can also reproduce the issue with threading.Thread. I updated the example
to remove the PyQt5 dependence.
…On Wed, Dec 19, 2018 at 1:30 PM William Woodall ***@***.***> wrote:
I suppose rmw is not designed to be threadsafe.
It is not, but the client libraries should be. This is definitely a bug in
rclpy, but it might be related to using QThread (since it also has C
extension code), can this be done with just threading.Thread?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#255 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA_x-oVkPnUBJssqsmLCluSO5q8qTO9iks5u6q_0gaJpZM4ZbAgM>
.
|
I'm not sure destroying the subscription is the right thing to do. In C++ you just let the subscription go out of scope, then the executor continues to hold ownership until it wakes up at which point it will release its lock too and the subscription can be destroyed. @sloretz may have better insight into what should be done here or if the design of rclpy should be changed in some way. |
Following up, it turns out that other destroy_* methods are not thread-safe, including destroy_node, and destroy_timer. See the test functions in |
It looks like this is a related issue with similar discussion #192 |
Reproducible example with a timerimport threading
import time
import rclpy
from rclpy.executors import SingleThreadedExecutor
rclpy.init()
# Background executor for convenience
executor = SingleThreadedExecutor()
t = threading.Thread(target=executor.spin, daemon=True)
t.start()
try:
node = rclpy.create_node('TestDestroy')
timer = node.create_timer(0.1, lambda: print('Timer called'))
executor.add_node(node)
time.sleep(1)
# All done with timer, better destroy it right?
node.destroy_timer(timer)
finally:
# Shutdown the executor since there's nothing left to execute
executor.shutdown()
rclpy.shutdown() Console output:
IIUC the First File objects in python seem like good model to follow. Their memory is Unfortunately, |
This is a follow up from ros-visualization/rqt#182. Destroying subscriptions is not thread safe. Per this comment ros-visualization/rqt#181 (comment) I suppose rmw is not designed to be threadsafe. It would be very helpful for overall stability if there was a thread management solution in the client libraries.
Bug report
Required Info:
Steps to reproduce issue
Minimal example here https://github.com/brawner/crashing_subscription_destroyer
Change the code in
subscriber.py
to see an example that does not crashExpected behavior
Node continues to spin and subscription is ended
Actual behavior
Locking/or thread management is required to safely destroy a subscription
Stacktrace
The text was updated successfully, but these errors were encountered: