Make rclpy.try_shutdown() behavior to follow rclpy.shutdown() more closely #868
Conversation
…osely Signed-off-by: Ivan Santiago Paunovic <[email protected]>
| g_default_context = Context() | ||
| g_default_context.try_shutdown() | ||
| if not g_default_context.ok(): | ||
| g_default_context = None |
There was a problem hiding this comment.
I wish the default context registered an on_shutdown callback to manage the global variable too.
There was a problem hiding this comment.
Currently all the work is done with the default context lock taken.
If we register an on_shutdown() callback I don't think that would work.
Maybe it's possible to do that with some extra trick.
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
|
I had one extra issue related to #868 (comment) after doing some further testing. d65b487 fixes that. |
| self._callbacks.append(weakref.WeakMethod(callback, self._remove_callback)) | ||
| else: | ||
| self._callbacks.append(weakref.ref(callback, self._remove_callback)) | ||
| self._callbacks.append(callback) |
There was a problem hiding this comment.
I could also see the case of always retaining shared ownership and the user explictily passing a weak object when they need that, but that would be an unexpected change (particularly for methods).
Oh hmm. This is interesting. I don't see a better way, and it does seem like the behavior would be intuitive.
|
CI for this PR alone (I will rerun PR for ros2/ros2cli#683 after merging this one):
|
delete-merged-branch
bot
deleted the
ivanpauno/make-try-shutdonwn-behavior-match-shutdown
branch
Needed by ros2/ros2cli#683.
I'm not sure if this is the ideal approach, maybe init()/shutdown() are doing too much (?).
I'm open to alternatives.