Fix issue with iterating through incomplete Tasks

Never bool-test a py::object::attr() return value without an explicit py::cast<bool>. Signed-off-by: Brad Martin <[email protected]>
ros2 · Jan 21, 2025 · 7cc7f5f · 7cc7f5f
1 parent 4a181d8
commit 7cc7f5f
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 23 deletions.
diff --git a/rclpy/src/rclpy/events_executor/events_executor.cpp b/rclpy/src/rclpy/events_executor/events_executor.cpp
@@ -827,7 +827,7 @@ void EventsExecutor::IterateTask(py::handle task)
   py::gil_scoped_acquire gil_acquire;
   // Calling this won't throw, but it may set the exception property on the task object.
   task();
-  if (task.attr("done")()) {
+  if (py::cast<bool>(task.attr("done")())) {
     py::object ex = task.attr("exception")();
     // Drop reference with GIL held.  This doesn't necessarily destroy the underlying Task, since
     // the `create_task()` caller may have retained a reference to the returned value.

diff --git a/rclpy/test/test_executor.py b/rclpy/test/test_executor.py
@@ -329,34 +329,34 @@ async def coro2():
 
     def test_create_task_dependent_coroutines(self) -> None:
         self.assertIsNotNone(self.node.handle)
-        # TODO(bmartin427) EventsExecutor doesn't yet properly handle coroutines
-        executor = SingleThreadedExecutor(context=self.context)
-        executor.add_node(self.node)
+        for cls in [SingleThreadedExecutor, EventsExecutor]:
+            executor = cls(context=self.context)
+            executor.add_node(self.node)
 
-        async def coro1():
-            nonlocal future2
-            await future2
-            return 'Sentinel Result 1'
+            async def coro1():
+                nonlocal future2
+                await future2
+                return 'Sentinel Result 1'
 
-        future1 = executor.create_task(coro1)
+            future1 = executor.create_task(coro1)
 
-        async def coro2():
-            return 'Sentinel Result 2'
+            async def coro2():
+                return 'Sentinel Result 2'
 
-        future2 = executor.create_task(coro2)
+            future2 = executor.create_task(coro2)
 
-        # Coro1 is the 1st task, so it gets to await future2 in this spin
-        executor.spin_once(timeout_sec=0)
-        # Coro2 execs in this spin
-        executor.spin_once(timeout_sec=0)
-        self.assertFalse(future1.done())
-        self.assertTrue(future2.done())
-        self.assertEqual('Sentinel Result 2', future2.result())
+            # Coro1 is the 1st task, so it gets to await future2 in this spin
+            executor.spin_once(timeout_sec=0)
+            # Coro2 execs in this spin
+            executor.spin_once(timeout_sec=0)
+            self.assertFalse(future1.done())
+            self.assertTrue(future2.done())
+            self.assertEqual('Sentinel Result 2', future2.result())
 
-        # Coro1 passes the await step here (timeout change forces new generator)
-        executor.spin_once(timeout_sec=1)
-        self.assertTrue(future1.done())
-        self.assertEqual('Sentinel Result 1', future1.result())
+            # Coro1 passes the await step here (timeout change forces new generator)
+            executor.spin_once(timeout_sec=1)
+            self.assertTrue(future1.done())
+            self.assertEqual('Sentinel Result 1', future1.result())
 
     def test_create_task_during_spin(self) -> None:
         self.assertIsNotNone(self.node.handle)