Replace public async void with public async Task for XUnit

[MichelZ]

Fixes #2947

[MichelZ]

@dotnet-policy-service agree

[benrr101]

Thanks for both reporting the issue and fixing the issue :D I think the likelihood that we'll upgrade xunit versions anytime soon is sorta slim - iirc there's a dependency issue with a library provided by the dotnet team, that won't work with a newer version. It's been like 7 months since I looked at that, so it might not be a concern anymore. But, regardless, this is the proper way to do async tests, so we should be doing this.

I'll kick off a CI build, and assuming it passes without issue (no reason it shouldn't), we'll get it merged 🚢

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.29%. Comparing base (699f6ab) to head (0fb95ae).
Report is 30 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2948      +/-   ##
==========================================
+ Coverage   72.23%   72.29%   +0.06%     
==========================================
  Files         291      288       -3     
  Lines       59769    59660     -109     
==========================================
- Hits        43172    43131      -41     
+ Misses      16597    16529      -68     

Flag	Coverage Δ
addons	92.58% <ø> (-0.33%)	⬇️
netcore	75.38% <ø> (-0.42%)	⬇️
netfx	70.69% <ø> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[edwardneal]

Thanks for this - I didn't realise we were using async void anywhere.

I can see a bundle of other async void tests when I search for it in the solution. I can submit a PR to your branch with them if you want, unless they were excluded deliberately?

[MichelZ]

Hmm. I upgraded XUnit and it gave Warnings about these. I‘m not sure why it wouldn‘t warn about the others, but of course I/we can change them as well

[MichelZ]

I think the reason the others are not flagged is that they don't use the [Fact] or [Theory] attributes, but the [ConditionalTheory]. The analyzer probably ignores those. I'll update them as well

[MichelZ]

Done.

There are also async void's in the BenchmarkRunners for Benchmark.NET, I'm not too familiar with Benchmark.NET, so this might be on purpose? If not, I can also fix them in a separate PR.

Additionally, there is one async void in SNIPacket.cs here:

SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIPacket.cs

Line 330 in 9d5ca32

public async void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, SNIProviders provider)

Not sure if that needs fixing, too (in another PR)

[MichelZ]

Thanks for both reporting the issue and fixing the issue :D I think the likelihood that we'll upgrade xunit versions anytime soon is sorta slim - iirc there's a dependency issue with a library provided by the dotnet team, that won't work with a newer version. It's been like 7 months since I looked at that, so it might not be a concern anymore. But, regardless, this is the proper way to do async tests, so we should be doing this.

I'll kick off a CI build, and assuming it passes without issue (no reason it shouldn't), we'll get it merged 🚢

You mean this comment?
#2379 (comment)

Wasn't this fixed by upgrading to .NET8 now? (.NET 6 was removed)
Do you think I can add a PR to upgrade to the latest versions?

[edwardneal]

Thanks. I don't think the BenchmarkRunners will make much difference, but it's worth testing with/without.

Additionally, there is one async void in SNIPacket.cs here:

Unfortunately this is a different case. It's intentional - you can see that WriteToStreamAsync invokes the callback itself. WriteToStreamAsync is called by SNITcpHandle.SendAsync, which supplies the handle's send callback. SendAsync effectively starts the task and abandons it until the callback is invoked. It does that because that's how the native SNI layer works.

In theory, I think it'd be possible to move to a Task-first approach; (where the managed SNI layer uses Tasks in line with normal .NET, and the native SNI layer takes on the responsibility for marshalling between its master "write complete" callback and TaskCompletionSources) I think this'd be an improvement and if practical, I'd help with that. In practice, I'd want to do that on a merged and stable TdsParser and TdsParserStateObject layer; it's possible that SqlClientX is a more promising option by the time that the merge/stabilisation is done.

[MichelZ]

Let's see how TdsParser goes...
I'm curious about SqlClientX and what the roadmap/timeframe looks for that

[benrr101]

Do you think I can add a PR to upgrade to the latest versions?

Yknow what, I think you're right. Since we dropped net6 we should be able to take the latest version of Microsoft.DotNet.XUnitExtensions and probably take the latest of XUnit (might even be a requirement for the aforementioned). I'm always a bit hesitant with this codebase, but once this one is checked in, I would support a PR that upgrades XUnit/extensions

I'm curious about SqlClientX and what the roadmap/timeframe looks for that

... same 😅

[benrr101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).