Convert Publisher and Subscription to use C++ Classes by ahcorde · Pull Request #756 · ros2/rclpy

ahcorde · 2021-04-06T11:08:59Z

Part of #665.

This converts the Publisher and Subscription functions to a C++ classes.

I have some issue with the QoSEvent destructor in some of the tests:

Thread 29 "python3" received signal SIGSEGV, Segmentation fault.
[Cambiando a Thread 0x7fffa3fff700 (LWP 338069)]
0x00000000006152cf in _is_legal_capsule (invalid_capsule=0x6d7160 "PyCapsule_GetPointer called with invalid PyCapsule object", capsule=0x3) at ../Objects/capsule.c:84
84	../Objects/capsule.c: No existe el archivo o el directorio.
(gdb) bt
#0  0x00000000006152cf in _is_legal_capsule (invalid_capsule=0x6d7160 "PyCapsule_GetPointer called with invalid PyCapsule object", capsule=0x3) at ../Objects/capsule.c:84
#1  PyCapsule_GetPointer (o=<unknown at remote 0x3>, name=0x7ffff5985881 "rcl_node_t") at ../Objects/capsule.c:84
#2  0x00007ffff6c182fb in rclpy_handle_get_pointer_from_capsule () from /home/ahcorde/ros2_galactic/install/lib/librclpy_common.so
#3  0x00007ffff5932510 in rclpy::Handle::rcl_ptr(char const*) () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#4  0x00007ffff5918bc3 in rcl_node_t* rclpy::Handle::cast_or_warn<rcl_node_t*>(char const*) () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#5  0x00007ffff593f2c4 in rclpy::Publisher::Publisher(pybind11::capsule, pybind11::object, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, pybind11::object)::{lambda(rcl_publisher_t*)#1}::operator()(rcl_publisher_t*) const [clone .lto_priv.0] () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#6  0x00007ffff594567e in std::_Sp_counted_deleter<rcl_publisher_t*, rclpy::Publisher::Publisher(pybind11::capsule, pybind11::object, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, pybind11::object)::{lambda(rcl_publisher_t*)#1}, std::allocator<void>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() [clone .lto_priv.0] ()
   from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#7  0x00007ffff58f464e in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#8  0x00007ffff58edfbb in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#9  0x00007ffff58ec990 in std::__shared_ptr<rcl_publisher_t, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() ()
   from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#10 0x00007ffff58ec9d8 in std::shared_ptr<rcl_publisher_t>::~shared_ptr() [clone .lto_priv.0] () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#11 0x00007ffff588c75c in rclpy::QoSEvent::~QoSEvent() [clone .lto_priv.0] () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#12 0x00007ffff5953d36 in rclpy::QoSEvent::~QoSEvent() () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#13 0x00007ffff5953d76 in std::default_delete<rclpy::QoSEvent>::operator()(rclpy::QoSEvent*) const () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#14 0x00007ffff588bc24 in std::unique_ptr<rclpy::QoSEvent, std::default_delete<rclpy::QoSEvent> >::~unique_ptr() [clone .lto_priv.0] ()
   from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#15 0x00007ffff594c117 in pybind11::class_<rclpy::QoSEvent, rclpy::Destroyable>::dealloc(pybind11::detail::value_and_holder&) ()
   from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#16 0x00007ffff589ac2f in pybind11::detail::clear_instance(_object*) () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so
#17 0x00007ffff589ad30 in pybind11_object_dealloc () from /home/ahcorde/ros2_galactic/build/rclpy/test_rclpy/_rclpy_pybind11.cpython-38-x86_64-linux-gnu.so

Do you mind to have a look @hidmic or @sloretz ?

Signed-off-by: ahcorde ahcorde@gmail.com

Signed-off-by: ahcorde <ahcorde@gmail.com>

hidmic

Thread 29 "python3" received signal SIGSEGV, Segmentation fault.

If I'm not mistaken, the issue is that QoSEvent no longer holds a Handle to an rcl_subscription_t, but the rcl_subscription_t itself. Destruction order w.r.t. your node handle is no longer guaranteed.

I believe the plan was to move away from Handle instances and simply use std::shared_ptr instances. Is that right @sloretz? If so, QoSEvent should keep an std::shared_ptr to its parent, which in turn keeps an std::shared_ptr to the node.

hidmic · 2021-04-06T15:39:09Z

rclpy/src/rclpy/publisher.cpp

+    new rcl_publisher_t,
+    [this](rcl_publisher_t * publisher)
+    {
+      if (!node_handle_) {


@ahcorde how can this ever happen? IIRC the whole purpose of Handle is to ensure ordered destruction.

maybe there is some wrong with the destructor somewhere else, but If I remove this lines then the test_qos_event will fail

Indeed. Default destructors for Publisher and Subscription are dropping node_handle_ too early (and thus leaking!). Inverting member fields order or a custom destructor should do.

rclpy/src/rclpy/publisher.hpp

rclpy/src/rclpy/subscription.cpp

rclpy/src/rclpy/subscription.hpp

rclpy/test/test_destruction.py

sloretz · 2021-04-06T16:12:51Z

If I'm not mistaken, the issue is that QoSEvent no longer holds a Handle to an rcl_subscription_t, but the rcl_subscription_t itself. Destruction order w.r.t. your node handle is no longer guaranteed.

I believe the plan was to move away from Handle instances and simply use std::shared_ptr instances. Is that right @sloretz? If so, QoSEvent should keep an std::shared_ptr to its parent, which in turn keeps an std::shared_ptr to the node.

Ohhh shoot. I was thinking the QoSEvent just needed to keep an std::shared_ptr to the parent rcl type to keep that alive, but I forgot that doesn't keep it's grandparent, the Node, alive.

You're right that we'll need a shared pointer to all the ancestors. I'm not sure what that looks like to implement, but a shared pointer to the parent wrapper sounds reasonable.

Signed-off-by: ahcorde <ahcorde@gmail.com>

…m_this (#757) * Use std::shared_ptr holder type Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Use shared_from_this for pub/sub Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Make Timer keep Clock wrapper Signed-off-by: Shane Loretz <sloretz@openrobotics.org>

Signed-off-by: ahcorde <ahcorde@gmail.com>

rclpy/rclpy/publisher.py

rclpy/src/rclpy/publisher.hpp

rclpy/src/rclpy/qos_event.hpp

rclpy/src/rclpy/publisher.cpp

rclpy/src/rclpy/subscription.hpp

rclpy/src/rclpy/utils.cpp

Signed-off-by: ahcorde <ahcorde@gmail.com>

hidmic

LGTM pending green CI

rclpy/rclpy/publisher.py

rclpy/src/rclpy/subscription.hpp

rclpy/src/rclpy/publisher.hpp

rclpy/src/rclpy/qos_event.hpp

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-04-07T18:30:47Z

Building up-to rclpy and testing rclpy

Linux
Linux-aarch64
macOS
Windows

ahcorde · 2021-04-08T06:17:47Z

Thank for retargeting the PR, merging!

* Convert Publisher and Subscription to C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed destructor issues Signed-off-by: ahcorde <ahcorde@gmail.com> * [pub/sub 756 addition] use std::shared_ptr holder type and shared_from_this (ros2#757) * Use std::shared_ptr holder type Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Use shared_from_this for pub/sub Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Make Timer keep Clock wrapper Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Fixed publisher and subscription destructors Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed docs Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Shane Loretz <sloretz@openrobotics.org>

* Convert Publisher and Subscription to use C++ Classes (#756) * Convert Publisher and Subscription to C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed destructor issues Signed-off-by: ahcorde <ahcorde@gmail.com> * [pub/sub 756 addition] use std::shared_ptr holder type and shared_from_this (#757) * Use std::shared_ptr holder type Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Use shared_from_this for pub/sub Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Make Timer keep Clock wrapper Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Fixed publisher and subscription destructors Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed docs Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Shane Loretz <sloretz@openrobotics.org> * Use py::class_ for rcl_action_goal_handle_t (#751) * Class object Signed-off-by: Greg Balke <greg@openrobotics.org> * Python adjustments Signed-off-by: Greg Balke <greg@openrobotics.org> * Adjustments to server and enum Signed-off-by: Greg Balke <greg@openrobotics.org> * Adjust destructor calls Signed-off-by: Greg Balke <greg@openrobotics.org> * Clear out destructor Signed-off-by: Greg Balke <greg@openrobotics.org> * Do what rclcpp did Signed-off-by: Greg Balke <greg@openrobotics.org> * Remove iostream (used for debugging) Signed-off-by: Greg Balke <greg@openrobotics.org> * Matching upstream changes Signed-off-by: Greg Balke <greg@openrobotics.org> * Convert ActionClient to use C++ classes (#759) * Convert ActionClient to use C++ classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Added convert_from_py to utils Signed-off-by: ahcorde <ahcorde@gmail.com> * improved docs Signed-off-by: ahcorde <ahcorde@gmail.com> * nits to adapt the code to pybind11 Signed-off-by: ahcorde <ahcorde@gmail.com> * throw a exception in convert_from_py Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed docs Signed-off-by: ahcorde <ahcorde@gmail.com> * Convert ActionServer to use C++ Classes (#766) * Convert ActionServer to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed Test_action_server Signed-off-by: ahcorde <ahcorde@gmail.com> * Improved docs Signed-off-by: ahcorde <ahcorde@gmail.com> * Improved docs and minor fixes Signed-off-by: ahcorde <ahcorde@gmail.com> * Convert WaitSet to use C++ Classes (#769) * Convert WaitSet to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Removed unused structs (#770) Signed-off-by: ahcorde <ahcorde@gmail.com> * Convert Guardcondition to use C++ classes (#772) * Convert Guardcondition to use C++ classes Signed-off-by: ahcorde <ahcorde@gmail.com> * make destroy guard condition capsule function private Signed-off-by: ahcorde <ahcorde@gmail.com> * Pybind11 action goal handle nitpicks (#767) * Add GoalEvent back to module Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * self._goal -> self._goal_handle Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Remove unused API Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Co-authored-by: ahcorde <ahcorde@gmail.com> * Pybind11 actionserver nitpicks and docblock improvements (#774) Signed-off-by: ahcorde <ahcorde@gmail.com> * Convert Node and Context to use C++ Classes (#771) * convert Node and Context to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Implement solution 1 Signed-off-by: ahcorde <ahcorde@gmail.com> * Prevent double early destruction of destroyable This along with an extra call to `destroy_node()` in `Node.__del__` was causing a segfault in the test_publisher test. The problem is `destroy_when_not_in_use()` was being called twice, and the second time is now a problem because rcl_ptrs_ is dereferenced in the qos_event destroy() method. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Remove unnecessary Node.__del__ This should not be required. If it is, it means there's a bug that shows when the Node is garbage collected. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix order of pointers in RclPtrs structs The order of the pointers in the RclPtrs structs is important. Things at the bottom are destroyed first, so children must be placed underneath the parent's that must be kept alive. Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Intentionally copy rclpy:: objects instead of having RclPtrs structs (#773) * Rough pass converting everything Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fixes to get copy destruction order working Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Fix qos_event test flakiness Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Signed-off-by: ahcorde <ahcorde@gmail.com> * MacOS warning Signed-off-by: ahcorde <ahcorde@gmail.com> * added feedback and removed some files Signed-off-by: ahcorde <ahcorde@gmail.com> * Create only one context otherwise throw a exception Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Shane Loretz <sloretz@osrfoundation.org> Co-authored-by: Shane Loretz <sloretz@openrobotics.org> Co-authored-by: Alejandro Hernández Cordero <alejandro@openrobotics.org> Co-authored-by: Greg Balke <11024792+gbalke@users.noreply.github.com> Co-authored-by: ahcorde <ahcorde@gmail.com>

* Convert Publisher and Subscription to C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed destructor issues Signed-off-by: ahcorde <ahcorde@gmail.com> * [pub/sub 756 addition] use std::shared_ptr holder type and shared_from_this (#757) * Use std::shared_ptr holder type Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Use shared_from_this for pub/sub Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Make Timer keep Clock wrapper Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Fixed publisher and subscription destructors Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed docs Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Shane Loretz <sloretz@openrobotics.org>

* Convert Publisher and Subscription to C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed destructor issues Signed-off-by: ahcorde <ahcorde@gmail.com> * [pub/sub 756 addition] use std::shared_ptr holder type and shared_from_this (#757) * Use std::shared_ptr holder type Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Use shared_from_this for pub/sub Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Make Timer keep Clock wrapper Signed-off-by: Shane Loretz <sloretz@openrobotics.org> * Fixed publisher and subscription destructors Signed-off-by: ahcorde <ahcorde@gmail.com> * Added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> * Fixed docs Signed-off-by: ahcorde <ahcorde@gmail.com> Co-authored-by: Shane Loretz <sloretz@openrobotics.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Convert Publisher and Subscription to C++ Classes

0048bb9

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde added the enhancement New feature or request label Apr 6, 2021

ahcorde requested a review from hidmic April 6, 2021 11:08

ahcorde self-assigned this Apr 6, 2021

hidmic reviewed Apr 6, 2021

View reviewed changes

Fixed destructor issues

8cc6ca0

Signed-off-by: ahcorde <ahcorde@gmail.com>

This was referenced Apr 6, 2021

Use Pybind11 for rclpy Python Bindings #665

Closed

[pub/sub 756 addition] use std::shared_ptr holder type and shared_from_this #757

Merged

Use std::shared_ptr holder type for Destroyable subsclasses to keep grandparent wrapper instances alive #758

Closed

ahcorde requested a review from hidmic April 7, 2021 07:32

Fixed publisher and subscription destructors

cd159f9

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde mentioned this pull request Apr 7, 2021

Convert ActionClient to use C++ classes #759

Merged

hidmic reviewed Apr 7, 2021

View reviewed changes

Added feedback

23136bd

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from hidmic April 7, 2021 15:51

hidmic approved these changes Apr 7, 2021

View reviewed changes

ivanpauno mentioned this pull request Apr 7, 2021

Fix crash at shutdown of rqt_console plugin ros-visualization/rqt_console#30

Closed

Fixed docs

fc721ee

Signed-off-by: ahcorde <ahcorde@gmail.com>

ivanpauno mentioned this pull request Apr 7, 2021

An rcl_event_t might be destroyed while a waitset is still using it #760

Closed

sloretz mentioned this pull request Apr 7, 2021

Use py::class_ for rcl_action_goal_handle_t #751

Merged

sloretz changed the base branch from master to post_galactic_freeze April 7, 2021 22:38

ahcorde merged commit 2a4eac4 into post_galactic_freeze Apr 8, 2021

delete-merged-branch bot deleted the ahcorde/pybind11-pubsub branch April 8, 2021 06:17

Conversation

ahcorde commented Apr 6, 2021

Uh oh!

hidmic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

ahcorde Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

hidmic Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sloretz commented Apr 6, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahcorde commented Apr 7, 2021

Uh oh!

ahcorde commented Apr 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hidmic left a comment •

edited

Loading