Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception detail in header is lost if handler throws an AggregateException #2365

Closed
SimonCropp opened this issue Sep 10, 2014 · 5 comments
Closed
Labels
Milestone

Comments

@SimonCropp
Copy link
Contributor

When a Handler throws an AggregateException.

class Handler : IHandleMessages<Message>
{
    public void Handle(Message message)
    {
        try
        {
            MethodThatThrows();
        }
        catch (Exception exception)
        {
            throw new AggregateException("My Exception", exception);
        }
    }

    void MethodThatThrows()
    {
        throw new Exception("My Inner Exception");
    }
}

TransportReceiver does a GetBaseException

if (ex is AggregateException)
{
       ex = ex.GetBaseException();
}

Which will unwrap the AggregateException converting it to the instance of Exception("My Inner Exception");. And hence Exception("My Inner Exception"); will be passed to IManageMessageFailures instead of the correct AggregateException("My Exception", exception);

Note that we also do the same thing (ex = ex.GetBaseException();) again in FirstLevelRetries so it needs to be fixed in two locations

@SimonCropp
Copy link
Contributor Author

@andreasohlund here is one to ruin your lunch

@SimonCropp SimonCropp added the Bug label Sep 10, 2014
@SimonCropp SimonCropp added this to the 4.7.0 milestone Sep 10, 2014
@johnsimons
Copy link
Member

This is actually a bit more different then just the highlighted code in the description.

So there are 2 sides to this scenario.

  1. User throws an AggregateException that contains only one exception (the example above)
  2. User throws an AggregateException that contains multiple exceptions

In scenario 1, yes we are unwrapping the AggregateException and displaying the inner exception.
Yes agree, we are in essence losing the user thrown exception, but IMO we are not losing the true meaning of the error (if you understand what I mean).

In scenario 2, an the original AggregateException is the result of calling GetBaseException() (yes I have been caught in this situation and decompiled the code). So we are not losing the original thrown exception.

Scenario 1 could be remedied with a know issue section in the release notes?

@SimonCropp
Copy link
Contributor Author

we are not losing the true meaning of the error

perhaps but we are loosing the stack trace and the message. I dont think documenting this as a known bug is enough

@johnsimons
Copy link
Member

we are loosing the stack trace and the message. I dont think documenting this as a known bug is enough

I'm pretty sure we are not losing the message (AggregateException.ToString is smart enough to figure out if there is only 1 exception will display that exception message), regarding the stacktrace, not sure, have you tested it?

@SimonCropp
Copy link
Contributor Author

fixed in d414f49

@SimonCropp SimonCropp changed the title Exception is lost if handler throws an AggregateException Exception detail in header is lost if handler throws an AggregateException Oct 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants