PR ReviewIt’s the error handling, again
This is part of a PR related to making sure that disposing once works. It contains this code:
This loses critically important information. Namely, the stack trace of the original exception. That leaves aside the issue that an aggregate exception may contain multiple exceptions as well.
In general, and I know this is old hat, whenever you see “throw e;” or “throw e.InnerException;” of any kind, you should always treat it as a bug.
More posts in "PR Review" series:
- (19 Dec 2017) The simple stuff will trip you
- (08 Nov 2017) Encapsulation stops at the assembly boundary
- (25 Oct 2017) It’s the error handling, again
- (23 Oct 2017) Beware the things you can’t see
- (20 Oct 2017) Code has cost, justify it
- (10 Aug 2017) Errors, errors and more errors
- (21 Jul 2017) Is your error handling required?
- (23 Jun 2017) avoid too many parameters
- (21 Jun 2017) the errors should be nurtured
Comments
The ExceptionDispatchInfo class has some functionality if you really need this behavior:
Will throw ex, using its original stack trace. I've used this to "unwrap" a TargetInvocationException and just keep the underlying exception.
Thank you! Any recommendations about the right way?
I wish AggregateExceptions were thrown only in case of multiple exceptions, but oh well ... It completely broke exception handling behavior like catch (UnauthorisedException uex) ... writing simple code is out the window due to AggregateExceptions/
Here is what ExceptionDispatchInfo.Capture will result in:
You lose a what is in the middle, but keep the rest.
@Janivz: i suppose a simple “throw;” would do the job.
njy, that is what I would rather do, or even throw another exception with the inner exception as the new one's inner exception, with additional details
Instead of catching and rethrowing using
ExceptionDispatchInfo
, I would usetask.GetAwaiter().GetResult()
. It's kind of confusing (why is it callingGetAwaiter()
if it's not related toawait
?), but it's the simplest code that does the right thing (and probably also fastest).Svick is right. The problem is the .Wait(). Wait() and .Result will always wrap the original exception in an aggregate exception. In cases of tasks it is usually just containing a single exception even when the task represent a WhenAll. Only if the user code throws an aggregate exception then you get multiple exception and the hierachy needs to be flattened. In thid case you want to use GetAwaiter().GetResult() because it properly throws the root exception. Obviously having an async callstack about be best but I guess there are reasons why this sample is sync.
While in general what you explained here is important to care about I think choosing the task based approach as a sample here misses a bit the point you were trying to make. My 2cents
Comment preview