Turn error_handlers into error_trackers

* simplify logic--tracker just tracks errors.  Trackers should not raise,
      and are not be responsible for logging.
* adapted code to use trackers.
* Started cleanup of error handling code:
  - if need to add info and re-raise, just do that.  No logging.
  - if working around a problem, log and track as needed.

common/lib/xmodule/tests/test_import.py

@@ -4,7 +4,7 @@ import unittest
 from lxml import etree
 
 from xmodule.x_module import XMLParsingSystem, XModuleDescriptor
-from xmodule.errorhandlers import ignore_errors_handler
+from xmodule.errortracker import null_error_tracker
 from xmodule.modulestore import Location
 
 class ImportTestCase(unittest.TestCase):
@@ -27,7 +27,7 @@ class ImportTestCase(unittest.TestCase):
             raise Exception("Shouldn't be called")
 
         system = XMLParsingSystem(load_item, resources_fs,
-                                  ignore_errors_handler, process_xml)
+                                  null_error_tracker, process_xml)
         system.render_template = render_template
 
         return system

common/lib/xmodule/xmodule/backcompat_module.py

@@ -35,18 +35,21 @@ def process_includes(fn):
                 # insert  new XML into tree in place of include
                 parent.insert(parent.index(next_include), incxml)
             except Exception:
-                msg = "Error in problem xml include: %s" % (etree.tostring(next_include, pretty_print=True))
-                log.exception(msg)
-                parent = next_include.getparent()
+                # Log error and work around it
+                msg = "Error in problem xml include: %s" % (
+                    etree.tostring(next_include, pretty_print=True))
+
+                system.error_tracker(msg)
 
+                parent = next_include.getparent()
                 errorxml = etree.Element('error')
                 messagexml = etree.SubElement(errorxml, 'message')
                 messagexml.text = msg
                 stackxml = etree.SubElement(errorxml, 'stacktrace')
                 stackxml.text = traceback.format_exc()
-
                 # insert error XML in place of include
                 parent.insert(parent.index(next_include), errorxml)
+
             parent.remove(next_include)
 
             next_include = xml_object.find('include')

common/lib/xmodule/xmodule/error_module.py

@@ -15,7 +15,7 @@ class ErrorModule(XModule):
         TODO (vshnayder): proper style, divs, etc.
         '''
         if not self.system.is_staff:
-            return self.system.render_template('module-error.html')
+            return self.system.render_template('module-error.html', {})
 
         # staff get to see all the details
         return self.system.render_template('module-error-staff.html', {
@@ -31,22 +31,27 @@ class ErrorDescriptor(EditingDescriptor):
     module_class = ErrorModule
 
     @classmethod
-    def from_xml(cls, xml_data, system, org=None, course=None):
+    def from_xml(cls, xml_data, system, org=None, course=None, err=None):
         '''Create an instance of this descriptor from the supplied data.
 
         Does not try to parse the data--just stores it.
+
+        Takes an extra, optional, parameter--the error that caused an
+        issue.
         '''
 
         definition = {}
+        if err is not None:
+            definition['error'] = err
+
         try:
             # If this is already an error tag, don't want to re-wrap it.
             xml_obj = etree.fromstring(xml_data)
             if xml_obj.tag == 'error':
                 xml_data = xml_obj.text
         except etree.XMLSyntaxError as err:
-            # Save the error to display later
-            definition['error'] = str(err)
-
+            # Save the error to display later--overrides other problems
+            definition['error'] = err
 
         definition['data'] = xml_data
         # TODO (vshnayder): Do we need a unique slug here?  Just pick a random

common/lib/xmodule/xmodule/errorhandlers.py

-import logging
-import sys
-
-log = logging.getLogger(__name__)
-
-def in_exception_handler():
-    '''Is there an active exception?'''
-    return sys.exc_info() != (None, None, None)
-
-def strict_error_handler(msg, exc_info=None):
-    '''
-    Do not let errors pass.  If exc_info is not None, ignore msg, and just
-    re-raise.  Otherwise, check if we are in an exception-handling context.
-    If so, re-raise.  Otherwise, raise Exception(msg).
-
-    Meant for use in validation, where any errors should trap.
-    '''
-    if exc_info is not None:
-        raise exc_info[0], exc_info[1], exc_info[2]
-
-    if in_exception_handler():
-        raise
-
-    raise Exception(msg)
-
-
-def logging_error_handler(msg, exc_info=None):
-    '''Log all errors, but otherwise let them pass, relying on the caller to
-    workaround.'''
-    if exc_info is not None:
-        log.exception(msg, exc_info=exc_info)
-        return
-
-    if in_exception_handler():
-        log.exception(msg)
-        return
-
-    log.error(msg)
-
-
-def ignore_errors_handler(msg, exc_info=None):
-    '''Ignore all errors, relying on the caller to workaround.
-    Meant for use in the LMS, where an error in one part of the course
-    shouldn't bring down the whole system'''
-    pass

common/lib/xmodule/xmodule/errortracker.py

+import logging
+import sys
+
+log = logging.getLogger(__name__)
+
+def in_exception_handler():
+    '''Is there an active exception?'''
+    return sys.exc_info() != (None, None, None)
+
+
+def make_error_tracker():
+    '''Return a tuple (logger, error_list), where
+    the logger appends a tuple (message, exc_info=None)
+    to the error_list on every call.
+
+    error_list is a simple list.  If the caller messes with it, info
+    will be lost.
+    '''
+    errors = []
+
+    def error_tracker(msg):
+        '''Log errors'''
+        exc_info = None
+        if in_exception_handler():
+            exc_info = sys.exc_info()
+
+        errors.append((msg, exc_info))
+
+    return (error_tracker, errors)
+
+def null_error_tracker(msg):
+    '''A dummy error tracker that just ignores the messages'''
+    pass

common/lib/xmodule/xmodule/mako_module.py

@@ -2,10 +2,10 @@ from x_module import XModuleDescriptor, DescriptorSystem
 
 
 class MakoDescriptorSystem(DescriptorSystem):
-    def __init__(self, load_item, resources_fs, error_handler,
+    def __init__(self, load_item, resources_fs, error_tracker,
                  render_template, **kwargs):
         super(MakoDescriptorSystem, self).__init__(
-            load_item, resources_fs, error_handler, **kwargs)
+            load_item, resources_fs, error_tracker, **kwargs)
 
         self.render_template = render_template
 

common/lib/xmodule/xmodule/modulestore/mongo.py

@@ -6,7 +6,7 @@ from itertools import repeat
 from path import path
 
 from importlib import import_module
-from xmodule.errorhandlers import strict_error_handler
+from xmodule.errortracker import null_error_tracker
 from xmodule.x_module import XModuleDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
 from mitxmako.shortcuts import render_to_string
@@ -26,7 +26,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
     from, with a backup of calling to the underlying modulestore for more data
     """
     def __init__(self, modulestore, module_data, default_class, resources_fs,
-                 error_handler, render_template):
+                 error_tracker, render_template):
         """
         modulestore: the module store that can be used to retrieve additional modules
 
@@ -38,13 +38,13 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
 
         resources_fs: a filesystem, as per MakoDescriptorSystem
 
-        error_handler:
+        error_tracker:
 
         render_template: a function for rendering templates, as per
             MakoDescriptorSystem
         """
         super(CachingDescriptorSystem, self).__init__(
-                self.load_item, resources_fs, error_handler, render_template)
+                self.load_item, resources_fs, error_tracker, render_template)
         self.modulestore = modulestore
         self.module_data = module_data
         self.default_class = default_class
@@ -79,7 +79,8 @@ class MongoModuleStore(ModuleStore):
     """
 
     # TODO (cpennington): Enable non-filesystem filestores
-    def __init__(self, host, db, collection, fs_root, port=27017, default_class=None):
+    def __init__(self, host, db, collection, fs_root, port=27017, default_class=None,
+                 error_tracker=null_error_tracker):
         self.collection = pymongo.connection.Connection(
             host=host,
             port=port
@@ -90,13 +91,17 @@ class MongoModuleStore(ModuleStore):
 
         # Force mongo to maintain an index over _id.* that is in the same order
         # that is used when querying by a location
-        self.collection.ensure_index(zip(('_id.' + field for field in Location._fields), repeat(1)))
+        self.collection.ensure_index(
+            zip(('_id.' + field for field in Location._fields), repeat(1)))
 
-        # TODO (vshnayder): default arg default_class=None will make this error
-        module_path, _, class_name = default_class.rpartition('.')
-        class_ = getattr(import_module(module_path), class_name)
-        self.default_class = class_
+        if default_class is not None:
+            module_path, _, class_name = default_class.rpartition('.')
+            class_ = getattr(import_module(module_path), class_name)
+            self.default_class = class_
+        else:
+            self.default_class = None
         self.fs_root = path(fs_root)
+        self.error_tracker = error_tracker
 
     def _clean_item_data(self, item):
         """
@@ -148,7 +153,7 @@ class MongoModuleStore(ModuleStore):
             data_cache,
             self.default_class,
             resource_fs,
-            strict_error_handler,
+            self.error_tracker,
             render_to_string,
         )
         return system.load_item(item['location'])

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -3,7 +3,7 @@ from fs.osfs import OSFS
 from importlib import import_module
 from lxml import etree
 from path import path
-from xmodule.errorhandlers import logging_error_handler
+from xmodule.errortracker import null_error_tracker
 from xmodule.x_module import XModuleDescriptor, XMLParsingSystem
 from xmodule.mako_module import MakoDescriptorSystem
 from cStringIO import StringIO
@@ -29,7 +29,7 @@ def clean_out_mako_templating(xml_string):
     return xml_string
 
 class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
-    def __init__(self, xmlstore, org, course, course_dir, error_handler, **kwargs):
+    def __init__(self, xmlstore, org, course, course_dir, error_tracker, **kwargs):
         """
         A class that handles loading from xml.  Does some munging to ensure that
         all elements have unique slugs.
@@ -89,9 +89,9 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
         resources_fs = OSFS(xmlstore.data_dir / course_dir)
 
         MakoDescriptorSystem.__init__(self, load_item, resources_fs,
-                                      error_handler, render_template, **kwargs)
+                                      error_tracker, render_template, **kwargs)
         XMLParsingSystem.__init__(self, load_item, resources_fs,
-                                  error_handler, process_xml, **kwargs)
+                                  error_tracker, process_xml, **kwargs)
 
 
 
@@ -101,7 +101,7 @@ class XMLModuleStore(ModuleStore):
     """
     def __init__(self, data_dir, default_class=None, eager=False,
                  course_dirs=None,
-                 error_handler=logging_error_handler):
+                 error_tracker=null_error_tracker):
         """
         Initialize an XMLModuleStore from data_dir
 
@@ -116,8 +116,8 @@ class XMLModuleStore(ModuleStore):
         course_dirs: If specified, the list of course_dirs to load. Otherwise,
             load all course dirs
 
-        error_handler: The error handler used here and in the underlying
-                DescriptorSystem.  By default, raise exceptions for all errors.
+        error_tracker: The error tracker used here and in the underlying
+                DescriptorSystem.  By default, ignore all messages.
                 See the comments in x_module.py:DescriptorSystem
         """
 
@@ -125,7 +125,7 @@ class XMLModuleStore(ModuleStore):
         self.data_dir = path(data_dir)
         self.modules = {}  # location -> XModuleDescriptor
         self.courses = {}  # course_dir -> XModuleDescriptor for the course
-        self.error_handler = error_handler
+        self.error_tracker = error_tracker
 
         if default_class is None:
             self.default_class = None
@@ -154,7 +154,7 @@ class XMLModuleStore(ModuleStore):
             except:
                 msg = "Failed to load course '%s'" % course_dir
                 log.exception(msg)
-                error_handler(msg)
+                error_tracker(msg)
 
 
     def load_course(self, course_dir):
@@ -192,7 +192,7 @@ class XMLModuleStore(ModuleStore):
                 course = course_dir
 
             system = ImportSystem(self, org, course, course_dir,
-                                  self.error_handler)
+                                  self.error_tracker)
 
             course_descriptor = system.process_xml(etree.tostring(course_data))
             log.debug('========> Done with course import from {0}'.format(course_dir))

common/lib/xmodule/xmodule/raw_module.py

-from pkg_resources import resource_string
 from lxml import etree
 from xmodule.editing_module import EditingDescriptor
-from xmodule.mako_module import MakoModuleDescriptor
 from xmodule.xml_module import XmlDescriptor
 import logging
+import sys
 
 log = logging.getLogger(__name__)
 
@@ -20,13 +19,12 @@ class RawDescriptor(XmlDescriptor, EditingDescriptor):
         try:
             return etree.fromstring(self.definition['data'])
         except etree.XMLSyntaxError as err:
+            # Can't recover here, so just add some info and
+            # re-raise
             lines = self.definition['data'].split('\n')
             line, offset = err.position
             msg = ("Unable to create xml for problem {loc}. "
                    "Context: '{context}'".format(
                     context=lines[line - 1][offset - 40:offset + 40],
                     loc=self.location))
-            log.exception(msg)
-            self.system.error_handler(msg)
-            # no workaround possible, so just re-raise
-            raise
+            raise Exception, msg, sys.exc_info()[2]

common/lib/xmodule/xmodule/x_module.py

@@ -454,15 +454,15 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
             #        etree.fromstring(xml_data).tag,class_))
 
             descriptor = class_.from_xml(xml_data, system, org, course)
-        except (ResourceNotFoundError, XMLSyntaxError) as err:
+        except Exception as err:
             # Didn't load properly.  Fall back on loading as an error
             # descriptor.  This should never error due to formatting.
 
             # Put import here to avoid circular import errors
             from xmodule.error_module import ErrorDescriptor
 
-            #system.error_handler("Error loading from xml.")
-            descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course)
+            system.error_tracker("Error loading from xml.")
+            descriptor = ErrorDescriptor.from_xml(xml_data, system, org, course, err)
 
         return descriptor
 
@@ -533,16 +533,19 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
 
 
 class DescriptorSystem(object):
-    def __init__(self, load_item, resources_fs, error_handler, **kwargs):
+    def __init__(self, load_item, resources_fs, error_tracker, **kwargs):
         """
         load_item: Takes a Location and returns an XModuleDescriptor
 
         resources_fs: A Filesystem object that contains all of the
             resources needed for the course
 
-        error_handler: A hook for handling errors in loading the descriptor.
-            Must be a function of (error_msg, exc_info=None).
-            See errorhandlers.py for some simple ones.
+        error_tracker: A hook for tracking errors in loading the descriptor.
+            Used for example to get a list of all non-fatal problems on course
+            load, and display them to the user.
+
+            A function of (error_msg). errortracker.py provides a
+            handy make_error_tracker() function.
 
             Patterns for using the error handler:
                try:
@@ -551,10 +554,8 @@ class DescriptorSystem(object):
                except SomeProblem:
                   msg = 'Grommet {0} is broken'.format(x)
                   log.exception(msg) # don't rely on handler to log
-                  self.system.error_handler(msg)
-                  # if we get here, work around if possible
-                  raise # if no way to work around
-                       OR
+                  self.system.error_tracker(msg)
+                  # work around
                   return 'Oops, couldn't load grommet'
 
                OR, if not in an exception context:
@@ -562,25 +563,26 @@ class DescriptorSystem(object):
                if not check_something(thingy):
                   msg = "thingy {0} is broken".format(thingy)
                   log.critical(msg)
-                  error_handler(msg)
-                  # if we get here, work around
-                  pass   # e.g. if no workaround needed
+                  self.system.error_tracker(msg)
+
+               NOTE: To avoid duplication, do not call the tracker on errors
+               that you're about to re-raise---let the caller track them.
         """
 
         self.load_item = load_item
         self.resources_fs = resources_fs
-        self.error_handler = error_handler
+        self.error_tracker = error_tracker
 
 
 class XMLParsingSystem(DescriptorSystem):
-    def __init__(self, load_item, resources_fs, error_handler, process_xml, **kwargs):
+    def __init__(self, load_item, resources_fs, error_tracker, process_xml, **kwargs):
         """
-        load_item, resources_fs, error_handler: see DescriptorSystem
+        load_item, resources_fs, error_tracker: see DescriptorSystem
 
         process_xml: Takes an xml string, and returns a XModuleDescriptor
             created from that xml
         """
-        DescriptorSystem.__init__(self, load_item, resources_fs, error_handler,
+        DescriptorSystem.__init__(self, load_item, resources_fs, error_tracker,
                                   **kwargs)
         self.process_xml = process_xml
 

common/lib/xmodule/xmodule/xml_module.py

-from collections import MutableMapping
 from xmodule.x_module import XModuleDescriptor
 from xmodule.modulestore import Location
 from lxml import etree
@@ -8,13 +7,12 @@ import traceback
 from collections import namedtuple
 from fs.errors import ResourceNotFoundError
 import os
+import sys
 
 log = logging.getLogger(__name__)
 
-
 _AttrMapBase = namedtuple('_AttrMap', 'metadata_key to_metadata from_metadata')
 
-
 class AttrMap(_AttrMapBase):
     """
     A class that specifies a metadata_key, and two functions:
@@ -116,11 +114,12 @@ class XmlDescriptor(XModuleDescriptor):
 
             # VS[compat]
             # TODO (cpennington): If the file doesn't exist at the right path,
-            # give the class a chance to fix it up. The file will be written out again
-            # in the correct format.
-            # This should go away once the CMS is online and has imported all current (fall 2012)
-            # courses from xml
-            if not system.resources_fs.exists(filepath) and hasattr(cls, 'backcompat_paths'):
+            # give the class a chance to fix it up. The file will be written out
+            # again in the correct format.  This should go away once the CMS is
+            # online and has imported all current (fall 2012) courses from xml
+            if not system.resources_fs.exists(filepath) and hasattr(
+                    cls,
+                    'backcompat_paths'):
                 candidates = cls.backcompat_paths(filepath)
                 for candidate in candidates:
                     if system.resources_fs.exists(candidate):
@@ -130,19 +129,11 @@ class XmlDescriptor(XModuleDescriptor):
             try:
                 with system.resources_fs.open(filepath) as file:
                     definition_xml = cls.file_to_xml(file)
-            except (ResourceNotFoundError, etree.XMLSyntaxError):
+            except Exception:
                 msg = 'Unable to load file contents at path %s for item %s' % (
                     filepath, location.url())
-
-                log.exception(msg)
-                system.error_handler(msg)
-                # if error_handler didn't reraise, work around problem.
-                error_elem = etree.Element('error')
-                message_elem = etree.SubElement(error_elem, 'error_message')
-                message_elem.text = msg
-                stack_elem = etree.SubElement(error_elem, 'stack_trace')
-                stack_elem.text = traceback.format_exc()
-                return {'data': etree.tostring(error_elem)}
+                # Add info about where we are, but keep the traceback
+                raise Exception, msg, sys.exc_info()[2]
 
         cls.clean_metadata_from_xml(definition_xml)
         return cls.definition_from_xml(definition_xml, system)

lms/djangoapps/courseware/management/commands/clean_xml.py

@@ -10,7 +10,7 @@ from lxml import etree
 from django.core.management.base import BaseCommand
 
 from xmodule.modulestore.xml import XMLModuleStore
-
+from xmodule.errortracker import make_error_tracker
 
 def traverse_tree(course):
     '''Load every descriptor in course.  Return bool success value.'''
@@ -21,23 +21,6 @@ def traverse_tree(course):
 
     return True
 
-def make_logging_error_handler():
-    '''Return a tuple (handler, error_list), where
-    the handler appends the message and any exc_info
-    to the error_list on every call.
-    '''
-    errors = []
-
-    def error_handler(msg, exc_info=None):
-        '''Log errors'''
-        if exc_info is None:
-            if sys.exc_info() != (None, None, None):
-                exc_info = sys.exc_info()
-
-        errors.append((msg, exc_info))
-
-    return (error_handler, errors)
-
 
 def export(course, export_dir):
     """Export the specified course to course_dir.  Creates dir if it doesn't exist.
@@ -70,14 +53,14 @@ def import_with_checks(course_dir, verbose=True):
     data_dir = course_dir.dirname()
     course_dirs = [course_dir.basename()]
 
-    (error_handler, errors) = make_logging_error_handler()
+    (error_tracker, errors) = make_error_tracker()
     # No default class--want to complain if it doesn't find plugins for any
     # module.
     modulestore = XMLModuleStore(data_dir,
                    default_class=None,
                    eager=True,
                    course_dirs=course_dirs,
-                   error_handler=error_handler)
+                   error_tracker=error_tracker)
 
     def str_of_err(tpl):
         (msg, exc_info) = tpl