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

Add a way to opt out of TargetInvocationException wrapping on late-bound invokes. #22866

Closed
kingces95 opened this Issue Aug 2, 2017 · 16 comments

Comments

Projects
None yet
6 participants
@kingces95

kingces95 commented Aug 2, 2017

Late-bound invokes through Reflection wrap exceptions thrown by the target in a TargetInvocationException. In many cases, this behavior is not desired and counter-productive. For those who want the actual exception, it requires unwrapping the TargetInvocationException to rethrow the inner exception and to retrieve the full call stack. The fact that every exception is "caught" hampers the normal debugging experience. It'd be useful to have a way to opt out of this wrapping.

We can do this without adding lots of new overloads by adding a new member to the BindingFlags enum: BindingFlag.DoNotWrapExceptions. Setting this bit would disable the wrapping behavior.

Here is a fiddle of the code sample included below: https://dotnetfiddle.net/o9qUht

public class Program
{
	public static void Main()
	{
		try {
			var bf = BindingFlags.Static | 
                            BindingFlags.Public | 
                            BindingFlags.InvokeMethod;

                        bf |= BindingFlags.DoNotWrapExceptions;

			typeof(Program).InvokeMember("LateBoundTarget", bf, null, null, null);

		} catch(TargetInvocationException e) {
			Console.WriteLine("fail");

		} catch(InvalidOperationException e) {
			Console.WriteLine("success");
		}
	}
	
	public static void LateBoundTarget() {
		throw new InvalidOperationException();
	}
}

Apis that would be affected:

    public static class Activator
    {
        public static object CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, object[] args, CultureInfo culture);
        public static object CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, object[] args, CultureInfo culture, object[] activationAttributes);
    }

    public class Type
    {
        public object InvokeMember(string name, BindingFlags invokeAttr, Binder binder, object target, object[] args);
        public object InvokeMember(string name, BindingFlags invokeAttr, Binder binder, object target, object[] args, CultureInfo culture);
        public abstract object InvokeMember(string name, BindingFlags invokeAttr, Binder binder, object target, object[] args, ParameterModifier[] modifiers, CultureInfo culture, string[] namedParameters);
    }

    public class Assembly
    {
        public virtual object CreateInstance(string typeName, bool ignoreCase, BindingFlags bindingAttr, Binder binder, object[] args, CultureInfo culture, object[] activationAttributes);
    }

    public abstract class ConstructorInfo : MethodBase
    {
        public abstract object Invoke(BindingFlags invokeAttr, Binder binder, object[] parameters, CultureInfo culture);
    }

    public abstract class MethodBase : MemberInfo
    {
        public abstract object Invoke(object obj, BindingFlags invokeAttr, Binder binder, object[] parameters, CultureInfo culture);
    }

    public abstract class PropertyInfo : MemberInfo
    {
        public abstract object GetValue(object obj, BindingFlags invokeAttr, Binder binder, object[] index, CultureInfo culture);
        public abstract void SetValue(object obj, object value, BindingFlags invokeAttr, Binder binder, object[] index, CultureInfo culture);
    }

[EDIT] Added C# syntax highlight by @karelz

@AtsushiKan AtsushiKan added this to the Future milestone Aug 2, 2017

@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Aug 2, 2017

@MichalStrehovsky - Thoughts? This does seem like a pragmatic way to address a behavior that's been a long-standing irritance (at least for me.)

@AtsushiKan AtsushiKan self-assigned this Aug 2, 2017

@MichalStrehovsky

This comment has been minimized.

Member

MichalStrehovsky commented Aug 2, 2017

@MichalStrehovsky - Thoughts?

Heh, I never even saw the overload of Invoke/GetValue/SetValue that takes BindingFlags. I guess this should work. We just have to make sure we do whatever is the consistent thing to do when someone passes the new BindingFlag to e.g. Type.GetMethod (I assume we wouldn't return a different MethodInfo based on that).

Do we want to also take this opportunity to propose an overload of Activator.CreateInstance<T> that takes binding flags? We would obviously only respect the new binding flag and do whatever is the consistent thing for anything else (throw/ignore).

@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Aug 2, 2017

Activator.CreateInstance<T> - if we add a BindingFlags parameter, people will probably expect it to honor Public/NonPublic/Instance/Static as well as the new bit.

@JonHanna

This comment has been minimized.

Collaborator

JonHanna commented Aug 2, 2017

Surely CreateInstanceis ipso facto Instance? The behaviour with the other flags could be like that of the non-generic version that takes them, but its use is already more limited (one can normally do new T() if T is known at jit time to be suitable for use with it), so perhaps there's no gap to fill.

@MichalStrehovsky

This comment has been minimized.

Member

MichalStrehovsky commented Aug 2, 2017

one can normally do new T()

new T() actually expands to Activator.CreateInstance<T> with all the baggage (exceptions thrown from the constructor will be wrapped in TargetInvocationException). That compat ship has unfortunately already sailed. The new overload would be for people who expect new T() to do the reasonable thing but can't express it currently. We can make Activator.CreateInstance<T> super fast, as opposed to the overload that takes a Type (we already do make it fast on AOT platforms).

Making it also work for private constructors is a bunch of work for AOT, so I would propose we make BindingFlags.NonPublic throw and see how the feedback looks like?

@JonHanna

This comment has been minimized.

Collaborator

JonHanna commented Aug 2, 2017

with all the baggage

True, it could be worth losing the syntactic sugar of new() for getting away from that.

Making it also work for private constructors is a bunch of work for AOT

Could it slow-path through (T)Activator.CreateInstance(typeof(T), … so even if it isn't as fast, it still produces the correct results?

@MichalStrehovsky

This comment has been minimized.

Member

MichalStrehovsky commented Aug 2, 2017

Could it slow-path through (T)Activator.CreateInstance(typeof(T), … so even if it isn't as fast, it still produces the correct results?

It could, but it always makes me sad to have APIs with perf characteristics that can't be reasoned about (sure, we could document that, but who reads docs?).

If we make BindingFlags.NonPublic a thing, it's an extra day of work for CoreRT. Without that, it's as little work as adding a when clause to the catch here.

@JonHanna

This comment has been minimized.

Collaborator

JonHanna commented Aug 2, 2017

To the consumer, the main speed difference between slow and not working is how quickly they get sad.

@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Aug 2, 2017

There are also other ways to achieve non-exception-wrapping CreateInstance<T>() - a boolean parameter or a differently named method. That might be prefereable to over-advertising a capability we intend to throw on.

In any case, we should probably keep that separate from this api proposal. If the basic notion of a non-wrapping Invoke doesn't fly with the api review folks, figuring out how to extend it to the generic CreateInstance becomes moot.

@AtsushiKan AtsushiKan changed the title from Add BindingFlags.PassExceptions to prevent TargetInvocationException wrapping to Need a way to opt out of TargetInvocationException wrapping on late-bound invokes. Aug 3, 2017

@AtsushiKan AtsushiKan changed the title from Need a way to opt out of TargetInvocationException wrapping on late-bound invokes. to Add a way to opt out of TargetInvocationException wrapping on late-bound invokes. Aug 3, 2017

@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Aug 3, 2017

Cleaned up the proposal a bit and promoted to api-ready-for-review.

We can tackle the CreateInstance<T>() thing separately if this gets approved.

@terrajobst

This comment has been minimized.

Member

terrajobst commented Aug 29, 2017

Looks good, the only suggestion is to rename PassExceptions to DoNotWrapExceptions. The rationale being that specifying ~PassExceptions could be read as catch-all.

var bf = BindingFlags.Public | BindingFlags.DoNotWrapExceptions;
@JonHanna

This comment has been minimized.

Collaborator

JonHanna commented Aug 29, 2017

That has nice didactic qualities too: Seeing DoNotWrapExceptions immediately teaches people that exceptions get wrapped!

@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Aug 29, 2017

Next steps: will add the enum value to BindingFlags in the code so both CoreCLR and CoreRT use the same (numerical) value. Then we open separate issues for implementation on CoreRT and CoreCLR. (Latter can be up for grabs.)

@karelz

This comment has been minimized.

Member

karelz commented Aug 29, 2017

Marking as up-for-grabs.
Complexity wise: medium (see next steps in previous comment from @AtsushiKan)

@karelz

This comment has been minimized.

Member

karelz commented Aug 29, 2017

FYI: The API review discussion was recorded - see https://youtu.be/VppFZYG-9cA?t=5527 (duration: 14 min)

AtsushiKan added a commit to AtsushiKan/corert that referenced this issue Aug 31, 2017

Add the enum value for BindingFlags.DoNotWrapExceptions
This was approved here.

dotnet/corefx#22866

This does not implement the feature, it just adds
the member to the enum so that CoreCLR and CoreRT
will agree on the value.

AtsushiKan added a commit to dotnet/corert that referenced this issue Aug 31, 2017

Add the enum value for BindingFlags.DoNotWrapExceptions (#4433)
This was approved here.

dotnet/corefx#22866

This does not implement the feature, it just adds
the member to the enum so that CoreCLR and CoreRT
will agree on the value.

AtsushiKan added a commit to AtsushiKan/corert that referenced this issue Aug 31, 2017

Implement BindingFlags.DoNotWrapExceptions on Project N
This was approved here.

dotnet/corefx#22866

Ok, this one actually makes the feature work. Turned
out not to be too hard.

There are a couple of drive-by items being done here:

- Since the other Invoke overload on MethodInvoker
  was only for its use, downgraded its visibility.

- Moved the catch in InvokeUtilites before the finally
  that copies back arguments. We don't want to wrap
  any exceptions out of the argument post-processing
  steps.

AtsushiKan added a commit to dotnet/corert that referenced this issue Aug 31, 2017

Implement BindingFlags.DoNotWrapExceptions on Project N (#4437)
This was approved here.

dotnet/corefx#22866

Ok, this one actually makes the feature work. Turned
out not to be too hard.

There are a couple of drive-by items being done here:

- Since the other Invoke overload on MethodInvoker
  was only for its use, downgraded its visibility.

- Moved the catch in InvokeUtilites before the finally
  that copies back arguments. We don't want to wrap
  any exceptions out of the argument post-processing
  steps.
@AtsushiKan

This comment has been minimized.

Contributor

AtsushiKan commented Aug 31, 2017

Project N implementation is now merged.

CoreCLR implementation is still up for grabs. The exception wrapping seems to occur in various places in C++ code so this will require some C++ work.

AtsushiKan added a commit to dotnet/coreclr that referenced this issue Sep 6, 2017

Support for not wrapping exceptions with TargetInvocationException. (#…
…13767)

* Support for not wrapping exceptions with TargetInvocationException.

For dotnet/corefx#22866.

* Respond to PR feedback.

Mainly by making "WrapExceptions" consistently positive in FCalls.

* Remove BindingFlags.DoNotWrapExceptions tests in deference to CoreFX tests.

@AtsushiKan AtsushiKan removed the up-for-grabs label Sep 6, 2017

@AtsushiKan AtsushiKan self-assigned this Sep 6, 2017

AtsushiKan added a commit to AtsushiKan/corefx that referenced this issue Sep 8, 2017

@karelz karelz modified the milestones: Future, 2.1.0 Oct 28, 2017

akoeplinger added a commit to mono/mono that referenced this issue Mar 29, 2018

Add BindingFlags.DoNotWrapExceptions (#7863)
The original design review is here: dotnet/corefx#22866 . To summarize, this adds a new flag to `BindingFlags` called `DoNotWrapExceptions`. When this flag is used to invoke a method using reflection, any exceptions thrown are not wrapped with a `TargetInvocationException`.

It has already been implemented in some other version of .NET:
- dotnet/corert#4437
- dotnet/coreclr#13767

I would be delighted if this handy feature was also available in Mono.

I have in part based my changes on the CoreCLR implementation. There all already tests for this feature in corefx, so I have added those to the corlib_xtest.dll.sources file.

I have a couple of concerns about my implementation:
- I notice that `DynamicMethod.Invoke` was ignoring the BindingFlags and other arguments. I changed this method to pass along all the arguments. For what it's worth, it appears that [CoreCLR respects](https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Reflection/Emit/DynamicMethod.cs#L488) these arguments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment