[Galactic] Backport pybind11 changes by sloretz · Pull Request #803 · ros2/rclpy

sloretz · 2021-06-23T23:26:18Z

This backports the Pybind11 changes that were intended for Galactic but couldn't be merged due to the Galactic freeze. #777

* 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>

* 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> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* 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> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* 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> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* Convert WaitSet to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: ahcorde <ahcorde@gmail.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* 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> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* 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> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: ahcorde <ahcorde@gmail.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

* 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> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz · 2021-06-23T23:28:01Z

Full CI (repos URL: https://raw.githubusercontent.com/ros2/ros2/galactic/ros2.repos)

Linux
Linux-aarch64
macOS
Windows

sloretz · 2021-06-24T17:25:20Z

CI LGTM! ros2param.ros2param.test.test_verb_list.test_verb_list and ros2param.ros2param.test.test_verb_load.test_verb_load are known flaky tests - Here's a Galactic nigthly where they failed https://build.ros2.org/view/Gci/job/Gci__nightly-release_ubuntu_focal_amd64/8/

sloretz · 2021-07-23T18:20:52Z

@ivanpauno May I ask you to double check this PR before I merge it?

ivanpauno

I have only reviewed modifications in .py files, including tests, to detect possible API breaking changes.
I don't have time to look at all the changes in the cpp files, it would be good if someone that originally worked in these changes can take another look to this PR.

I'm okay with the backport if we're not breaking API and CI is green.

ivanpauno · 2021-08-03T13:35:14Z

rclpy/test/test_destruction.py

-            with sub.handle:
-                node.destroy_subscription(sub)
-                # handle valid because it's still being used
-                with sub.handle:
-                    pass
+            node.destroy_subscription(sub)


why this test changed?
is the subscription still valid after the node.destroy_subscription() call?

same question applies to the similar changes in this file and others

It was changed to prevent a case where destruction could be blocked forever.
There's a longer description in the body of #739. In short, the subscription is still valid to existing users, but no new users are allowed. This prevents new users extending an object's lifetime indefinitely.

mmm, I don't understand exactly, I will have to look at the code a bit more.

What does happen with the old code snippet now?
Will it throw an InvalidHandle exception? Will it block?

I would expect the inner with sub.handle to raise InvalidHandle.

Okay, I understand the change now.
Do we still the Handle class? It seems to have been replaced with shared ptrs and Destroyable (I'm not sure if completely though).

There's a minimal chance this breaks pre-existing code, but I don't think anybody was relying on this behavior.

On master the Handle class is gone, but I'm not sure which PR to backport. The only use I see of it on this branch is in test_handle.py.

rclpy/test/test_subscription.py

ivanpauno

LGTM with green full CI.

I would also prefer if someone involved in the original work can left an approval.

hidmic

I skimmed through the PR. It looks good. I left a few comments, nothing to address here.

I do have to ask though: are we absolutely sure we want to backport over ~3000 lines of code into an stable distribution?

hidmic · 2021-08-09T21:02:13Z

rclpy/rclpy/context.py

+            try:
+                if self.__context is not None:
+                    raise RuntimeError
+            except AttributeError:


@sloretz nit: I know this is in master, but it's still unusual. Why not set self.__context to None in Context.__init__()?

Hmm this is strange. I opened #812 to try setting it to None in __init__()

hidmic · 2021-08-09T21:14:10Z

rclpy/src/rclpy/action_client.cpp

+          rcl_get_error_string().str);
+        rcl_reset_error();
+      }
+      delete action_client;


@sloretz meta: in retrospect, I wonder if we should've used a Python allocator-backed std::allocator for this.

Oh yeah! You mean like the allocator rclpy::Client is using? Looks lt's the only one using it.

rclpy/rclpy/src/rclpy/client.cpp

Lines 55 to 70 in fbe9ad1

rcl_client_ = std::shared_ptr<rcl_client_t>(

PythonAllocator<rcl_client_t>().allocate(1),

[node](rcl_client_t * client)

{

// Intentionally capture node by value so shared_ptr can be transferred to copies

rcl_ret_t ret = rcl_client_fini(client, node.rcl_ptr());

if (RCL_RET_OK != ret) {

// Warning should use line number of the current stack frame

int stack_level = 1;

PyErr_WarnFormat(

PyExc_RuntimeWarning, stack_level, "Failed to fini client: %s",

rcl_get_error_string().str);

rcl_reset_error();

}

PythonAllocator<rcl_client_t>().deallocate(client, 1);

});

hidmic · 2021-08-09T21:25:11Z

rclpy/src/rclpy/context.cpp

-    }
-  }
-  PyMem_FREE(context);
+  rcl_context_.reset();


@sloretz meta: one thing that's probably not great is that, if it weren't for the consistent with obj: where obj is a Destroyable, you could easily crash the process by calling APIs after an explicit destroy() -- even in a single threaded setting where there's no risk for concurrent destruction making that with block somewhat unnecessary.

Hmm yeah, maybe that with obj: logic could be moved into the APIs themselves. Maybe a DestroyableScopeGuard or something with https://pybind11.readthedocs.io/en/stable/advanced/functions.html#call-guard that did the equivalent of __enter__ and __exit__ could replace that, and get rid of one of the jumps between Python and C++ at the same time.

An implicit call guard would do, yeah.

sloretz · 2021-08-11T18:03:15Z

I do have to ask though: are we absolutely sure we want to backport over ~3000 lines of code into an stable distribution?

That's a great question to ask. I think it's important in this case since it enables more backports, and makes the destruction handling of entities consistent.

sloretz · 2021-08-11T18:53:32Z

CI (galactic repos file + this branch, build: --packages-above-and-dependencies rclpy test: --packages-above rclpy)

Linux
Linux-aarch64
macOS
Windows

sloretz · 2021-08-11T22:51:05Z

CI LGTM, merging 🎉

ahcorde and others added 10 commits June 23, 2021 16:17

Convert WaitSet to use C++ Classes (#769)

7595292

* Convert WaitSet to use C++ Classes Signed-off-by: ahcorde <ahcorde@gmail.com> * added feedback Signed-off-by: ahcorde <ahcorde@gmail.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Removed unused structs (#770)

c04915c

Signed-off-by: ahcorde <ahcorde@gmail.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Pybind11 actionserver nitpicks and docblock improvements (#774)

ee34534

Signed-off-by: ahcorde <ahcorde@gmail.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

sloretz self-assigned this Jun 23, 2021

sloretz requested a review from ivanpauno July 23, 2021 18:20

ivanpauno reviewed Aug 3, 2021

View reviewed changes

sloretz mentioned this pull request Aug 4, 2021

Remove unused function make_mock_subscription (backport #809) #810

Merged

ivanpauno approved these changes Aug 9, 2021

View reviewed changes

hidmic self-requested a review August 9, 2021 17:06

hidmic approved these changes Aug 9, 2021

View reviewed changes

sloretz mentioned this pull request Aug 11, 2021

Set Context.__context to None in __init__ #812

Merged

sloretz merged commit 4233111 into galactic Aug 11, 2021

delete-merged-branch bot deleted the backport_pybind11_galactic branch August 11, 2021 22:51

	rcl_client_ = std::shared_ptr<rcl_client_t>(
	PythonAllocator<rcl_client_t>().allocate(1),
	[node](rcl_client_t * client)
	{
	// Intentionally capture node by value so shared_ptr can be transferred to copies
	rcl_ret_t ret = rcl_client_fini(client, node.rcl_ptr());
	if (RCL_RET_OK != ret) {
	// Warning should use line number of the current stack frame
	int stack_level = 1;
	PyErr_WarnFormat(
	PyExc_RuntimeWarning, stack_level, "Failed to fini client: %s",
	rcl_get_error_string().str);
	rcl_reset_error();
	}
	PythonAllocator<rcl_client_t>().deallocate(client, 1);
	});

Conversation

sloretz commented Jun 23, 2021

Uh oh!

sloretz commented Jun 23, 2021

Uh oh!

sloretz commented Jun 24, 2021

Uh oh!

sloretz commented Jul 23, 2021

Uh oh!

ivanpauno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ivanpauno left a comment

Choose a reason for hiding this comment

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sloretz commented Aug 11, 2021

Uh oh!

sloretz commented Aug 11, 2021

Uh oh!

sloretz commented Aug 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants