Fix exception handling #199
Conversation
dirk-thomas
added
bug
rclpy/rclpy/task.py
Outdated
| if not self._done: | ||
| raise Error | ||
| if self._exception is not None: | ||
| raise self._exception |
There was a problem hiding this comment.
This is following the logic of the asyncio future.
rclpy/rclpy/client.py
Outdated
| @@ -55,8 +55,6 @@ def unblock(future): | |||
| future.add_done_callback(unblock) | |||
|
|
|||
| event.wait() | |||
| if future.exception() is not None: | |||
| raise future.exception() | |||
There was a problem hiding this comment.
With the change to result() this is not necessary anymore since it happens implicitly.
rclpy/rclpy/executors.py
Outdated
| self._executor.submit(handler) | ||
| future = self._executor.submit(handler) | ||
| # currently the future is been waited on? | ||
| # the result needs to be checked in order to re-raise potential exceptions |
There was a problem hiding this comment.
What should happen here?
dirk-thomas
force-pushed
the
fix_exception_handling
branch
3 times, most recently
from
55169ce
to
ab1ea62
Compare
dirk-thomas
force-pushed
the
fix_exception_handling
branch
from
ab1ea62
to
914b81a
Compare
dirk-thomas
added
in review
dirk-thomas
force-pushed
the
fix_exception_handling
branch
from
04c5c8b
to
8b9fe32
Compare
|
We can't check for This should at least pass our tests now. Back in review. |
dirk-thomas
added
in review
|
rclpy/rclpy/task.py
Outdated
| @@ -42,6 +44,12 @@ def __init__(self, *, executor=None): | |||
| self._executor = None | |||
| self._set_executor(executor) | |||
|
|
|||
| def __del__(self): | |||
| if self._exception is not None: | |||
There was a problem hiding this comment.
This will print even for exceptions that were retrieved, is that intended?
There was a problem hiding this comment.
That is correct. Unfortunately I don't see a way to avoid that since there is no API calling code invoked to "take" an exception.
There was a problem hiding this comment.
I added a boolean flag. When exception() is being called it won't be reported assuming the caller did "the right thing" already.
rclpy/rclpy/task.py
Outdated
| raise CancelledError | ||
| # a coroutine might not have finished after calling spin_once | ||
| # if not self._done: | ||
| # raise Error |
There was a problem hiding this comment.
Is this the right behavior? I believe that in the other Future's Python provides, calling result before being set is an error (you should check if it is done first).
If calling result prematurely is an issue, and we only care about whether or not there was an exception, why not just do something like this in the executors:
if future.exception() is not None:
raise future.exception()There was a problem hiding this comment.
Is this the right behavior?
This is the current behavior of our coroutines / spinning them. The commented code is what Python's future class does but we can't enforce that at the moment.
why not just do something like this in the executors
We can do that instead. We don't even have to change the result() method at all for now.
There was a problem hiding this comment.
I removed all the checking from result() for now. We should revisit this though after the release (see follow up ticket #200).
|
rclpy/rclpy/task.py
Outdated
| @@ -42,6 +44,12 @@ def __init__(self, *, executor=None): | |||
| self._executor = None | |||
| self._set_executor(executor) | |||
|
|
|||
| def __del__(self): | |||
| if self._exception is not None and self._exception_fetched: | |||
There was a problem hiding this comment.
and not self._exception_fetched?
There was a problem hiding this comment.
Very good point. I changed the logic to match the naming of the
variable: c63e3be
Fixes #198.
Looking for feedback on this patch.