Re-raising a RuntimeError - good practice?

[Steven D'Aprano]

On Wed, 23 Oct 2013 20:23:21 -0700, Victor Hooi wrote:

> Hi,
> 
> I have a Python class that represents a loading job.
> 
> Each job has a run_all() method that calls a number of other class
> methods.
> 
> I'm calling run_all() on a bunch of jobs.
> 
> Some of methods called by run_all() can raise exceptions (e.g. missing
> files, DB connection failures) which I'm catching and logging.
> 
> If any of the methods fails, I'd like to terminate running that job, and
> move onto the next job.

That should be simple:

for job in many_jobs:
    try:
        job.run_all()
    except RunAllException as err:
        logger.error(err)


Another approach is to put all your error handling in the one place (or 
as few places as possible):

for job in jobs:
    try:
        try:
            job.run_all()
        except Exception as err:  # catch *everything*
            logger.error(err)
            raise
    except (SpamError, EggsError, CheeseError):
        # We expect these exceptions, and ignore them.
        # Everything else is a bug.
        pass


With this approach, only your top-level code needs to care about catching 
exceptions. Although some of your lower-level code may do so as well, but 
it's (potentially) no longer their responsibility to deal with logging.

However, the down-side of this approach is that the list of "ignorable 
exceptions" needs to be kept small and manageable. That may requires 
either diligence on your part, tracking which exceptions can be raised, 
or having each method be responsible for generating the right exceptions. 
See below.



> I'm currently re-raising a RuntimeError, so that I can break out the
> run_all() and move onto the next job.
> 
> For example:
> 
>     def run_all(self):
>         self.logger.debug('Running loading job for %s' %
>                            self.friendly_name)
>         try:
>             self.export_to_csv()
>             self.gzip_csv_file()
>             self.upload_to_foo()
>             self.load_foo_to_bar()
>         except RuntimeError as e:
>             self.logger.error('Error running job %s' %
>                               self.friendly_name)

At first glance, that looks pretty nasty to me. The *actual* exception 
causing the problem is lost. This is one step up from the infamous, and 
horrible

"An error occurred"

message. At least you report the name of the job that failed, but you 
don't report the actual error, or which component failed.

Fortunately the actual error is logged by the method which fails, which 
makes it much better, but suggests that this exception handler isn't 
doing anything: the error message it generates is pointless and is just 
noise in the log file, so you ought to get rid of it.



>     def export_to_csv(self):
>     ...
>         try:
>             with open(self.export_sql_file, 'r') as f:
>                 self.logger.debug('Attempting to read in SQL export
>                 statement from %s' % self.export_sql_file)
>                 self.export_sql_statement = f.read()
>                 self.logger.debug('Successfully read in SQL export
>                 statement')
>         except Exception as e:
>             self.logger.error('Error reading in %s - %s' %
>                              (self.export_sql_file, e), exc_info=True) 
>             raise RuntimeError
> 
> My question is - is the above Pythonic, or an acceptable practice?


It's certainly acceptable practice, but the downside is that *every* 
method needs to care about catching exceptions and logging them. It's 
better to push as much of that responsibility as you can out of the 
individual methods and into the caller.

Sometimes, though, a method may have to deal with too many different 
possible exceptions, which makes managing the list of expected exceptions 
to ignore too hard. In that case, use custom exceptions:


class SpamException(Exception):
    pass


class Job:
    def export_to_csv(self):
     ...
     try:
         with open(self.export_sql_file, 'r') as f:
             self.logger.debug('Attempting to read in SQL export
                 statement from %s' % self.export_sql_file)
             self.export_sql_statement = f.read()
             self.logger.debug('Successfully read in SQL export
                 statement')
     except IOError, OSError as err:
         # Low-level methods should always be conservative in what they
         # catch, not too eager to cover up bugs in the code.
         raise SpamError("blah blah blah", err)

or similar. E.g. you can use RuntimeError, as you do.



-- 
Steven