Simplify exception serialization to avoid issues with complex exceptions

django/tasks/backends/immediate.py

@@ -7,7 +7,7 @@ from asgiref.sync import async_to_sync
 from django.db import transaction
 from django.tasks.signals import task_enqueued, task_finished
 from django.tasks.task import ResultStatus, TaskResult
-from django.tasks.utils import exception_to_dict, get_random_id, json_normalize
+from django.tasks.utils import get_exception_traceback, get_random_id, json_normalize
 from django.utils import timezone
 
 from .base import BaseTaskBackend
@@ -46,10 +46,9 @@ class ImmediateBackend(BaseTaskBackend):
                 raise
 
             object.__setattr__(task_result, "finished_at", timezone.now())
-            try:
+
-                object.__setattr__(task_result, "_exception_data", exception_to_dict(e))
+            object.__setattr__(task_result, "_traceback", get_exception_traceback(e))
-            except Exception:
+            object.__setattr__(task_result, "_exception_class", type(e))
-                logger.exception("Task id=%s unable to save exception", task_result.id)
 
             object.__setattr__(task_result, "status", ResultStatus.FAILED)
 

django/tasks/task.py

@@ -1,7 +1,7 @@
 from dataclasses import dataclass, field, replace
 from datetime import datetime, timedelta
 from inspect import iscoroutinefunction
-from typing import Any, Callable, Dict, Optional
+from typing import Any, Callable, Dict, Optional, Type
 
 from asgiref.sync import async_to_sync, sync_to_async
 
@@ -10,7 +10,7 @@ from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
 
 from .exceptions import ResultDoesNotExist
-from .utils import exception_from_dict, get_module_path, json_normalize
+from .utils import get_module_path, json_normalize
 
 DEFAULT_TASK_BACKEND_ALIAS = "default"
 DEFAULT_QUEUE_NAME = "default"
@@ -19,7 +19,8 @@ MAX_PRIORITY = 100
 DEFAULT_PRIORITY = 0
 
 TASK_REFRESH_ATTRS = {
-    "_exception_data",
+    "_exception_class",
+    "_traceback",
     "_return_value",
     "finished_at",
     "started_at",
@@ -214,27 +215,10 @@ class TaskResult:
     backend: str
     """The name of the backend the task will run on"""
 
+    _exception_class: Optional[Type[BaseException]] = field(init=False, default=None)
+    _traceback: Optional[str] = field(init=False, default=None)
+
     _return_value: Optional[Any] = field(init=False, default=None)
-    _exception_data: Optional[Dict[str, Any]] = field(init=False, default=None)
-
-    @property
-    def exception(self):
-        return (
-            exception_from_dict(self._exception_data)
-            if self.status == ResultStatus.FAILED and self._exception_data is not None
-            else None
-        )
-
-    @property
-    def traceback(self):
-        """
-        Return the string representation of the traceback of the task if it failed
-        """
-        return (
-            self._exception_data["exc_traceback"]
-            if self.status == ResultStatus.FAILED and self._exception_data is not None
-            else None
-        )
 
     @property
     def return_value(self):
@@ -244,14 +228,32 @@ class TaskResult:
         If the task didn't succeed, an exception is raised.
         This is to distinguish against the task returning None.
         """
-        if self.status == ResultStatus.FAILED:
+        if not self.is_finished:
-            raise ValueError("Task failed")
-
-        elif self.status != ResultStatus.SUCCEEDED:
             raise ValueError("Task has not finished yet")
 
         return self._return_value
 
+    @property
+    def exception_class(self):
+        """The exception raised by the task function"""
+        if not self.is_finished:
+            raise ValueError("Task has not finished yet")
+
+        return self._exception_class
+
+    @property
+    def traceback(self):
+        """The traceback of the exception if the task failed"""
+        if not self.is_finished:
+            raise ValueError("Task has not finished yet")
+
+        return self._traceback
+
+    @property
+    def is_finished(self):
+        """Has the task finished?"""
+        return self.status in {ResultStatus.FAILED, ResultStatus.SUCCEEDED}
+
     def refresh(self):
         """
         Reload the cached task data from the task store

django/tasks/utils.py

@@ -6,7 +6,6 @@ from functools import wraps
 from traceback import format_exception
 
 from django.utils.crypto import RANDOM_STRING_CHARS
-from django.utils.module_loading import import_string
 
 
 def is_module_level_function(func):
@@ -52,27 +51,14 @@ def retry(*, retries=3, backoff_delay=0.1):
     return wrapper
 
 
+def get_exception_traceback(exc):
+    return "".join(format_exception(type(exc), exc, exc.__traceback__))
+
+
 def get_module_path(val):
     return f"{val.__module__}.{val.__qualname__}"
 
 
-def exception_to_dict(exc):
-    return {
-        "exc_type": get_module_path(type(exc)),
-        "exc_args": json_normalize(exc.args),
-        "exc_traceback": "".join(format_exception(type(exc), exc, exc.__traceback__)),
-    }
-
-
-def exception_from_dict(exc_data):
-    exc_class = import_string(exc_data["exc_type"])
-
-    if not inspect.isclass(exc_class) or not issubclass(exc_class, BaseException):
-        raise TypeError(f"{type(exc_class)} is not an exception")
-
-    return exc_class(*exc_data["exc_args"])
-
-
 def get_random_id():
     """
     Return a random string for use as a task id.

docs/ref/tasks.txt

@@ -183,15 +183,20 @@ Attributes of ``TaskResult`` cannot be modified.
 
     The backend the result is from.
 
-.. attribute:: TaskResult.exception
+.. attribute:: TaskResult.exception_class
 
-    The exception raised when executing the task, or ``None`` if no exception
+    The exception class raised when executing the task.
-    was raised.
+
+    If the task has not finished, ``ValueError`` is raised. If the task finished
+    successfully, the exception class is ``None``.
 
 .. attribute:: TaskResult.traceback
 
     The exception traceback from the raised exception when the task failed.
 
+    If the task has not finished, ``ValueError`` is raised. If the task finished
+    successfully, the traceback is ``None``.
+
 .. attribute:: TaskResult.return_value
 
     The return value from the task function.
@@ -206,6 +211,10 @@ Attributes of ``TaskResult`` cannot be modified.
 
     The ``async`` variant of :meth:`TaskResult.refresh`.
 
+.. attribute:: TaskResult.is_finished
+
+    Whether the task has finished (successfully or not).
+
 Backends
 ========
 

docs/topics/tasks.txt

@@ -323,15 +323,13 @@ Exceptions
 ----------
 
 If the task doesn't succeed, and instead raises an exception, either
-as part of the task or as part of running it, the exception instance is saved
+as part of the task or as part of running it, the exception class is saved
-to the :attr:`django.tasks.TaskResult.exception` attribute::
+to the :attr:`django.tasks.TaskResult.exception_class` attribute::
 
-    assert isinstance(result.exception, ValueError)
+    assert result.exception_class == ValueError
 
-As part of the serialization process for exceptions, some information is lost.
+Note that this is just the type of exception, and contains no other values.
 The traceback information is reduced to a string which you can use to help
 debugging::
 
     print(result.traceback)
-
-If the exception could not be serialized, ``exception`` is ``None``.

tests/tasks/test_dummy_backend.py

@@ -32,6 +32,7 @@ class DummyBackendTestCase(SimpleTestCase):
                 result = cast(Task, task).enqueue(1, two=3)
 
                 self.assertEqual(result.status, ResultStatus.NEW)
+                self.assertFalse(result.is_finished)
                 self.assertIsNone(result.started_at)
                 self.assertIsNone(result.finished_at)
                 with self.assertRaisesMessage(ValueError, "Task has not finished yet"):
@@ -48,6 +49,7 @@ class DummyBackendTestCase(SimpleTestCase):
                 result = await cast(Task, task).aenqueue()
 
                 self.assertEqual(result.status, ResultStatus.NEW)
+                self.assertFalse(result.is_finished)
                 self.assertIsNone(result.started_at)
                 self.assertIsNone(result.finished_at)
                 with self.assertRaisesMessage(ValueError, "Task has not finished yet"):
@@ -118,6 +120,15 @@ class DummyBackendTestCase(SimpleTestCase):
         self.assertIn("enqueued", captured_logs.output[0])
         self.assertIn(result.id, captured_logs.output[0])
 
+    def test_exceptions(self):
+        result = test_tasks.noop_task.enqueue()
+
+        with self.assertRaisesMessage(ValueError, "Task has not finished yet"):
+            result.exception_class
+
+        with self.assertRaisesMessage(ValueError, "Task has not finished yet"):
+            result.traceback
+
 
 class DummyBackendTransactionTestCase(TransactionTestCase):
     available_apps = []

tests/tasks/test_immediate_backend.py

@@ -30,6 +30,7 @@ class ImmediateBackendTestCase(SimpleTestCase):
                 result = cast(Task, task).enqueue(1, two=3)
 
                 self.assertEqual(result.status, ResultStatus.SUCCEEDED)
+                self.assertTrue(result.is_finished)
                 self.assertIsNotNone(result.started_at)
                 self.assertIsNotNone(result.finished_at)
                 self.assertGreaterEqual(result.started_at, result.enqueued_at)
@@ -45,6 +46,7 @@ class ImmediateBackendTestCase(SimpleTestCase):
                 result = await cast(Task, task).aenqueue()
 
                 self.assertEqual(result.status, ResultStatus.SUCCEEDED)
+                self.assertTrue(result.is_finished)
                 self.assertIsNotNone(result.started_at)
                 self.assertIsNotNone(result.finished_at)
                 self.assertGreaterEqual(result.started_at, result.enqueued_at)
@@ -80,11 +82,12 @@ class ImmediateBackendTestCase(SimpleTestCase):
 
                 # assert result
                 self.assertEqual(result.status, ResultStatus.FAILED)
+                self.assertTrue(result.is_finished)
                 self.assertIsNotNone(result.started_at)
                 self.assertIsNotNone(result.finished_at)
                 self.assertGreaterEqual(result.started_at, result.enqueued_at)
                 self.assertGreaterEqual(result.finished_at, result.started_at)
-                self.assertIsInstance(result.exception, exception)
+                self.assertEqual(result.exception_class, exception)
                 self.assertTrue(
                     result.traceback
                     and result.traceback.endswith(f"{exception.__name__}: {message}\n")
@@ -114,7 +117,8 @@ class ImmediateBackendTestCase(SimpleTestCase):
         self.assertGreaterEqual(result.finished_at, result.started_at)
 
         self.assertIsNone(result._return_value)
-        self.assertIsNone(result.traceback)
+        self.assertEqual(result.exception_class, ValueError)
+        self.assertIn('ValueError(ValueError("This task failed"))', result.traceback)
 
         self.assertEqual(result.task, test_tasks.complex_exception)
         self.assertEqual(result.args, [])

tests/tasks/test_utils.py

@@ -1,13 +1,8 @@
 import datetime
-import hashlib
-import optparse
 import subprocess
-from typing import List
 from unittest.mock import Mock
 
-from django.core.exceptions import ImproperlyConfigured
 from django.tasks import utils
-from django.tasks.exceptions import InvalidTaskError
 from django.test import SimpleTestCase
 
 from . import tasks as test_tasks
@@ -79,87 +74,27 @@ class RetryTestCase(SimpleTestCase):
         self.assertFalse(utils.retry()(lambda: False)())
 
 
-class ExceptionSerializationTestCase(SimpleTestCase):
+class ExceptionTracebackTestCase(SimpleTestCase):
-    def test_serialize_exceptions(self):
+    def test_literal_exception(self):
-        for exc in [
+        self.assertEqual(
-            ValueError(10),
+            utils.get_exception_traceback(ValueError("Failure")),
-            SyntaxError("Wrong"),
+            "ValueError: Failure\n",
-            ImproperlyConfigured("It's wrong"),
+        )
-            InvalidTaskError(""),
-            SystemExit(),
-        ]:
-            with self.subTest(exc):
-                data = utils.exception_to_dict(exc)
-                self.assertEqual(utils.json_normalize(data), data)
-                self.assertEqual(
-                    set(data.keys()), {"exc_type", "exc_args", "exc_traceback"}
-                )
-                exception = utils.exception_from_dict(data)
-                self.assertIsInstance(exception, type(exc))
-                self.assertEqual(exception.args, exc.args)
 
-                # Check that the exception traceback contains a minimal traceback
+    def test_exception(self):
-                msg = str(exc.args[0]) if exc.args else ""
-                traceback = data["exc_traceback"]
-                self.assertIn(exc.__class__.__name__, traceback)
-                self.assertIn(msg, traceback)
-
-    def test_serialize_full_traceback(self):
         try:
-            # Using optparse to generate an error because:
+            1 / 0
-            # - it's pure python
+        except ZeroDivisionError as e:
-            # - it's easy to trip down
+            traceback = utils.get_exception_traceback(e)
-            # - it's unlikely to change ever
+            self.assertIn("ZeroDivisionError: division by zero", traceback)
-            optparse.OptionParser(option_list=[1])  # type: ignore
+        else:
-        except Exception as e:
+            self.fail("ZeroDivisionError not raised")
-            traceback = utils.exception_to_dict(e)["exc_traceback"]
-            # The test is willingly fuzzy to ward against changes in the
-            # traceback formatting
-            self.assertIn("traceback", traceback.lower())
-            self.assertIn("line", traceback.lower())
-            self.assertIn(optparse.__file__, traceback)
-            self.assertTrue(
-                traceback.endswith("TypeError: not an Option instance: 1\n")
-            )
 
-    def test_serialize_traceback_from_c_module(self):
+    def test_complex_exception(self) -> None:
         try:
-            # Same as test_serialize_full_traceback, but uses hashlib
+            {}[datetime.datetime.now()]
-            # because it's in C, not in Python
+        except KeyError as e:
-            hashlib.md5(1)  # type: ignore
+            traceback = utils.get_exception_traceback(e)
-        except Exception as e:
+            self.assertIn("KeyError: datetime.datetime", traceback)
-            traceback = utils.exception_to_dict(e)["exc_traceback"]
+        else:
-            self.assertIn("traceback", traceback.lower())
+            self.fail("KeyError not raised")
-            self.assertTrue(
-                traceback.endswith(
-                    "TypeError: object supporting the buffer API required\n"
-                )
-            )
-            self.assertIn("hashlib.md5(1)", traceback)
-
-    def test_cannot_deserialize_non_exception(self):
-        serialized_exceptions: List[utils.SerializedExceptionDict] = [
-            {
-                "exc_type": "subprocess.check_output",
-                "exc_args": ["exit", "1"],
-                "exc_traceback": "",
-            },
-            {"exc_type": "True", "exc_args": [], "exc_traceback": ""},
-            {"exc_type": "math.pi", "exc_args": [], "exc_traceback": ""},
-            {"exc_type": __name__, "exc_args": [], "exc_traceback": ""},
-            {
-                "exc_type": utils.get_module_path(type(self)),
-                "exc_args": [],
-                "exc_traceback": "",
-            },
-            {
-                "exc_type": utils.get_module_path(Mock),
-                "exc_args": [],
-                "exc_traceback": "",
-            },
-        ]
-
-        for data in serialized_exceptions:
-            with self.subTest(data):
-                with self.assertRaises((TypeError, ImportError)):
-                    utils.exception_from_dict(data)