From 4b860cb823fd160742ab050f481eb65e389f9a7b Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Fri, 12 Aug 2011 19:59:19 -0500 Subject: Create processing errors and raise BadMediaFail on failure to load the image --- mediagoblin/process_media/__init__.py | 8 +++++- mediagoblin/process_media/errors.py | 54 +++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 mediagoblin/process_media/errors.py (limited to 'mediagoblin/process_media') diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 8e12ca4d..00402d7e 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -21,6 +21,8 @@ from celery.task import task from mediagoblin import mg_globals as mgg from contextlib import contextmanager +from mediagoblin.process_media.errors import BadMediaFail + THUMB_SIZE = 180, 180 MEDIUM_SIZE = 640, 640 @@ -51,7 +53,11 @@ def process_media_initial(media_id): mgg.queue_store, queued_filepath, 'source') - thumb = Image.open(queued_filename) + try: + thumb = Image.open(queued_filename) + except IOError: + raise BadMediaFail() + thumb.thumbnail(THUMB_SIZE, Image.ANTIALIAS) # ensure color mode is compatible with jpg if thumb.mode != "RGB": diff --git a/mediagoblin/process_media/errors.py b/mediagoblin/process_media/errors.py new file mode 100644 index 00000000..f2ae87ff --- /dev/null +++ b/mediagoblin/process_media/errors.py @@ -0,0 +1,54 @@ +# GNU MediaGoblin -- federated, autonomous media hosting +# Copyright (C) 2011 Free Software Foundation, Inc +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . + +from mediagoblin.util import lazy_pass_to_ugettext as _ + +class BaseProcessingFail(Exception): + """ + Base exception that all other processing failure messages should + subclass from. + + You shouldn't call this itself; instead you should subclass it + and provid the exception_path and general_message applicable to + this error. + """ + general_message = u'' + + @property + def exception_path(self): + return u"%s.%s" % ( + self.__class__.__module__, self.__class__.__name__) + + def __init__(self, **metadata): + self.metadata = metadata or {} + + def generate_error_message(self): + """ + Generate an error to display to users in the panel. + + Uses this class's general_message, possibly interpolated + with any metadata in self.metadata['error_message_vars'], + if appropriate. + """ + return self.general_message % self.metadata.get('error_message_vars', {}) + + +class BadMediaFail(BaseProcessingFail): + """ + Error that should be raised when an inappropriate file was given + for the media type specified. + """ + general_message = _(u'Invalid file given for media type.') -- cgit v1.2.3 From 4a477e246d07a4c26f084db2596caf3310b78609 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 13 Aug 2011 10:59:34 -0500 Subject: Proper handling of processor failures, working as hoped! BaseProcessingFail based exceptions recorded and marked appropriately in the database. Other exceptions also caught and marked (or rather not marked) appropriately in the database as well. --- mediagoblin/process_media/__init__.py | 76 ++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 11 deletions(-) (limited to 'mediagoblin/process_media') diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 00402d7e..d6cdd747 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -15,13 +15,14 @@ # along with this program. If not, see . import Image -from mediagoblin.db.util import ObjectId -from celery.task import task -from mediagoblin import mg_globals as mgg from contextlib import contextmanager +from celery.task import task, Task +from celery import registry -from mediagoblin.process_media.errors import BadMediaFail +from mediagoblin.db.util import ObjectId +from mediagoblin import mg_globals as mgg +from mediagoblin.process_media.errors import BaseProcessingFail, BadMediaFail THUMB_SIZE = 180, 180 @@ -34,6 +35,7 @@ def create_pub_filepath(entry, filename): unicode(entry['_id']), filename]) + @contextmanager def closing(callback): try: @@ -41,12 +43,66 @@ def closing(callback): finally: pass -@task -def process_media_initial(media_id): - workbench = mgg.workbench_manager.create_workbench() - entry = mgg.database.MediaEntry.one( - {'_id': ObjectId(media_id)}) +################################ +# Media processing initial steps +################################ + +class ProcessMedia(Task): + """ + Pass this entry off for processing. + """ + def run(self, media_id): + """ + Pass the media entry off to the appropriate processing function + (for now just process_image...) + """ + entry = mgg.database.MediaEntry.one( + {'_id': ObjectId(media_id)}) + process_image(entry) + entry['state'] = u'processed' + entry.save() + + def on_failure(self, exc, task_id, args, kwargs, einfo): + """ + If the processing failed we should mark that in the database. + + Assuming that the exception raised is a subclass of BaseProcessingFail, + 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[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] + + +def process_image(entry): + """ + Code to process an image + """ + workbench = mgg.workbench_manager.create_workbench() queued_filepath = entry['queued_media_file'] queued_filename = workbench.localized_file( @@ -107,8 +163,6 @@ def process_media_initial(media_id): media_files_dict['original'] = original_filepath if medium_processed: media_files_dict['medium'] = medium_filepath - entry['state'] = u'processed' - entry.save() # clean up workbench workbench.destroy_self() -- cgit v1.2.3 From 6788b4123ef00241d6b6c80ca93d655e4307d6e3 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 13 Aug 2011 12:21:06 -0500 Subject: 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 +++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 22 deletions(-) (limited to 'mediagoblin/process_media') 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): -- cgit v1.2.3 From 2e5ea6b9b79244a0436264c5f1b606ec5328b70e Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 13 Aug 2011 12:52:22 -0500 Subject: @task decorator no longer used! Removing that import. --- mediagoblin/process_media/__init__.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'mediagoblin/process_media') diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 69e4fc45..18836919 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -17,7 +17,7 @@ import Image from contextlib import contextmanager -from celery.task import task, Task +from celery.task import Task from celery import registry from mediagoblin.db.util import ObjectId @@ -86,6 +86,18 @@ process_media = registry.tasks[ProcessMedia.name] def mark_entry_failed(entry_id, exc): + """ + Mark a media entry as having failed in its conversion. + + Uses the exception that was raised to mark more information. If the + exception is a derivative of BaseProcessingFail then we can store extra + information that can be useful for users telling them why their media failed + to process. + + Args: + - entry_id: The id of the media entry + + """ # Was this a BaseProcessingFail? In other words, was this a # type of error that we know how to handle? if isinstance(exc, BaseProcessingFail): -- cgit v1.2.3 From ff520ff53b7a17204c64beea28c9dbde13c5cf26 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sat, 13 Aug 2011 12:52:56 -0500 Subject: Converting multi-line-string-comment to a real comment. --- mediagoblin/process_media/__init__.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'mediagoblin/process_media') diff --git a/mediagoblin/process_media/__init__.py b/mediagoblin/process_media/__init__.py index 18836919..e1289a4c 100644 --- a/mediagoblin/process_media/__init__.py +++ b/mediagoblin/process_media/__init__.py @@ -147,11 +147,9 @@ def process_image(entry): with closing(thumb_file): thumb.save(thumb_file, "JPEG", quality=90) - """ - If the size of the original file exceeds the specified size of a `medium` - file, a `medium.jpg` files is created and later associated with the media - entry. - """ + # If the size of the original file exceeds the specified size of a `medium` + # file, a `medium.jpg` files is created and later associated with the media + # entry. medium = Image.open(queued_filename) medium_processed = False -- cgit v1.2.3 From e3e9b8fcc962621e39a56748a7d34793a39e6bc6 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Sun, 14 Aug 2011 07:53:24 -0500 Subject: Switch BaseProcessingFail.exception_path's separator from period to colon Also removing .generator_error_message() which doesn't make sense really... we need to get the message when we don't have an instance of the exception, and this method requires an instance. --- mediagoblin/process_media/errors.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) (limited to 'mediagoblin/process_media') diff --git a/mediagoblin/process_media/errors.py b/mediagoblin/process_media/errors.py index f2ae87ff..f8ae9ab2 100644 --- a/mediagoblin/process_media/errors.py +++ b/mediagoblin/process_media/errors.py @@ -29,22 +29,12 @@ class BaseProcessingFail(Exception): @property def exception_path(self): - return u"%s.%s" % ( + return u"%s:%s" % ( self.__class__.__module__, self.__class__.__name__) def __init__(self, **metadata): self.metadata = metadata or {} - def generate_error_message(self): - """ - Generate an error to display to users in the panel. - - Uses this class's general_message, possibly interpolated - with any metadata in self.metadata['error_message_vars'], - if appropriate. - """ - return self.general_message % self.metadata.get('error_message_vars', {}) - class BadMediaFail(BaseProcessingFail): """ -- cgit v1.2.3