Opinion: MultiThreadedExecutor should not raise exceptions if the Task's exception was retrieved

[apockill]

The current implementation of a MultiThreadedExecutor is such that if you create a task and an exception happens within it, the executor will re-raise the exception regardless of whether the use retrieved it.

What this means is that node.create_task(callable) isn't very usable at the moment, since if your callable happens to raise an exception and you catch it, it doesn't matter- the executor will raise it again and crash your node.

Here's what would be nice:

possible_failure_future = node.create_task(possible_failure_callable)

try:
    possible_failure_future.result()
except PossibleFailure:
   # reasonable exception handling logic goes here :D
   ...

At the moment, this will fail, regardless of whether you call result() or exception on the future.

Here's what I would suggest:

class MultiThreadedExecutor(Executor):
    ...

    def _spin_once_impl(
        self,
        timeout_sec: Optional[Union[float, TimeoutObject]] = None,
        wait_condition: Callable[[], bool] = lambda: False
    ) -> None:
        try:
            handler, entity, node = self.wait_for_ready_callbacks(
                timeout_sec, None, wait_condition)
        except ExternalShutdownException:
            pass
        except ShutdownException:
            pass
        except TimeoutException:
            pass
        except ConditionReachedException:
            pass
        else:
            self._executor.submit(handler)
            self._futures.append(handler)
            for future in self._futures:  # check for any exceptions
                if future.done():
                    self._futures.remove(future)
                    if not future._exception_fetched:    # <-- ADDED LINE
                        future.result()

This would allow users to create tasks, handle errors, and so on. If they don't handle the error, only then will the executor raise the exception.

I'd love to hear folks thoughts on this!

[apockill]

Hmm, there's a few race conditions with my approach as I see it right now (the future could finish quickly, and immediately re-raise the exception before a user has a chance to check it).

I'm not sure what to do here. How can one create tasks if they can't also use a Future's exception-holding capabilities?

[sloretz]

Seems like this might be a duplicate of #1098

Maybe we could copy the behavior of asyncio by adding an api to set an exception handler on the executor, where the default is to log and move on?

[sloretz]

Closing as a duplicate, we can reopen if it's not!