I would still be catching Exception (which captures most everything
anyway.) I don't think our code is throwing java.lang.Errors as a part
of normal application processing, even during cleanup, those are for
(should be) rare non-application system bugs that need to be handled by
higher levels outside the application, not masked by Roller with Roller
subsequently trying to continue to run. If Roller is throwing
java.lang.Errors as part of its normal processing, let's fix that issue
and/or have it throw something else, rather than just catch Throwables
to mask that problem.
If there are specific Java.lang.Errors subclasses we do want to trap, as
you mention below, we can trap those explicitly in those places where it
is legitimate for the application to be throwing them, while letting the
other java.lang.Errors percolate outside the app. But catch(Throwable)
casts too wide a net, and it's a statement that we always want Roller to
continue no matter what the exception (VirtualMachineError, ThreadDeath,
LinkageError, etc.) and further a statement that we always want to
suppress such exceptions from the J2EE app server or servlet container
hosting Roller, positions I'm not so sure about.
Glen
On 08/06/2013 01:52 PM, Anil Gangolli wrote:
Another type of situation where this may be warranted is in finally
clauses doing cleanup, where one does not want an error to mask an
original exception and the cleanup problem may be logged but not
critical.
--a.
On 8/6/13 10:31 AM, Glen Mazza wrote:
Yes, I'll look at them case-by-case (if no objections from anyone else).
Glen
On 08/06/2013 01:20 PM, Anil Gangolli wrote:
Some of these may be intended and necessary when catching IOError,
NoClassDefFoundError
and others. [At least the instance in GuiceWebloggerProvider, for
example, seems to be there to catch the latter.] Some of them
appear not to be needed, and it may be possible to make them more
specific, but I wouldn't just uniformly change them. I'd suggest
looking at the 34 cases individually.
--a.
On 8/6/13 10:01 AM, Glen Mazza wrote:
Hi folks, Sonar is complaining that in 34 cases[1] we're catching
Throwable, which is not recommended as it ends up catching
java.lang.Error messages
(http://docs.oracle.com/javase/7/docs/api/java/lang/Error.html) for
non-application system problems that should instead percolate up to
the JDK and/or application server to be handled and reported at
that level. The googling I've done ([2] for example) tends to
agree with Sonar outside of a few special circumstances (e.g.,
calling a poorly written 3rd party library that throws Errors for
normal app exceptions).
I'd like to switch them to catching Exception() instead (which has
its own headaches[3], but is an improvement), which lets the
java.lang.Errors pass to the JDK and/or servlet container. If it
turns out in a few of the cases it might actually be better to
catch Throwable (looking at the 34, I'm not seeing anything
though), we can re-instate it in those cases, but this time just
add a comment on why we're catching throwable. WDYT?
Thanks,
Glen
[1]
https://analysis.apache.org/drilldown/issues/145470?&rule=pmd%3AAvoidCatchingThrowable&rule_sev=CRITICAL&severity=CRITICAL
[2]
http://stackoverflow.com/questions/6083248/is-it-a-bad-practice-to-catch-the-throwable
http://stackoverflow.com/questions/352780/when-to-catch-java-lang-error
[3] http://doc.nuxeo.com/display/CORG/Catching+Exceptions
