On 08/11/2013 10:16 AM, Anil Gangolli wrote:
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.
Done the first part. As for the second, it's used in HitCountQueue and
ReferrerQueueManagerImpl. It seems to be six on one side, half-a-dozen
on the other whether to try to silently continue with java.lang.Errors
(with the understanding that, if unsuccessful, these two queues aren't
going to be working anyway and that nobody's going to know unless
they're reading the log files regularly) vs. having Roller actually halt
on a java.lang.Error (making it clear and obvious that something's wrong
with the setup). I would say let's run it for a few months as-is, find
out what (if any) java.lang.Errors are occurring that we can recover
from and just trap *those* (or fix the code so they won't occur).
Sometimes you want the system to halt during development so you are
immediately aware of a problem, so you can get it fixed.
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.
OK, I'll keep these as-is, except throwing the ThreadDeaths as the
Javadoc advises.
Thanks,
Glen
Regards,
Glen
