ros2 / rclpy

rclpy (ROS Client Library for Python)
Apache License 2.0
268 stars 221 forks source link

Make rclpy initialization context-manager aware. #1298

Closed clalancette closed 1 week ago

clalancette commented 2 weeks ago

This PR does two somewhat controversial things:

  1. It adds in a new "rclpy.managed_init" method to rclpy, which would be used with a context manager instead of rclpy.init. I had to do things this way because there is no way (as far as I know) for the callee to know whether it was called within a context manager or not, and I don't think we can reasonably change the semantics of rclpy.init() at this point.

  2. It changes the context manager implementation of Context so that it does not call init() within itself. This definitely changes the semantics, but as it stands that initialization doesn't make sense because it can't take arguments. I think this change is warranted, though we may have to search through documentation and examples to make sure this doesn't break anything.

@sloretz I particularly am interested in your feedback on these changes.

clalancette commented 2 weeks ago

After discussing this with @sloretz , we don't need the managed_init method at all; we can just return the context from init(). I've now done this in 25a26a5 .

clalancette commented 2 weeks ago

CI:

clalancette commented 2 weeks ago

All right, I've now heavily revamped this based on feedback from @sloretz and @wjwwood . In particular, we now are doing a couple of different things:

  1. We have a proxy initialization object where we deal with initialization and shutdown. This allows us to do all of the work we need to do when initializing and shutting down, and also allows us to do it with a context manager.
  2. We track all nodes associated with contexts, and properly destroy the nodes when the context is shutdown. Since nodes also shutdown all pubs, subs, clients, servers, and timers associated with it when it shuts down, this means that it is now enough to do:
with rclpy.init():
    node = rclpy.create_node()
    pub = node.create_publisher()
    sub = node.create_subscription()

And everything will be properly cleaned up after when the context is exited.

Please take another look when you get a chance.

clalancette commented 2 weeks ago

Here is new CI:

clalancette commented 2 weeks ago

New CI:

clalancette commented 2 weeks ago

All right, after responding to review, here is another set of CI: