Repository: enthought/traits-futures (language: python)

Issue:
Fix marshal_exception not to use global state
The `marshal_exception` function used `formatted_traceback = str(traceback.format_exc())` to format the traceback, which implicitly picked up the exception details from `sys.exc_info()`. This meant that it only worked correctly from within an "except" block (which is the only way that we're currently using it). This indirection may have been necessary in Python 2, but for Python 3 we can access the traceback directly from the exception object.

This PR fixes `marshal_exception` to use the actual exception traceback.

While we're here, we also improve the `marshal_exception` docstring and tidy up the implementation.
Consider using `type.__name__` instead of `str(type)` in `marshal_exception()`
Consider using `type.__name__` instead of `str(type)` in `marshal_exception()` for better use of that string.

In my opinion, `'ZeroDivisionError'` is more useful than `"<class 'ZeroDivisionError'>"`.

https://github.com/enthought/traits-futures/blob/main/traits_futures/exception_handling.py#L22

https://github.com/enthought/traits-futures/blob/839a775285ee0b28be81672734b0f196bfb596c5/traits_futures/exception_handling.py#L17-L25
Rethink exception handling
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.)



Discussion / hints:

Thanks! I think we can squeeze a fix for this into the coming release.

See also #318, which isn't scheduled for this release.

@peterzahemszky : What do you think about using something a bit more qualified than just `__name__`, for example something that includes the module information? E.g., for a `ZeroDivisionError` we'd still get just `ZeroDivisionError`, but for something like `TraitError` we'd get `traits.trait_errors.TraitError`?


So we probably want something that uses both `__module__` and `__qualname__`, except in the case where the module is builtins. This is close to what CPython does to construct the repr of a class: https://github.com/python/cpython/blob/2f180ce2cb6e6a7e3c517495e0f4873d6aaf5f2f/Objects/typeobject.c#L1076-L1092
> * 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.)
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
```

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

Fix patch:
diff --git a/traits_futures/exception_handling.py b/traits_futures/exception_handling.py
index c3829784..1165b385 100644
--- a/traits_futures/exception_handling.py
+++ b/traits_futures/exception_handling.py
@@ -14,6 +14,34 @@
 import traceback
 
 
+def _qualified_type_name(class_):
+    """
+    Compute a descriptive string representing a class, including
+    a module name where relevant.
+
+    Example outputs are "RuntimeError" for the built-in RuntimeError
+    exception, or "struct.error" for the struct module exception class.
+
+    Parameters
+    ----------
+    class_ : type
+
+    Returns
+    -------
+    class_name : str
+    """
+    # We're being extra conservative here and allowing for the possibility that
+    # the class doesn't have __module__ and/or __qualname__ attributes. This
+    # function is called during exception handling, so we want to minimise the
+    # possibility that it raises a new exception.
+    class_module = getattr(class_, "__module__", "<unknown>")
+    class_qualname = getattr(class_, "__qualname__", "<unknown>")
+    if class_module == "builtins":
+        return f"{class_qualname}"
+    else:
+        return f"{class_module}.{class_qualname}"
+
+
 def marshal_exception(exception):
     """
     Turn exception details into something that can be safely
@@ -31,7 +59,7 @@ def marshal_exception(exception):
         formatted traceback.
     """
     return (
-        str(type(exception)),
+        _qualified_type_name(type(exception)),
         str(exception),
         "".join(
             traceback.format_exception(


Test patch:
diff --git a/traits_futures/tests/background_call_tests.py b/traits_futures/tests/background_call_tests.py
index 62661ed2..d79c0e39 100644
--- a/traits_futures/tests/background_call_tests.py
+++ b/traits_futures/tests/background_call_tests.py
@@ -292,7 +292,7 @@ def assertNoResult(self, future):
             future.result
 
     def assertException(self, future, exc_type):
-        self.assertEqual(future.exception[0], str(exc_type))
+        self.assertIn(exc_type.__name__, future.exception[0])
 
     def assertNoException(self, future):
         with self.assertRaises(AttributeError):
diff --git a/traits_futures/tests/background_iteration_tests.py b/traits_futures/tests/background_iteration_tests.py
index 754740f3..bc78e8e2 100644
--- a/traits_futures/tests/background_iteration_tests.py
+++ b/traits_futures/tests/background_iteration_tests.py
@@ -448,7 +448,7 @@ def assertNoResult(self, future):
             future.result
 
     def assertException(self, future, exc_type):
-        self.assertEqual(future.exception[0], str(exc_type))
+        self.assertIn(exc_type.__name__, future.exception[0])
 
     def assertNoException(self, future):
         with self.assertRaises(AttributeError):
diff --git a/traits_futures/tests/background_progress_tests.py b/traits_futures/tests/background_progress_tests.py
index 9f8c5a64..9d11bd69 100644
--- a/traits_futures/tests/background_progress_tests.py
+++ b/traits_futures/tests/background_progress_tests.py
@@ -332,4 +332,4 @@ def assertNoException(self, future):
             future.exception
 
     def assertException(self, future, exc_type):
-        self.assertEqual(future.exception[0], str(exc_type))
+        self.assertIn(exc_type.__name__, future.exception[0])
diff --git a/traits_futures/tests/test_exception_handling.py b/traits_futures/tests/test_exception_handling.py
index 6de33ed7..894e4c12 100644
--- a/traits_futures/tests/test_exception_handling.py
+++ b/traits_futures/tests/test_exception_handling.py
@@ -13,6 +13,10 @@
 from traits_futures.exception_handling import marshal_exception
 
 
+class CustomException(Exception):
+    """Custom exception for testing purposes."""
+
+
 class TestExceptionHandling(unittest.TestCase):
     def test_marshal_exception(self):
         try:
@@ -25,7 +29,7 @@ def test_marshal_exception(self):
         self.assertIsInstance(exc_value, str)
         self.assertIsInstance(exc_traceback, str)
 
-        self.assertEqual(exc_type, str(RuntimeError))
+        self.assertEqual(exc_type, "RuntimeError")
         self.assertIn("something went wrong", exc_value)
         self.assertIn("test_marshal_exception", exc_traceback)
 
@@ -41,7 +45,7 @@ def test_marshal_exception_with_unicode_message(self):
         self.assertIsInstance(exc_value, str)
         self.assertIsInstance(exc_traceback, str)
 
-        self.assertEqual(exc_type, str(ValueError))
+        self.assertEqual(exc_type, "ValueError")
         self.assertIn(message, exc_value)
         self.assertIn("test_marshal_exception", exc_traceback)
 
@@ -59,6 +63,41 @@ def test_marshal_exception_works_outside_except(self):
         self.assertIsInstance(exc_value, str)
         self.assertIsInstance(exc_traceback, str)
 
-        self.assertEqual(exc_type, str(RuntimeError))
+        self.assertEqual(exc_type, "RuntimeError")
         self.assertIn("something went wrong", exc_value)
         self.assertIn("test_marshal_exception", exc_traceback)
+
+    def test_marshal_exception_non_builtin(self):
+        message = "printer on fire"
+        try:
+            raise CustomException(message)
+        except BaseException as exception:
+            marshalled = marshal_exception(exception)
+
+        exc_type, exc_value, exc_traceback = marshalled
+        self.assertIsInstance(exc_type, str)
+        self.assertIsInstance(exc_value, str)
+        self.assertIsInstance(exc_traceback, str)
+
+        self.assertEqual(
+            exc_type,
+            f"{__name__}.CustomException",
+        )
+        self.assertIn(message, exc_value)
+        self.assertIn("test_marshal_exception", exc_traceback)
+
+    def test_marshal_exception_nested_exception(self):
+        class NestedException(Exception):
+            pass
+
+        try:
+            raise NestedException()
+        except BaseException as exception:
+            marshalled = marshal_exception(exception)
+
+        exc_type, exc_value, exc_traceback = marshalled
+        self.assertEqual(
+            exc_type,
+            f"{__name__}.TestExceptionHandling."
+            "test_marshal_exception_nested_exception.<locals>.NestedException",
+        )