Also, a quick note: we recently went from 3.1.74 to 3.3.74 and I just saw 3.4.3, but I tried all 3 and this issue is present on each one.

Yeah, the async/await support is not the best in SpecFlow. You will find some issues here on GitHub about it.

The exception handling code that throws the BindingException is in BindingInvoker, lines 62 to 67. So far I was able to identify the difference between synchronous and asynchronous code using the tests provided by @btvanhooser:

• When an exception is thrown while invoking a synchronous method it is a TargetInvocationException
• When an exception is thrown while invoking an asynchronous method it is a ArgumentException

I opened #2328 with a fix proposal: Change AsyncHelpers.RunSync exception handling to throw a TargetInvocationException when an exception is thrown while executing the async task. This way exceptions thrown by synchronous and asynchronous executions are the same and can be handled by the same code.

If this fix is reasonable I will proceed with other adjustments in this PR.

I think the AsyncHelpers.RunSync is a general purpose helper to queue async calls and the additional wrapping with TargetInvocationException does not fit well into the responsibility of this helper method.
Probably we could look into the SynchronousBindingDelegateInvoker to see if we can unify the exception handling for the sync and async binding call?

But to be honest I don't fully understand the root cause of this problem yet. What happens to the "original" TargetInvocationException thrown by bindingDelegate.DynamicInvoke(invokeArgs) if this runs within AsyncHelpers.RunSync?

@tzongithub comparing both stack traces I think the root cause is that in the asynchronous code the exception is not thrown when DynamicInvoke is called. The DynamicInvoke simply returns a Task and then the exception is thrown when GetResult is called and the inner workings of both methods is different.

Asynchronous code stack trace:

   at System.Linq.ThrowHelper.ThrowArgumentNullException(ExceptionArgument argument)
   at System.Linq.Enumerable.Where[TSource](IEnumerable`1 source, Func`2 predicate)
   at TechTalk.SpecFlow.Specs.StepDefinitions.AsyncExceptionSteps.<WhenSomeBasicAsyncAction>d__2.MoveNext() in C:\git\SpecFlow\Tests\TechTalk.SpecFlow.Specs\StepDefinitions\AsyncExceptionSteps.cs:line 28
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at TechTalk.SpecFlow.Bindings.SynchronousBindingDelegateInvoker.<>c__DisplayClass2_0.<<InvokeBindingDelegateAsync>b__0>d.MoveNext() in C:\git\SpecFlow\TechTalk.SpecFlow\Bindings\SynchronousBindingDelegateInvoker.cs:line 41

Synchronous code stack trace:

   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Delegate.DynamicInvokeImpl(Object[] args)
   at System.Delegate.DynamicInvoke(Object[] args)
   at TechTalk.SpecFlow.Bindings.SynchronousBindingDelegateInvoker.InvokeBindingDelegateSync(Delegate bindingDelegate, Object[] invokeArgs) in C:\git\SpecFlow\TechTalk.SpecFlow\Bindings\SynchronousBindingDelegateInvoker.cs:line 23

About your suggestion I can either:

1. Move the exception handling code to SynchronousBindingDelegateInvoker.InvokeBindingDelegateAsync, throw a TargetInvocationException in there and let the (unchanged) BindingInvoker handle it;
2. Or handle both scenarios (sync and async) in SynchronousBindingDelegateInvoker to throw a custom exception and the handle it in BindingInvoker (but I think this would be more intrusive);

What's your opinion?

@mdalepiane I debugged the execution and came to the same conclusion: for some reason only the synchronous Delegate.DynamicInvoke call wraps the exception within the delegate method with TargetInvocationException. In case of the async method delegate the framework does not wrap the original exception.

As you pointed out, the exception handling happens in BindingInvoker.InvokeBinding, where the exceptions of type ArgumentException are treated specially by wrapping them into a BindingException with the message "Error calling binding method...". This dates back to the initial commit of SpecFlow here:

• Delegate.DynamicInvoke fails to call the binding method and throws an ArgumentException. This means that there is a problem with the "binding infrastructure" itself, and the method with the "user's code" is not even executed. Hence a BindingException can be thrown.
• Delegate.DynamicInvoke calls the binding method and the "user's code" throws an exception. This exception is wrapped into a TargetInvocationException by the framework that we need to "unpack" to have a better readable test output.

Now the problem is that we cannot distinguish these two cases anymore in case of async. We simply don't have this information based on the type of the exception.

Wrapping the exception into a TargetInvocationException has a few drawbacks:

• We don't know which case from the above two happened. We would always turn the exception into a "user's code" exception and the catch with the ArgumentExeption will be never hit anyway.
• We have another "exception unpacking" for AggregateException. If we wrap an AggregateException into a TargetInvocationException then the 2-level-unpacking won't work.

I'm considering an alternative solution: get rid of the catch case of ArgumentException in https://github.com/SpecFlowOSS/SpecFlow/blob/master/TechTalk.SpecFlow/Bindings/BindingInvoker.cs#L62

• we cannot simply distinguish this case anymore when using async
• since the initial commit the finding of the right delegate and parameter conversions have been improved a lot, it is unlikely that we run into a "binding problem" when calling the delegate
• there were already complaints that the BindingException obfuscates the original error by hiding the stack trace. So it might make more harm than good.

What do you think?
@SabotageAndi any thoughts on this?

Do we have tests for the wanted behavior? If not, we should write them. When they are green, I am happy with any implementation.

@tzongithub I tested with a synchronous method throwing an AggregateException there is no 2-level-unpacking because the AggregateException is wrapped in a TargetInvocationException. If this 2-level-unpacking was expected for synchronous calls I think we have another issue at hand.

I believe the following code in InvokeBindingDelegateAsync would still allow us to distinguish an ArgumentException thrown by a binding failure in Delegate.DynamicInvoke. I believe any binding issue would throw an exception in line 5 that would not be wrapped.

 1   protected virtual object InvokeBindingDelegateAsync(Delegate bindingDelegate, object[] invokeArgs)
 2   {
 3       return AsyncHelpers.RunSync(async () =>
 4       {
 5           var r = bindingDelegate.DynamicInvoke(invokeArgs);
 6           if (r is Task t)
 7           {
 8               try
 9               {
10                   await t;
11               }
12               catch (Exception ex)
13               {
14                   throw new TargetInvocationException(ex);
15               }
16           }
17           return r;
18       });
19   }

Any idea on how I can force an ArgumentException when calling Delegate.DynamicInvoke to check this hypothesis?

@mdalepiane
Good idea, putting the try-catch around the await might indeed work to distinguish the cases in case of async calls.
Unfortunately I don't know either how to force the DynamicInvoke to fail (on its own behalf). I think we would need a case where the arguments do not match the biding method. But if I simply try with mismatching arguments I get conversion errors or BindingException in an earlier stage of execution (TestExecutionEngine.GetExecuteArguments) and I don't get this far in the execution.
It suspect that this case simply cannot happen in an end2end scenario anymore due to the other improvements in the binding selection and argument transformations. That's why I considered removing the ArgumentException case handling in general to simplify the logic.

The AggregateException handling has been added exactly due to async support here, so most probably it was never really relevant for sync cases. Hence my concern about the 2-level-unpacking might be still valid. I think sync cases might be wrapped with TargetInvocationException while async cases might be wrapped with AggregateException and hence the unpacking works right now.
A potential solution for this could be if you would wrap the exception into an AggregateException, and only if it was not an AggregateException already. The TargetInvocationException we would leave for the sync cases only.

Due to time constraints we will try to look into this later in the OSS Iteration 2