Fix S6966 FP: Should not raise for Microsoft.data.SqlClient

[LudovicT]

Description

Rule Id: S6966
Microsoft.data.SqlClient currently offer Async methods for almost all their API, but these Async methods suffer from "hidden" performance hit.
The rule should not raise for the current SqlClient as it might be a conscious choice to not use the Async method due to performance.

Once SqlClientX is release the rule should be applied to it, more details about SqlClientX and the Async issue can be found here:
dotnet/SqlClient#2603
dotnet/SqlClient#2612

Repro steps

private static async Task CreateCommand(string queryString, string connectionString)
{
    using (SqlConnection connection = new SqlConnection(connectionString))
    {
        connection.Open(); // S6966: Await OpenAsync instead

        SqlCommand command = new SqlCommand(queryString, connection);
        using(SqlDataReader reader = command.ExecuteReader()) // S6966: Await ExecuteReaderAsync instead
        {
            while (reader.Read()) // S6966: Await ReadAsync instead
            {
                Console.WriteLine(String.Format("{0}", reader[0]));
            }
        }
    }
    await Task.Delay(1); // Present to make the method async
}

Expected behavior

S6966 should not raise for Microsoft.Data.Client methods

Actual behavior

S6966 raise for Microsoft.Data.Client methods

Known workarounds

1. Disable the rule -> also disables the check for all other methods
2. Wrap the calls in an extension that ignore the warning -> more boilerplate to maintain
3. Mark as FP in SonarQube -> time consuming when you have a number of calls

Related information

• C#/VB.NET Plugins version: 9.27.0.93347
• Visual Studio version: VS 2022, the issue show up mostly when using Sonar during our pipeline build
• MSBuild / dotnet version: 17.8.3+195e7f5a3/ NET 8.0.100
• SonarScanner for .NET version: 5.15.1
• Operating System: Win11 / Windows Server 2019

[CristianAmbrosini]

Hi @LudovicT !
I would consider this an edge case. From my understanding, there are scenarios where asynchronous operations can be slower, while in other cases, they may not be.

Introducing an exception, as you suggested, could potentially disable the rule for Entity Framework as well since it might utilize the SQL package under the hood. I'm afraid that it will do more damage than good.
If you have reliable benchmarking data indicating that the asynchronous version performs worse in your specific case, you can disable the rule by using the pragma directive for each affected query.

[LudovicT]

Hi @CristianAmbrosini!

While I agree that this is not always slower, it is important to note that this is a real concern that might not be obvious at first glance (unless you properly read the documentation).

Since you mention Entity Framework, do note that they have warnings in various pages of their documentation regarding the usage of async with Microsoft.Data.SqlClient:

https://learn.microsoft.com/en-us/ef/core/miscellaneous/async
https://learn.microsoft.com/en-us/ef/core/providers/sql-server#install
https://learn.microsoft.com/en-us/ef/core/performance/efficient-querying#asynchronous-programming
https://learn.microsoft.com/en-us/aspnet/core/data/ef-rp/intro#asynchronous-ef-methods-in-aspnet-core-web-apps

Regarding benchmarking you can refer to these issues:
dotnet/SqlClient#601
dotnet/SqlClient#593

Most of the issues stems from opening connections (very common) and reading large amount of data (less common).
For connections, if you max out the connection pool (default of 100), you can start having performances issues when you open new connections asynchronously. A maxed out connection pool is easily reached if your website/API has some user engagement.
For reading large data this is especially impactful, async can be slower than sync by multiple order of magnitudes as the size increase. (x100 and more for 10MB+).

It is unfortunate that following a suggestion from SonarQube may lower your software reliability.