Skip to content

Only catch TargetInvocationException's - #211

Merged
pakrym merged 1 commit into
dotnet:devfrom
henkmollema:capture-exception
Apr 19, 2017
Merged

Only catch TargetInvocationException's#211
pakrym merged 1 commit into
dotnet:devfrom
henkmollema:capture-exception

Conversation

@henkmollema

Copy link
Copy Markdown

There might be cases when the inner exception is null. This results in another exception from ExceptionDispatchInfo.Capture because the source parameter is null.

@dnfclas

dnfclas commented Apr 3, 2017

Copy link
Copy Markdown

@henkmollema,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@henkmollema

Copy link
Copy Markdown
Author

I actually ran into this when attempting to activate an abstract type. I proposed a more specific fix for that in #210. However, there might be other scenarios where the inner exception is null.

@Eilon
Eilon requested a review from pakrym April 12, 2017 17:08
@Eilon

Eilon commented Apr 12, 2017

Copy link
Copy Markdown

@pakrym - can you take a look?

return _constructor.Invoke(_parameterValues);
}
catch (Exception ex)
catch (Exception ex) when (ex.InnerException != null)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should just catch different exception type? Like TargetInvocationException?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would work as well. There are some other exceptions that Invoke might throw though. Do you think it's safe to assume it will just throw a TargetInvocationException?

@rynowak rynowak Apr 18, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invoke specifically throws TargetInvocationException when it was thrown by the method that's called. The other exception types indicate a mistake on the part of the code doing reflection

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course. Not sure what I was thinking 😅
I'll change the catch-clause.

@pakrym

pakrym commented Apr 18, 2017

Copy link
Copy Markdown

And I would prefer having a test is possible.

@henkmollema

Copy link
Copy Markdown
Author

@pakrym I'll try to create a test case for this. I threw my own glasses when #210 got merged though. 😄

When a `TargetInvocationException` occurs, we want to throw its inner exception. Other exception types are not guaranteed to have an inner exception.
@henkmollema henkmollema changed the title Throw root exception when no inner exception Only catch TargetInvocationException's Apr 19, 2017
@henkmollema

Copy link
Copy Markdown
Author

@pakrym @rynowak I've changed the catch-clause and added a test for it.

@pakrym
pakrym merged commit 2088c29 into dotnet:dev Apr 19, 2017
@pakrym

pakrym commented Apr 19, 2017

Copy link
Copy Markdown

@henkmollema thank you!

@ghost ghost locked as resolved and limited conversation to collaborators May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants