From 6788b4123ef00241d6b6c80ca93d655e4307d6e3 Mon Sep 17 00:00:00 2001
From: Christopher Allan Webber <cwebber@dustycloud.org>
Date: Sat, 13 Aug 2011 12:21:06 -0500
Subject: [PATCH] Capture and properly handle errors.

Handled in several places:

 - In the run() of the ProcessMedia itself for
   handled (BaseProcessingFail derived) errors (best to do these not
   in on_failure because the errors are highlighted in celeryd in a
   way that looks inappropriate for when the errors are well handled)
 - In ProcessMedia.on_failure() for all other errors
 - In the submit view where all exceptions are caught, media is marked
   at having failed, then the error is re-raised.  (The reason for
   this is that users running in "lazy" mode will get errors
   propagated by celery and so on_failure won't run for them.)
---
 mediagoblin/process_media/__init__.py | 54 ++++++++++++++++-----------
 mediagoblin/submit/views.py           | 21 +++++++++--
 2 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py
index d6cdd747..69e4fc45 100644
--- a/mediagoblin/process_media/__init__.py
+++ b/mediagoblin/process_media/__init__.py
@@ -59,7 +59,14 @@ class ProcessMedia(Task):
         """
         entry = mgg.database.MediaEntry.one(
             {'_id': ObjectId(media_id)})
-        process_image(entry)
+
+        # Try to process, and handle expected errors.
+        try:
+            process_image(entry)
+        except BaseProcessingFail, exc:
+            mark_entry_failed(entry[u'_id'], exc)
+            return
+            
         entry['state'] = u'processed'
         entry.save()
 
@@ -71,31 +78,34 @@ class ProcessMedia(Task):
         we can use that to get more information about the failure and store that
         for conveying information to users about the failure, etc.
         """
-        media_id = args[0]
-        entry = mgg.database.MediaEntry.one(
-            {'_id': ObjectId(media_id)})
+        entry_id = args[0]
+        mark_entry_failed(entry_id, exc)
 
-        entry[u'state'] = u'failed'
-
-        # Was this a BaseProcessingFail?  In other words, was this a
-        # type of error that we know how to handle?
-        if isinstance(exc, BaseProcessingFail):
-            # Looks like yes, so record information about that failure and any
-            # metadata the user might have supplied.
-            entry[u'fail_error'] = exc.exception_path
-            entry[u'fail_metadata'] = exc.metadata
-        else:
-            # Looks like no, so just mark it as failed and don't record a
-            # failure_error (we'll assume it wasn't handled) and don't record
-            # metadata (in fact overwrite it if somehow it had previous info
-            # here)
-            entry[u'fail_error'] = None
-            entry[u'fail_metadata'] = {}
 
-        entry.save()
+process_media = registry.tasks[ProcessMedia.name]
 
 
-process_media = registry.tasks[ProcessMedia.name]
+def mark_entry_failed(entry_id, exc):
+    # Was this a BaseProcessingFail?  In other words, was this a
+    # type of error that we know how to handle?
+    if isinstance(exc, BaseProcessingFail):
+        # Looks like yes, so record information about that failure and any
+        # metadata the user might have supplied.
+        mgg.database['media_entries'].update(
+            {'_id': entry_id},
+            {'$set': {u'state': u'failed',
+                      u'fail_error': exc.exception_path,
+                      u'fail_metadata': exc.metadata}})
+    else:
+        # Looks like no, so just mark it as failed and don't record a
+        # failure_error (we'll assume it wasn't handled) and don't record
+        # metadata (in fact overwrite it if somehow it had previous info
+        # here)
+        mgg.database['media_entries'].update(
+            {'_id': entry_id},
+            {'$set': {u'state': u'failed',
+                      u'fail_error': None,
+                      u'fail_metadata': {}}})
 
 
 def process_image(entry):
diff --git a/mediagoblin/submit/views.py b/mediagoblin/submit/views.py
index 25b3664b..1ba17954 100644
--- a/mediagoblin/submit/views.py
+++ b/mediagoblin/submit/views.py
@@ -28,7 +28,7 @@ from mediagoblin.util import (
 from mediagoblin.util import pass_to_ugettext as _
 from mediagoblin.decorators import require_active_login
 from mediagoblin.submit import forms as submit_forms, security
-from mediagoblin.process_media import process_media
+from mediagoblin.process_media import process_media, mark_entry_failed
 from mediagoblin.messages import add_message, SUCCESS
 
 
@@ -102,9 +102,22 @@ def submit_start(request):
             #
             # (... don't change entry after this point to avoid race
             # conditions with changes to the document via processing code)
-            process_media.apply_async(
-                [unicode(entry['_id'])], {},
-                task_id=task_id)
+            try:
+                process_media.apply_async(
+                    [unicode(entry['_id'])], {},
+                    task_id=task_id)
+            except BaseException as exc:
+                # The purpose of this section is because when running in "lazy"
+                # or always-eager-with-exceptions-propagated celery mode that
+                # the failure handling won't happen on Celery end.  Since we
+                # expect a lot of users to run things in this way we have to
+                # capture stuff here.
+                #
+                # ... not completely the diaper pattern because the exception is
+                # re-raised :)
+                mark_entry_failed(entry[u'_id'], exc)
+                # re-raise the exception
+                raise
 
             add_message(request, SUCCESS, _('Woohoo! Submitted!'))
 
-- 
2.25.1