Catch practices: avoid losing catched exception information. #1555
Conversation
There was a problem hiding this comment.
There is also the converse subject: avoid being too noisy with exceptions.
Many cases log fully the exception then re-throw it (directly or as inner of another one). I think this is a bad pattern tending to bloat the logs, and it should be avoided: log at most a message in such case. But as this would reduce the available logs data, maybe should it be changed only for 6.0 in a separated PR.
| @@ -362,8 +362,6 @@ private void DoCompile(IDictionary<string, string> replacements, bool shallow, S | |||
| } | |||
| catch ( RecognitionException e ) | |||
| { | |||
| // we do not actually propogate ANTLRExceptions as a cause, so | |||
| // log it here for diagnostic purposes | |||
There was a problem hiding this comment.
Many cases do propagate them as inner exception, so removing the comment, they are now all propagated.
But I let the log in place, as this could be considered as a (very minor) breaking change to remove logs.
fredericDelaporte
force-pushed
the
catch
branch
from
2e9ad1c to
81a7880
Compare
All catches have been reviewed for avoiding losing original exception information: * When another exception is raised, it should embed the original one as an inner exception. * Otherwise when re-thrown, it should be done without altering its original stack-trace. Preferred way is of course to use "throw;" in the catch clause without specifying the exception, but otherwise ReflectHelper provide a way to preserve the stack-trace. * Otherwise it should be transmitted to the logger. Some cases still do not follow this pattern for avoiding being too noisy, like "Try" utilities, failure in closing an already failed object, some "implemented by exception" features, ...
fredericDelaporte
force-pushed
the
catch
branch
from
81a7880 to
9a4acd4
Compare
| { | ||
| // ignore it; the incoming property could not be found so we | ||
| // cannot be sure what to do here. At the very least, the | ||
| // safest is to simply not apply any dereference toggling... | ||
| Log.Info(ex, "Unable to find property {0}, no dereference will be handled for it.", propertyName); |
There was a problem hiding this comment.
This shall be at Debug level. Info is too high.
There was a problem hiding this comment.
This is for matching the other log. Not very important, so well:
- change both to Debug?
- just this one?
- keep them both at Info?
All catches have been reviewed for avoiding losing original exception information:
Preferred way is of course to use "throw;" in the catch clause without specifying the exception, but otherwise ReflectHelper provide a way to preserve the stack-trace.
Some cases still do not follow this pattern for avoiding being too noisy, like "Try" utilities, failure in
closing an already failed object, some "implemented by exception" features, ...