Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethink exception handling #318

Open
mdickinson opened this issue May 19, 2021 · 6 comments
Open

Rethink exception handling #318

mdickinson opened this issue May 19, 2021 · 6 comments
Labels
type: enhancement New feature or request
Milestone

Comments

@mdickinson
Copy link
Member

mdickinson commented May 19, 2021

Currently, when an exception is raised by a background task, the future receives marshalled information related to that exception in its "exception" trait. The marshalled form of the exception isn't particularly standard, and isn't easy to use either.

Instead, we should consider simply providing the exception object. There's no need for the (type, value, traceback) triple: the exception object alone is enough - the traceback is already attached to the exception (since Python 3), and we can extract its type.

That would then allow a method call that either returns the result or raises the exception.

Things to worry about:

  • When doing multiprocessing, the exception may not be pickleable / unpickleable
  • By returning the exception object itself, we risk keeping references to objects used in the background task.
  • What do we call the trait that carries the exception object? "exception" is already taken. (With hindsight, it would have been better to use something different for the marshalled exception.)
@mdickinson mdickinson added the type: enhancement New feature or request label May 19, 2021
@mdickinson mdickinson added this to the Release 0.3.0 milestone May 19, 2021
@mdickinson
Copy link
Member Author

  • What do we call the trait that carries the exception object? "exception" is already taken.

"error" seems like the least worst choice. (Thanks @jwiggins for the suggestion.)

@mdickinson
Copy link
Member Author

We should also look at what concurrent.futures does, for comparison. The behaviour there differs depending on whether we're doing multiprocessing or multithreading. In the multiprocessing case, we get a _RemoteTraceback; in the multithreading case, we keep the original traceback:

>>> from concurrent.futures import *
>>> def h(): 1/0
... 
>>> def g(): h()
... 
>>> def f(): g()
... 
>>> with ThreadPoolExecutor() as executor:
...     executor.submit(f).result()
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/concurrent/futures/_base.py", line 438, in result
    return self.__get_result()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/concurrent/futures/_base.py", line 390, in __get_result
    raise self._exception
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/concurrent/futures/thread.py", line 52, in run
    result = self.fn(*self.args, **self.kwargs)
  File "<stdin>", line 1, in f
  File "<stdin>", line 1, in g
  File "<stdin>", line 1, in h
ZeroDivisionError: division by zero

@mdickinson
Copy link
Member Author

Removing the 0.3.0 milestone; there's no particular urgency on this.

@mdsmarte
Copy link

Was about to open a similar issue, so just want to give this a +1... having the exception object is convenient for passing through to logger.exception, which includes the traceback in the log message.

@mdickinson mdickinson added this to the Release 0.4.0 milestone Jul 21, 2021
@mdickinson
Copy link
Member Author

Thanks @mdsmarte; the logger.exception use-case is an important one. We're not going to get this into the upcoming release, but I can tentatively schedule it for the next one.

Just to be clear, for now I'd recommend that background tasks don't deliberately make use of exceptions for communication with the future; "expected" failures should be communicated in the return value, instead (which may mean adding a big try / except around the function being called, and converting the return value from something simple into a tuple of the form (success, details)). The exception trait is mainly there for catching unexpected exceptions. But of course, those unexpected exceptions can happen, and then logging them is an obvious thing to want to do.

@mdickinson
Copy link
Member Author

mdickinson commented Jul 21, 2021

Here's the ProcessPoolExecutor behaviour.

Script:

import concurrent.futures
import logging

logger = logging.getLogger(__name__)


def f(): g()
def g(): h()
def h(): 1/0

if __name__ == "__main__":
    try:
        with concurrent.futures.ProcessPoolExecutor() as executor:
            executor.submit(f).result()
    except Exception:
        logger.exception("something went wrong")

Results of executing this on my machine:

(traits-futures) mdickinson@mirzakhani traits-futures % python process_exception.py
something went wrong
concurrent.futures.process._RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/concurrent/futures/process.py", line 243, in _process_worker
    r = call_item.fn(*call_item.args, **call_item.kwargs)
  File "/Users/mdickinson/Enthought/Projects/traits-futures/process_exception.py", line 7, in f
    def f(): g()
  File "/Users/mdickinson/Enthought/Projects/traits-futures/process_exception.py", line 8, in g
    def g(): h()
  File "/Users/mdickinson/Enthought/Projects/traits-futures/process_exception.py", line 9, in h
    def h(): 1/0
ZeroDivisionError: division by zero
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/mdickinson/Enthought/Projects/traits-futures/process_exception.py", line 14, in <module>
    executor.submit(f).result()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/concurrent/futures/_base.py", line 445, in result
    return self.__get_result()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/concurrent/futures/_base.py", line 390, in __get_result
    raise self._exception
ZeroDivisionError: division by zero

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants