Throw CancelledException into cancelled tasks

[nadavelkabets]

PR #1377 addresses issue #1099 by filtering out cancelled tasks. The original attempt encountered issues where cancelled coroutines were not closed properly. This is resolved by throwing CancelledException into the coroutine, similar to the behavior of asyncio.

The solution is not ready yet, as the current implementation raises CancelledException when calling task.result(), which crashes the executor due to the way exceptions in tasks are currently treated:

rclpy/rclpy/rclpy/executors.py

Lines 853 to 855 in 4e8b071

	handler()
	if handler.exception() is not None:
	raise handler.exception()

Potential Solutions

1. Change Executor Behavior to Log Exceptions: Modify the executor to log exceptions instead of crashing, aligning with asyncio behavior. This approach would also resolve issue Tasks that raise an exception crash the entire executor, even when the exception is caught #1098.
2. Ignore Cancelled Tasks in task.result(): Prevent CancelledException from being set in self._exception. Users would need to call task.cancelled() to check cancellation status instead.
3. Executor Ignores CancelledException: Allow the exception to be raised but modify the executor to prevent it from crashing.
4. Introduce a Flag (is_awaited): The flag is set upon entering __await__, causing the executor to ignore or log any exception.

Would love to hear from you :)
@weber-niklas @fujitatomoya @haudren-woven @sloretz

[weber-niklas]

I am by no means an expert, but the changes made to so far seem very logical to me.

For your potential solutions to the executor problem:

1. if this would still allow the catching of the Exceptions when awaiting or calling future.result() etc., following asyncios lead is probably a good idea. Big plus would be the fix of Tasks that raise an exception crash the entire executor, even when the exception is caught #1098 which has been bothering me too :D
2. Personally, I do not like this approach as you would need to always save the task separately:

task = executor.create_task(some_coro)
try:
    await task
except SomeException:
    ....
if task.cancelled():
    ...

instead of

try:
    await executor.create_task(some_coro)
except CancelledException:
    ...
except SomeOtherException:
    ....

As I wrote this I realized you need to do so anyways if you want to cancel the task, so not as much of an issue, but I'd prefer the expanded try except over another condition check
3. Can work, but probably prefer option 1 over this since it kills two birds with one stone
4. Would this work when just working with done callbacks? The user never actively awaits the task

task =  executor.create_task(some_coro)
task.add_done_callback(done_cb)
task.cancel()

def done_cb(task):
    try:
        task.result()
    except Exception:
        ....   

Please correct me if I am wrong: As far as I understand all exceptions raised during the execution of the task are already set as the tasks exception:
https://github.com/ros2/rclpy/pull/1392/files#diff-e85e03231947306447b58d0673c912753a94400f92ea660b56a5e4ec56b7f078R268-R277

All we need to do is make sure the executor does not crash when the exceptions are raised in the first place?
If that is the case, logging them seems valid.

[nadavelkabets]

1. if this would still allow the catching of the Exceptions when awaiting or calling future.result() etc., following asyncios lead is probably a good idea. Big plus would be the fix of Tasks that raise an exception crash the entire executor, even when the exception is caught #1098 which has been bothering me too :D

Let's say we're going with the asyncio logging approach, we have to decide on the logging behavior.
In asyncio, unretrieved exceptions are logged on garbage collection. This is a big inconvenience since garbage collection in python is inconsistent and is usually done on program shutdown. It would be very annoying if your program would not work as expected, only to find out you've had an exception on shutdown.
Also, regular callable functions are treated differently in asyncio. If I recall correctly an exception in a regular callable crashes the asyncio event loop. Here in ROS tasks the exceptions must be handled the same way for coroutines and callables...

As I see it, the two possible ways to retrieve exceptions from tasks is either awaiting the task or fetching the exception in a done_callback.
I updated my code with the following behaviors:

1. _wait_for_ready_callbacks now iterates directly on self._tasks - since the executor is now handling tasks FIFO, scheduled callbacks will have the opportunity to retrieve the exception before filtering the task from the list
2. When filtering the tasks, check if there is an unretrieved exception. If there is one and the task is not awaited, raise the exception.

What do you think? Should we stick with the asyncio approach?
Also I need to think how this may effect the multi threaded executor