Code review comment for lp://staging/~thumper/launchpad/more-careful-network-service-usage

On Fri, Apr 30, 2010 at 05:26:29AM -0000, Tim Penhey wrote:
> === modified file 'lib/lp/services/job/runner.py'
> --- lib/lp/services/job/runner.py 2010-04-06 03:37:16 +0000
> +++ lib/lp/services/job/runner.py 2010-04-30 05:22:33 +0000

> @@ -181,13 +184,24 @@
> with self.error_utility.oopsMessage(
> dict(job.getOopsVars())):
> try:
> - self.logger.debug('Running %r', job)
> - self.runJob(job)
> - except job.user_error_types, e:
> - job.notifyUserError(e)
> - except Exception:
> + try:
> + self.logger.debug('Running %r', job)
> + self.runJob(job)
> + except job.user_error_types, e:
> + job.notifyUserError(e)
> + except Exception:
> + info = sys.exc_info()
> + return self._doOops(job, info)
> + except Exception, e:
> + # This only happens if sending attempting to notify users
> + # about errors fails for some reason (like a misconfigured
> + # email server).
> + self.logger.exception(e)
> info = sys.exc_info()
> - return self._doOops(job, info)
> + self.error_utility.raising(info)
> + oops = self.error_utility.getLastOopsReport()
> + # Returning the oops says something went wrong.
> + return oops

As discussed on IRC, this new exception handling is untested. To avoid
things breaking, and to avoid people getting tempted into refactoring
the outer exception handling into using self._doOops(), we need two
tests; one where job.notifyUserError() errors out, and one where
job.notifyOops() errors out.

Since it's EOD for you now, I'd be willing to let this branch land
without tests (since it won't break anything not already broken), so
that you can QA it more easily on Monday. Given that you promise to
write the tests on Monday, of course.

    vote approve

--
Björn Tillenius | https://launchpad.net/~bjornt