I just put code similar to this in for TaskScheduler, I'm not sure of
the need yet for WorkerThread, as unlike TaskScheduler it's *doing*
but not *calling* tasks (and would like to continue even if a single
task it calls throws a throwable) and not trapping Throwable allows
the Throwable to percolate up to the TaskScheduler, where it can be
properly handled.
A couple of comments: The cleanup in WorkerThread will now be skipped
in the case of an uncaught Throwable. You can probably fix that by
moving the cleanup to a finally clause, (where it probably should have
been in the first place), but more than that, I am not sure all of the
code that uses WorkerThread is actually ready for uncaught Throwables
and to restart if the thread is meant to be long-lived. One example I
notice is that ContinuousWorkerThread, which extends WorkerThread, has
the problem that it may now not be as "continuous" as desired; it seems
to have been relying on super.run() (i.e. WorkerThread.run()) never
throwing anything. Maybe the wrapping should be there, but you should
also look at other places that WorkerThread is used and what happens if
it dies.
BTW, are you sure the two cases in
org.apache.roller.weblogger.business.GuiceWebloggerProvider can't rely
on Exception instead of Throwable? I kept those two at Throwable per
your earlier comment on them but is seems like Exception may be good
enough for those.
Commonly Class.forName can throw both ClassNotFoundException and
NoClassDefFoundError, and possibly various IOErrors depending on the
ClassLoader used. So catching Throwable is not uncommon when using
that. It looks like the current code takes whatever was caught and
rethrows it wrapped in a RuntimeException. I think in either case the
webapp will die during initialization, so it probably does not matter
that much. I'm not sure if the wrapping was done for some purpose or
not, like inducing better logging from a higher level. Dave might know
this one.
Regards,
Glen
