ExecutionContext does not flow between hooks or steps in latest beta

[JoshKeegan]

SpecFlow Version

3.10.2-beta

Which test runner are you using?

xUnit

Test Runner Version Number

2.4.1

.NET Implementation

.NET 6.0

Project Format of the SpecFlow project

Sdk-style project format

.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

The changes in the latest beta that switch from steps & hooks being run synchronously by default and async ones being wrapped by AsyncHelpers.RunSync to running async natively introduces a breaking change that affects any test that depends on the ExecutionContext, e.g. by using AsyncLocal<T> or HttpContext.Current. This is because when a Task is awaited the execution context does not get copied back from within the Task. So if a hook or step sets a value on the execution context, later steps or hooks won't be able to access it because the execution context it was stored on is now lost.

In previous (or current stable) versions of Specflow this worked because as long as your hook or step that was storing something on the execution context was synchronous then it would be modifying the the same execution context that would get passed to later steps/hooks. If a later step or hook was async, then it would still be able to read a value because in dotnet the execution context gets copied into a Task when it is created, just any changes are not copied back to the parent when it is awaited.

Steps to Reproduce

Very simple repro project with AsyncLocal<T>: SpecFlowExecutionContextNoFlow.zip

Link to Repro Project

No response

[JoshKeegan]

Related to the changes of #2582 @gasparnagy

[gasparnagy]

@JoshKeegan I have started to analyze this issue and it feels that there is definitely some problems with the implementation, but I am not sure really what the expected result should be. A long comment comes...

I'm not a super expert in Execution Context (EC), but what I have learned and experienced was that normally the EC only flows down, but not up. So for example the following code fails (xUnit test).

private static readonly AsyncLocal<string> _al = new AsyncLocal<string>();

[Fact]
public async Task Test1()
{
    await M1();
    await M2();
}

private async Task M1()
{
    _al.Value = "42"; // this value will not flow up and get to M2
}

private async Task M2()
{
    Assert.Equal("42", _al.Value); // will be <null>
}

So based on that, I would say that setting anything in to an EC (via AsyncLocal) in an async step definition is not valid, as the step def is a "leaf". (In the example above, M1 and M2 are in the same role as step definitions in SpecFlow.)

If I change M1 to a normal sync method, the test will pass, because sync methods use the same EC so basically setting the value sets it in the "root" EC that will flow to M2. Same happens if the value is initialized in the constructor.

There is a strange behavior if you initialize the value in the static constructor and than call M1 async (that normally should not have any impact, because EC does not flow up) then the value is reset and it holds <null> and not the initialized value. This can be reproduced both with xUnit and a Console app, but I don't understand.

I have extended your repro and published it here: https://github.com/gasparnagy/issue-repro-specflow-2600

An additional complexity is that for SpecFlow you can use both async Task and async void step definitions, and they seem to work differently.

So based on that, I think the behavior should be:

The result should be independent of whether you read it from a sync and async or an async void step definitions, but the realty is quite different...

I have marked the "wrong" results with bold.

Case	Expected	SpecFlow v3	SpecFlow v4 beta
1. set AL in sync stepdef, read it in sync/async/async-void step def	pass: the value is available	pass	fail
2. set AL in ctor, read it in sync/async/async-void step def	pass: the value is available	pass	pass
3. set AL in static ctor, read it in sync/async/async-void step def	pass: the value is available	pass	pass
4. set AL in async step def, read it in sync/async/async-void step def	fail: the value is null	fail	fail
5. set AL in async void step def, read it in sync/async/async-void step def	fail: the value is null	fail	fail

[Update 1]: It turned out that my tests for Case 3 (Initialize AL in static ctor) were wrong. This is not a realistic case, because the value is only reliable in the first scenario.

If I assume that the "Expected" behavior is correct as I have found out, we can say:

• SpecFlow v3 was correct.
• The async void structure is anyway not recommended by .NET, so the problems related to that are not that severe. Maybe we should even not allow these. [Update 2]: It seems that exceptions of async void step definitions are swallowed, so the "pass" result in case 1, 3, 5 was a fake result. In reality they fail, just we don't see the error. created separate issue: #2657
• If we ignore the async void structure, the problematic case is case 1 (this was your initial problem anyway) -- this is what we would need to fix probably

I would appreciate if you could review my observations and let me know if you agree to my conclusions.

//cc: @SabotageAndi

[gasparnagy]

I think I got to a better understanding of the problem and found a fix that is implemented in #2658. Please review that and add your comments.

[JoshKeegan]

I've taken a look at the changes and it seems like a good solution. I was expecting this to need a much larger change to create two separate paths for async & sync bindings, so managing to keep it all in a single code path is nice (even if it creates an area with some subtle complexities). I like the idea of using the IoC container to store the context, very elegant and not something I've seen done before.

I suppose my only question is: has it been tried in a real solution, like the repro I posted? It looks good & the unit tests are fine but it'd be good to just check it end to end.

[JoshKeegan]

Also just re-read your initial response. Not sure how much you've looked into it but async void is interesting in test runners. If I remember correctly xunit has a custom SynchronizationContext so that they can run these, track them running and wait for them to finish. It might be worth looking at that for some inspiration on solving your async void problem which appears to be the same.

[japj]

Hi, I also ran into the issues that #2582 solves (in 3.10.2-beta)

I was wondering, is there an official 3.10 release planned (after this specific issue is resolved)?

[gasparnagy]

@japj There will be no v3.10, we have "renamed" it to v4.0. We are very close to release it - I hope within a few weeks.

[gasparnagy]

@JoshKeegan Yes, I have tried with your sample and it worked.

About "async void" -- for now we decided to not allow it (#2662). Let's see if someone defines a concrete use-case and then we can re-think. (For test runners the problem is different, because there is no dependent action on the execution of the test, but for us a step has to be followed by another one. And if one is not awaitable (like async void is), it is not easy to keep the sequence.)

[japj]

@japj There will be no v3.10, we have "renamed" it to v4.0. We are very close to release it - I hope within a few weeks.

Thanks for clarifying, great to hear!

I guess this also means we should try the latest 4.0 beta instead of the 3.10 one then?

[JoshKeegan]

@gasparnagy good to know it works with that repro, thanks 👍

RE async void: not allowing it is a good idea, I can't think of a use case for it in Specflow. It might just trip a few people up on the v4 upgrade.

If a use-case does appear though, I'd still recommend looking at xunit. Since tests within the same class or collection are executed sequentially (creating a dependant action) they effectively emulate await via a sync context. Here's the class I'm referring to: https://github.com/xunit/xunit/blob/main/src/xunit.v3.core/Sdk/AsyncTestSyncContext.cs

[gasparnagy]

I guess this also means we should try the latest 4.0 beta instead of the 3.10 one then?

@japj Yes, exactly.

[github-actions[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.