Handle raising exceptions from PublicTask (#4078)

* Handle raising exceptions from PublicTask

When the task raises an exception there is no way to have a custom
result under ``AsyncResult.info``, it will be always the Exception
that was risen from inside the task.

Because of that, when the task raises an Exception we are handling it
inside the ``run`` method and we add the exception's message into our
custom result *without marking* the task as FAILURE.

* Test case for a PublicTask that raises an Exception

* Test case for method used to check a job status

Author: humitos

readthedocs/core/utils/tasks/public.py

@@ -67,20 +67,28 @@ def set_public_data(self, data):
         self.update_progress_data()
 
     def run(self, *args, **kwargs):
+        error = False
+        exception_raised = None
         self.set_permission_context(kwargs)
-        result = self.run_public(*args, **kwargs)
-        if result is not None:
-            self.set_public_data(result)
-        _, info = self.get_task_data()
-        return info
+        try:
+            result = self.run_public(*args, **kwargs)
+        except Exception as e:
+            # With Celery 4 we lost the ability to keep our data dictionary into
+            # ``AsyncResult.info`` when an exception was raised inside the
+            # Task. In this case, ``info`` will contain the exception raised
+            # instead of our data. So, I'm keeping the task as ``SUCCESS`` but
+            # the adding the exception message into an ``error`` key to be used
+            # from outside
+            exception_raised = e
+            error = True
 
-    def after_return(self, status, retval, task_id, args, kwargs, einfo):
-        """Add the error to the task data"""
         _, info = self.get_task_data()
-        if status == states.FAILURE:
-            info['error'] = retval
-        if STATUS_UPDATES_ENABLED:
-            self.update_state(state=status, meta=info)
+        if error and exception_raised:
+            info['error'] = str(exception_raised)
+        elif result is not None:
+            self.set_public_data(result)
+
+        return info
 
 
 def permission_check(check):

readthedocs/restapi/views/task_views.py

@@ -29,10 +29,12 @@ def get_status_data(task_name, state, data, error=None):
         'data': data,
         'started': state in STARTED_STATES,
         'finished': state in FINISHED_STATES,
-        'success': state in SUCCESS_STATES,
+        # When an exception is raised inside the task, we keep this as SUCCESS
+        # and add the exception messsage into the 'error' key
+        'success': state in SUCCESS_STATES and error is None,
     }
-    if error is not None and isinstance(error, Exception):
-        data['error'] = error.message
+    if error is not None:
+        data['error'] = error
     return data
 
 

readthedocs/rtd_tests/tests/test_api.py

@@ -21,6 +21,7 @@
 from readthedocs.integrations.models import Integration
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
 from readthedocs.projects.models import Feature, Project
+from readthedocs.restapi.views.task_views import get_status_data
 
 super_auth = base64.b64encode(b'super:test').decode('utf-8')
 eric_auth = base64.b64encode(b'eric:test').decode('utf-8')
@@ -895,3 +896,22 @@ def test_get_version_by_id(self):
             resp.data,
             version_data,
         )
+
+
+class TaskViewsTests(TestCase):
+
+    def test_get_status_data(self):
+        data = get_status_data(
+            'public_task_exception',
+            'SUCCESS',
+            {'data': 'public'},
+            'Something bad happened',
+        )
+        self.assertEqual(data, {
+            'name': 'public_task_exception',
+            'data': {'data': 'public'},
+            'started': True,
+            'finished': True,
+            'success': False,
+            'error': 'Something bad happened',
+        })

readthedocs/rtd_tests/tests/test_celery.py

@@ -123,3 +123,33 @@ def test_sync_repository(self):
                 args=(version.pk,),
             )
         self.assertTrue(result.successful())
+
+    def test_public_task_exception(self):
+        """
+        Test when a PublicTask rises an Exception.
+
+        The exception should be catched and added to the ``info`` attribute of
+        the result. Besides, the task should be SUCCESS.
+        """
+        from readthedocs.core.utils.tasks import PublicTask
+        from readthedocs.worker import app
+
+        class PublicTaskException(PublicTask):
+            name = 'public_task_exception'
+
+            def run_public(self):
+                raise Exception('Something bad happened')
+
+        app.tasks.register(PublicTaskException)
+        exception_task = PublicTaskException()
+        result = exception_task.delay()
+
+        # although the task risen an exception, it's success since we add the
+        # exception into the ``info`` attributes
+        self.assertEqual(result.status, 'SUCCESS')
+        self.assertEqual(result.info, {
+            'task_name': 'public_task_exception',
+            'context': {},
+            'public_data': {},
+            'error': 'Something bad happened',
+        })