Aggregate exceptions in InvokeBindingAsync replaced with first inner exception

[rossmasday]

SpecFlow Version

3.9.74

Which test runner are you using?

NUnit

Test Runner Version Number

3.13.3

.NET Implementation

.NET 6.0

Project Format of the SpecFlow project

Classic project format using <PackageReference> tags

.feature.cs files are generated using

SpecFlow.Tools.MsBuild.Generation NuGet package

Test Execution Method

Visual Studio Test Explorer

SpecFlow Section in app.config or content of specflow.json

No response

Issue Description

InvokeBindingAsync catches and re-throws the inner exception of aggregate exceptions so error information is lost when aggregate exceptions are thrown in user code https://github.com/SpecFlowOSS/SpecFlow/blob/8b7eb2e9c35c80b7c5f2c525f6b44019d421b669/TechTalk.SpecFlow/Bindings/BindingInvoker.cs#:~:text=var%20ex%20%3D%20aggregateEx.InnerExceptions.First()%3B

Steps to Reproduce

Throw an aggregate exception within step method
One of the following will happen, depending on whether there is an inner exception or not:

If it has an inner exception, the aggregate exception details are lost (no message, line number, stack trace etc...) and only the first inner exception is re-thrown.
If it does not have an inner exception, line shown causes an "System.InvalidOperationException : Sequence contains no elements" to show.

Link to Repro Project

No response

[SabotageAndi]

Please try out a SpecFlow 4.0 beta, because we changed the whole async/await support.

[rossmasday]

Hi @SabotageAndi the same code and issue exists in the v4 beta branch too, line 83 catch : https://github.com/SpecFlowOSS/SpecFlow/blob/v4.0.7-beta/TechTalk.SpecFlow/Bindings/BindingInvoker.cs#:~:text=catch%20(AggregateException,%7D

[clrudolphi]

For clarification of my understanding, what behavior should be expected? As I understand them, AggregateExceptions are carriers of the collection of Exceptions that occurred. They are not (usually) expected to contain a Message or Stacktrace of their own.

The lack of a check for an empty InnerExceptions enumerable is an oversight and should be corrected. I'm happy to pick up that task.

But assuming that the AE does contain inner exceptions, would the stacktrace and details of the first exception be sufficient?

[SabotageAndi]

@gasparnagy can you have a look at this?

[rossmasday]

It is possible for aggregate exceptions to carry a message, even if it is not usual, it is also possible for aggregate
exceptions to carry more than one inner exception. The behaviour I would expect is for the aggregate exception to be re-thrown as it was (with the message, stack trace and all inner exceptions and their details preserved) for the test runner to catch and display correctly, and not to have just the first inner exception re-thrown instead, in other words for specflow not to catch and manipulate the exception.

[gasparnagy]

@rossmasday @clrudolphi I tend to agree to @rossmasday, let's rethrow the AE as it is. But...

It seems that the AE catch was introduced at the time when we introduced the basic support for async step definitions. Now we have a proper async support, but we would need to test how error messages look like if there is a normal (not aggregate) exception is thrown from an async step definition.

Also I am a bit confused why we have the PreserveStackTrace call in the AE case, but not in the generic Exception catch block. (Maybe that generic block was never hit so far?)

So if @clrudolphi you would be up to pick up the task, please do the following:

1. Remove the AE catch block
2. Try it (manually with Visual Studio Test Runner) if the error messages and the stack trace looks good for

• Normal Exception (maybe with inner exception) in a non-async step def
• AE in a non-async step def
• Normal Exception (maybe with inner exception) in an async step def
• AE in a async step def

3. If all of those look good -> let's do it
4. If any of the manual tests show some strange behavior (and cannot be fixed by adding the PreserveStackTrace call to the generic block), let's discuss it again

[clrudolphi]

Yes, I will pick up the task.

[gasparnagy]

@clrudolphi Super. Please let me know if you get stuck or have questions.

[clrudolphi]

I have drafted a set of scenarios and bindings to simulate the conditions we've discussed (and added a scenario to demonstrate an AE that does not include an inner exception). Please review the project file at:
https://github.com/clrudolphi/SpecFlow_SF2649_ExceptionHandling

In the project folder of that solution are two text files that are the dotnet test logs of these scenarios before and after removing the catch(AggregateException) clause within the BindingInvoker.

The proposed change is in a branch of my github clone of SF at:
https://github.com/clrudolphi/SpecFlow/blob/SF2649/TechTalk.SpecFlow/Bindings/BindingInvoker.cs

@gasparnagy , @rossmasday : Please review the scenarios and how I implemented them. I want to ensure that these are adequate before I submit a pull request.

Chris

[gasparnagy]

@clrudolphi Nice work! I think the expected results look good, so we should go ahead with this change.

Two comments:

1. Because of some other change, I had to create some test infrastructure to be able to unit-test the BindingInvoker. It is able to run sync and async methods as well, so I think we could easily add unit tests for this functionality (in the assertion I would verify if the exception message and the name of the step definition method is included). I have created a small PR with this test infrastructure together with a sample test (see here). I will merge it as soon as the checks are done, so hopefully early afternoon. Would you try to "convert" the tests from the sample project to unit tests with that?
2. Looking at the stack trace, my feeling is that the PreserveStackTrace hack I did many years ago is not working. Although I don't remember fully, but I think that earlier I was able to eliminate the TestRunner.CollectScenarioErrorsAsync() call from the stack trace and show the original stack trace where the exception was thrown. (SpecFlow saves the exception to be able to check and report the steps after the failing one and throws it from the end of the scenario method.) Maybe we have lost this with .NET Core... Anyway, I have just learned about the magical ExceptionDispatchInfo class and based on the description, I think the ExceptionDispatchInfo.Throw static method is just doing what we want (never tried, but this is used in the .NET async infrastructure for the same purpose). If you have even more time, could you please also try how the stack trace would change if we would use this method to re-throw the exceptions in the TestRunner.CollectScenarioErrorsAsync()? Also if the PreserveStackTrace has any impact still... But we can do that in a separate PR, because thats another problem anyway.

[rossmasday]

Looks good, just one point about the inner exceptions, rather than creating new instances and storing them, it would be more realistic, and help to ensure all data is there, if the inner exceptions were actually thrown, caught and passed in the throw of the outer exceptions, that would demonstrate that the full details, stack trace and all are showing as expected, for example in the Thrower code:
try { throw new Exception("This Exception embedded in the AggregateException"); } Catch (Exception e) { throw new AggregateException("AggregateEx message (with Inner Exception)", e); }

[clrudolphi]

Yes, good point. I'll do that when I convert this to the unit test format suggested by @gasparnagy .

[clrudolphi]

I have a branch on my fork of SF with the changes we discussed. Take a look at:
BindingInvoker uses ExceptionDispatchInfo when rethrowing a captured exception.
https://github.com/clrudolphi/SpecFlow/blob/SF2649/TechTalk.SpecFlow/Bindings/BindingInvoker.cs

The tests for the combinations of exception situations, rewritten as a Unit Test:
https://github.com/clrudolphi/SpecFlow/blob/SF2649/Tests/TechTalk.SpecFlow.RuntimeTests/Bindings/BindingInvokerTests.cs

If this behaves as you both wish/expect, then a small cosmetic cleanup is required of the BindingInvoker (to remove the lines I commented out).
Once I do that, I'll submit a PR.

[gasparnagy]

Looks good. I also like the tests.

I wonder if we could also assert the impact of the ExceptionDispatchInfo.Throw... Maybe with asserting that the thrown.StackTrace contains the stepdef method name as string. (But would need to check if it fails is the ExceptionDispatchInfo.Throw is commented out.)

Technical side-note: when you put a link to a GitHub comment, you have to have the text in the [] and the URL in the (), so e.g. [link text](https://github.com) not reverse. (I have fixed these links in your comments.) But for simple links I usually just paste the link without any decoration.

[rossmasday]

Looks good, is it worth adding a scenario with multiple inner exceptions to ensure that it never regresses to just showing the first inner exception again?

[clrudolphi]

Both suggestions accepted and implemented.
@gasparnagy - I tried both mechanisms (PreserveStackTrace and ExceptionDispatchInfo). Both resulted in the same stack trace content. I included the use of the ExceptionDispatchInfo as that seemed the more modern way to do it.

The unit tests now confirm that the traces contain the method name in the trace (somewhere, but not asserting any specific placement within the trace). Due to differences in how the async mechanism works between .NET 6 and 462, I could not assert on the fully qualified Type.MethodName and had to fall back to simply confirming that the method name was present.

(my git history of the private branch I was using got fouled, so I started over with a new branch)
Please review the changes on this branch: AggregateExceptions_lost_from_async_StepDefs_2649

If all looks good, I'll submit a PR.

[gasparnagy]

@clrudolphi cool. go ahead with the PR!

[gasparnagy]

@clrudolphi Could you please also replace the line if (hookException != null) throw hookException; here with

if (hookException != null) ExceptionDispatchInfo.Capture(hookException).Throw();

in your PR?

I have tested that one locally and it works, but I don't want to mix that in to the PR I'm working on.

[clrudolphi]

@gasparnagy - TestExecutionEngine updated.

PR submitted