Use PyCapsule names #129

sloretz · 2017-10-18T23:00:42Z

Naming capsules prevents calling the wrong kind of cleanup on a pointer. This also removes the string parameter from rclpy_destroy_entity and rclpy_destroy_node_entity since it can be figured out from the capsule name.

If a non-capsule or a capsule of the wrong type is specified, the functions let PyCapsule_GetPointer raise.

Linux
Linux-aarch64
macOS
Windows

dirk-thomas · 2017-10-18T23:27:36Z

rclpy/rclpy/node.py

-        if not class_ or class_.__name__ != 'PyCapsule':
-            raise ValueError('The node handle must be a PyCapsule')
+        for sub in self.subscriptions:
+            _rclpy.rclpy_destroy_node_entity(sub.subscription_handle, self.handle)


The return value is not being considered anymore?

This is intentional. Currently rclpy_destroy_node_entity returns True on success or raises a RuntimeError on failure, so I got rid of the return value altogether.

I see. If the return value of the function doesn't have any purpose anymore I would suggest to return None instead (like any Python function without a return statement).

I believe that is what this PR is doing, raising and returning NULL on failure otherwise returning None:
https://github.com/ros2/rclpy/pull/129/files#diff-80dda03110b5be606b823d6b0558b5c2R1567

Sorry for the noise. I don't know what I was looking at.

mikaelarguedas

code change looks good to me, a few comment regarding docblock consistency throughout the PR

mikaelarguedas · 2017-10-20T21:03:30Z

rclpy/src/rclpy/_rclpy.c

  PyList_SET_ITEM(pylist, 1, PyLong_FromUnsignedLongLong((uint64_t)&gc->impl));

  return pylist;
 }

 /// Trigger a general purpose guard condition
 /**
+ * Raises ValueError if the guard condition is not a capsule of the correcttype
+ * Raises RuntimeError if the guard condition could not be triggered


Several other places do raise runtime error on this type of failure, maybe we should take advantage of this PR to update them all

I updated all the ones I found in 451a072

mikaelarguedas · 2017-10-20T21:03:55Z

rclpy/src/rclpy/_rclpy.c

@@ -597,6 +613,7 @@ rclpy_expand_topic_name(PyObject * Py_UNUSED(self), PyObject * args)
 * provided as pymsg_type to send messages over the wire.
 *
 * A ValueError is raised (and NULL returned) when the topic name is invalid.
+ * Raises ValueError if the capsules are not the correct types


Can you rephrase the sentence above to match this one as well?

Done 451a072

mikaelarguedas · 2017-10-20T21:05:08Z

rclpy/src/rclpy/_rclpy.c

@@ -772,6 +799,8 @@ rclpy_create_timer(PyObject * Py_UNUSED(self), PyObject * args)

 /// Returns the period of the timer in nanoseconds
 /**
+ * Raise ValueError if capsule is not a timer capsule


typo: Raises for consistency with other docblocks added in this PR
Can you also move the Raise RuntimeError on rcl error here? (same for the other functions)

Fixed 451a072

Naming capsules prevents calling the wrong kind of cleanup on a pointer

Rclpy named capsules fixups

dirk-thomas · 2017-10-25T16:21:05Z

rclpy/rclpy/node.py

+        self.services = []
+        self.clients = []
+        self.publishers = []
+        self.guards = []


Nitpick: maybe use the same order as above for the loops? That allows easier comparison that the same variables are used in both.

pub, sub, cli, srv, tmr, gc 1561483

dirk-thomas · 2017-10-25T16:23:11Z

rclpy/src/rclpy/_rclpy.c

+  rcl_guard_condition_t * gc = (rcl_guard_condition_t *)PyCapsule_GetPointer(
+    pygc, "rcl_guard_condition_t");
+  if (!gc) {
+    return NULL;


What is the expectation here? It should only return NULL when an error is set, right? But PyCapsule_GetPointer shouldn't set an error. Should a specific error message be set or should this return Py_RETURN_NONE instead (which would be a silent failure, probably not?)?

Same below for various other pointers.

PyCapsule_GetPointer will raise an exception if the Capsule is not a valid capsule with a matching name. I think the assumption here is that the only reason for gc to be NULL is if PyCapsule_GetPointer fails and thus an exception is already being set so we need to return NULL.

You are right, it does set an error when returning NULL. Sorry for the noise - should have checked the docs before.

dirk-thomas

LGTM

mikaelarguedas · 2017-10-25T17:31:08Z

I found a few bugs in this PR, please hold before merging

mikaelarguedas

lgtm, it would be great to rerun CI to make sure we didnt break anything along the way

sloretz · 2017-10-25T18:17:04Z

@mikaelarguedas CI in progress

edit: started again with d4048f6

Linux
Linux-aarch64
macOS
Windows

sloretz · 2017-10-25T19:52:24Z

CI green. squash/merging.

dirk-thomas · 2017-10-31T16:10:04Z

rclpy/src/rclpy/_rclpy.c

    }
-    PyErr_Format(PyExc_RuntimeError,
-      "Failed to create service: %s", rcl_get_error_string_safe());
+    Py_DECREF(service);


This line seems to be the reason for this new test failure: http://ci.ros2.org/view/nightly/job/nightly_win_deb/lastCompletedBuild/testReport/(root)/projectroot/rclpytests/

sloretz added the in review Waiting for review (Kanban column) label Oct 18, 2017

sloretz self-assigned this Oct 18, 2017

dirk-thomas reviewed Oct 18, 2017

View reviewed changes

sloretz mentioned this pull request Oct 19, 2017

Rclpy wait for service #127

Closed

mikaelarguedas reviewed Oct 20, 2017

View reviewed changes

sloretz added 2 commits October 24, 2017 08:18

Use PyCapsule names

d3cdbaf

Naming capsules prevents calling the wrong kind of cleanup on a pointer

rclpy_destroy_entity takes 1 arg

0840184

sloretz force-pushed the rclpy_named_capsules branch from 7841e73 to 0840184 Compare October 24, 2017 15:21

sloretz and others added 4 commits October 24, 2017 08:22

Intermediate list not necessary

8abfd9b

Documentation fixes

451a072

homogenize docblock for ValueError + few typos

25d2b03

homogenize error strings and quote strings

d12a127

mikaelarguedas mentioned this pull request Oct 25, 2017

Rclpy named capsules fixups #133

Merged

Merge pull request #133 from ros2/rclpy_named_capsules_fixups

6a19600

Rclpy named capsules fixups

dirk-thomas reviewed Oct 25, 2017

View reviewed changes

sloretz added 2 commits October 25, 2017 09:47

Ordered stuff: pub, sub, cli, srv, tmr, gc

1561483

Line length

104baeb

dirk-thomas approved these changes Oct 25, 2017

View reviewed changes

missed some capsule names

095267c

mikaelarguedas approved these changes Oct 25, 2017

View reviewed changes

basic check for service_names_and_types()

76f4359

basic checks of topic_names_and_types and get_node_names()

d4048f6

sloretz merged commit 7298020 into master Oct 25, 2017

sloretz removed the in review Waiting for review (Kanban column) label Oct 25, 2017

sloretz deleted the rclpy_named_capsules branch October 25, 2017 19:52

dhood mentioned this pull request Oct 31, 2017

Negative ref count in test_node.py on windows debug #136

Closed

dirk-thomas reviewed Oct 31, 2017

View reviewed changes

dirk-thomas mentioned this pull request Nov 17, 2017

keep consistency when destroying node raises an exception #149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PyCapsule names #129

Use PyCapsule names #129

sloretz commented Oct 18, 2017 •

edited

Loading

dirk-thomas Oct 18, 2017 •

edited

Loading

sloretz Oct 18, 2017

dirk-thomas Oct 19, 2017

mikaelarguedas Oct 19, 2017

dirk-thomas Oct 19, 2017

mikaelarguedas left a comment

mikaelarguedas Oct 20, 2017

sloretz Oct 24, 2017

mikaelarguedas Oct 20, 2017

sloretz Oct 24, 2017

mikaelarguedas Oct 20, 2017

sloretz Oct 24, 2017

dirk-thomas Oct 25, 2017

sloretz Oct 25, 2017

dirk-thomas Oct 25, 2017

mikaelarguedas Oct 25, 2017

dirk-thomas Oct 25, 2017

dirk-thomas left a comment

mikaelarguedas commented Oct 25, 2017

mikaelarguedas left a comment

sloretz commented Oct 25, 2017 •

edited

Loading

sloretz commented Oct 25, 2017

dirk-thomas Oct 31, 2017

Use PyCapsule names #129

Use PyCapsule names #129

Conversation

sloretz commented Oct 18, 2017 • edited Loading

dirk-thomas Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelarguedas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas left a comment

Choose a reason for hiding this comment

mikaelarguedas commented Oct 25, 2017

mikaelarguedas left a comment

Choose a reason for hiding this comment

sloretz commented Oct 25, 2017 • edited Loading

sloretz commented Oct 25, 2017

Choose a reason for hiding this comment

sloretz commented Oct 18, 2017 •

edited

Loading

dirk-thomas Oct 18, 2017 •

edited

Loading

sloretz commented Oct 25, 2017 •

edited

Loading