Fix subscription pycapsule not being destroyed (#320)

* Warn if subscription pycapsule is invalid Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Pycapsule destructor expects pycapsule Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Replace impossible conditional with assert Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Try to get pointer, clear error if it fails Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Return if error occurrs Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
ros2 · Apr 24, 2019 · 616e853 · 616e853
1 parent 5b977a6
commit 616e853
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 23 deletions.
diff --git a/rclpy/src/rclpy/_rclpy.c b/rclpy/src/rclpy/_rclpy.c
@@ -1671,16 +1671,27 @@ rclpy_time_since_last_call(PyObject * Py_UNUSED(self), PyObject * args)
 static void
 _rclpy_destroy_subscription(PyObject * pyentity)
 {
-  if (PyCapsule_IsValid(pyentity, "rclpy_subscription_t")) {
-    rclpy_subscription_t * sub = (rclpy_subscription_t *)PyCapsule_GetPointer(
-      pyentity, "rclpy_subscription_t");
-    if (!sub) {
-      return;
-    }
-    rcl_ret_t ret = rcl_subscription_fini(&(sub->subscription), sub->node);
-    (void)ret;
-    PyMem_Free(sub);
+  rclpy_subscription_t * sub = (rclpy_subscription_t *)PyCapsule_GetPointer(
+    pyentity, "rclpy_subscription_t");
+  if (!sub) {
+    // Don't want to raise an exception, who knows where it will get raised.
+    PyErr_Clear();
+    // Warning should use line number of the current stack frame
+    int stack_level = 1;
+    PyErr_WarnFormat(
+      PyExc_RuntimeWarning, stack_level, "_rclpy_destroy_subscrition failed to get pointer");
+    return;
+  }
+
+  rcl_ret_t ret = rcl_subscription_fini(&(sub->subscription), sub->node);
+  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 subscription: %s",
+      rcl_get_error_string().str);
   }
+  PyMem_Free(sub);
 }
 
 /// Create a subscription

diff --git a/rclpy/src/rclpy/_rclpy_pycapsule.c b/rclpy/src/rclpy/_rclpy_pycapsule.c
@@ -97,20 +97,7 @@ rclpy_pycapsule_destroy(PyObject * Py_UNUSED(self), PyObject * args)
     PyErr_Format(PyExc_ValueError, "PyCapsule does not have a destructor.");
   }
 
-  // Need name to get pointer
-  const char * name = PyCapsule_GetName(pycapsule);
-
-  if (PyErr_Occurred()) {
-    return NULL;
-  }
-
-  void * pointer = PyCapsule_GetPointer(pycapsule, name);
-
-  if (NULL == pointer) {
-    return NULL;
-  }
-
-  destructor(pointer);
+  destructor(pycapsule);
 
   if (0 != PyCapsule_SetDestructor(pycapsule, NULL)) {
     return NULL;