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

Make rclpy initialization context-manager aware. #1298

Merged
merged 7 commits into from
Jun 17, 2024

Conversation

clalancette
Copy link
Contributor

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.

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.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
That way, we don't need managed_init() anymore.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor Author

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
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

rclpy/rclpy/context.py Outdated Show resolved Hide resolved
That way we can use context managers, but actually properly
clean up, including uninstalling signal handlers.

We also switch to tracking node resources in the context,
so that when the context goes away, all nodes associated with
it are automatically destroyed.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor Author

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
Copy link
Contributor Author

Here is new CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor Author

New CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

This is looking great!

rclpy/rclpy/context.py Outdated Show resolved Hide resolved
rclpy/rclpy/context.py Outdated Show resolved Hide resolved
rclpy/rclpy/context.py Outdated Show resolved Hide resolved
rclpy/rclpy/context.py Outdated Show resolved Hide resolved
rclpy/rclpy/context.py Show resolved Hide resolved
clalancette and others added 3 commits June 17, 2024 09:07
Co-authored-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Co-authored-by: Shane Loretz <sloretz@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Contributor Author

clalancette commented Jun 17, 2024

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

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@clalancette clalancette merged commit cef4ce9 into rolling Jun 17, 2024
3 checks passed
@clalancette clalancette deleted the clalancette/context-managers branch June 17, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants