Perf problem on existence checks caused by SqlClient's connection resiliency feature

[Inspyro]

When using the same DbContext class (e.g. DbContext itself) with different connection strings, the Connection Resiliency Feature of .Net 4.5 causes DB-creation and deletion (EnsureCreated/EnsureDeleted) to take a long time (due to the DB-Existency checks).

I made a small repro sample where you can see the different effects.
Basically when using 2 default connection strings (without modified connection resiliency settings) with the same DbContext class, the DB-Creation takes >10s (as this seems to be the default ConnectRetryInterval, even when the MSDN docs suggest something else).

Repro sample

static void Main (string[] args)
{
    // Default resiliency settings (since .NET 4.5)
    var defaultConnectionString = "Data Source=localhost;Initial Catalog=EFIdleResiliencyBug1;Integrated Security=SSPI";

    // Retry Interval = 3s (instead of 10 which seems to be the default)
    var connectionStringThreeSeconds = "Data Source=localhost;Initial Catalog=EFIdleResiliencyBug2;Integrated Security=SSPI;ConnectRetryInterval=3";

    // Retry Count = 0 (instead of 1) => means Resiliency Feature is disabled.
    var connectionStringZeroRetry = "Data Source=localhost;Initial Catalog=EFIdleResiliencyBug3;Integrated Security=SSPI;ConnectRetryCount=0;";

    // Here we also create a default connection string but with a different db
    var defaultConnectionString2 = "Data Source=localhost;Initial Catalog=EFIdleResiliencyBug4;Integrated Security=SSPI";

    Drop(defaultConnectionString); // takes > 10s
    Drop(connectionStringZeroRetry); // Takes no time (connection resiliency disabled)
    Drop(connectionStringThreeSeconds); // takes > 3s
    Drop(defaultConnectionString2); // takes > 10s

    //All connection strings (same DbContext class) try to connect to the DB.
    Create(defaultConnectionString); // BUG: Takes 10s
    Create(connectionStringThreeSeconds); // BUG: Takes 3s
    Create(connectionStringZeroRetry); // Takes no time (connection resiliency disabled)
    Create(defaultConnectionString2); // BUG: Takes 10s (seems to be the default RetryInterval for some reason).

    // Note: When using a different derived class from DBContext for each connection string the bug does not exist.

    // Expected Behavior: Ignore connection resiliency for checking if the db exists. (It should not take 3s or even 10s).
    // This is especially a problem when executing integration / unit tests that drop the db at the beginning.
}

private static void Create(string connectionString)
{
    Console.WriteLine($"Creating {connectionString}");
    var sw = Stopwatch.StartNew();

    using (var dbContext = new TestDbContext(connectionString))
    {
        dbContext.Database.EnsureCreated();
        Console.WriteLine("Creating DB took: " + sw.Elapsed);
    }
}

private static void Drop(string connectionString)
{
    Console.WriteLine($"Droping {connectionString}");
    var sw = Stopwatch.StartNew();

    using (var dbContext = new TestDbContext(connectionString))
    {
        sw = Stopwatch.StartNew();
        dbContext.Database.EnsureDeleted();

        Console.WriteLine("Deleting DB took: " + sw.Elapsed);
    }
}

public class TestDbContext : DbContext
{
    private readonly string _connectionString;
    public TestDbContext(string connectionString)
    {
        _connectionString=connectionString;
    }
    
    protected override void OnConfiguring(DbContextOptionsBuilder options)
    {
        options.UseSqlServer(_connectionString);
    }
}

I hope this helps you fix the API or adapt the docs.

But in my opinion the current API (EF Core) suggests that you can use the same class (DBContext) for multiple connection strings (ctor for this). However with .Net 4.5 that causes serious performance problems.
This is especially noticable in our unit / integration tests where we drop multiple DBs with the same DBContext class and recreate them later.

Note: This issue was ported from EF 6 (http://entityframework.codeplex.com/workitem/2899) and re-tested with the EF.Core 1.1.0 (nuget package)

[divega]

Expected Behavior: Ignore connection resiliency for checking if the db exists. (It should not take 3s or even 10s).

@Inspyro Is the sentence above a good summary of your request or are there other issues I missed?

As mentioned at #6673 (comment) we have already discussed this issue and decided against trying to work around the new behavior of SqlClient.

Creating a separate connection string for existence checks has been problematic in the past because there isn't a uniform way to create a new connection that will preserve credentials when you use SQL Server authentication: In .NET Framework SqlConnection.Clone() can possibly work but in .NET Core we would need to create a new SqlConection object with a copy of the connection string having ConnectRetryCount=0 appended to it and it is possible that the user's password would have already been removed from that connection string.

I will try to do some basic testing to see if this approach works before we discuss this again in triage. I will also file a bug on SqlClient to get Clone() in .NET Core.

[divega]

I did some more investigation and here is what I learned:

1. I confirmed the behavior in .NET Framework and .NET Core is the same regarding removing the password from connection string once the connection is opened (although I am sure the implementation is different because in .NET Core SqlClient does not seem to rely on SecureString). Note that the password commonly needs to be specified in the connection string in platforms where integrated security is not supported (i.e. anywhere but in Windows).

2. In .NET Framework SqlConnection implements ICloneable.Clone() explicitly, so the probability of having a uniform way to clone the connection without losing credentials in the short term is small.

3. There doesn't seem to be a public way to mutate ConnectRetryCount on SqlConnection other than mutating ConnectionString. Also, mutating ConnectionString actually resets any password stored internally in the SqlConnection object, i.e. the following code will fail at the last line with the message Login failed for user 'diego'.:

var connection = new SqlConnection(
    @"Server=(localdb)\mssqllocaldb;Database=NewOne;Integrated Security=False;User id=diego;Password=&123Blah");
connection.Open();
connection.Close();
var connection2 = (SqlConnection)((ICloneable)connection).Clone();
connection2.ConnectionString += ";ConnectRetryCount=0";
connection2.Open();

Given all of this I re-discussed the issue with @ajcvickers who had done some investigation around it before and we both came to the conclusion that there isn't anything reasonable we can do on EF Core to address this problem in all cases. There is something we could do that would work in some cases though:

Store aside the connection string passed to us in UseSqlServer() so that we can modify it with ConnectRetryCount=0 (or at least ConnectRetryInterval=1) to create a separate connection object with the purpose of checking for database existence.

Note that this will fail if a SqlConnection object on which the password has already been removed from ConnectionString property (i.e. that was already opened) is passed to UseSqlServer().

We are now debating whether implementing this workaround in EF Core would actually be much better than making customers aware of the fact that SqlClient assumes ConnectRetryCount=1; ConnectRetryInterval=10 by default, i.e. by having our database creation methods issue a warning recommending to override these values so that EF Core can much better take care of connection retries with its own logic (e.g. see how the implementation of existence checks in our SQL Server provider, which leverages knowledge of whether the database has just been created to implement connection retries more efficiently https://github.com/aspnet/EntityFramework/blob/dev/src/Microsoft.EntityFrameworkCore.SqlServer/Storage/Internal/SqlServerDatabaseCreator.cs#L118).

In the end we suspect this whole issue could much better addressed if SqlClient exposed a way to temporarily override their connection resiliency feature.

[Inspyro]

Is the sentence above a good summary of your request or are there other issues I missed?

@divega Yes this sentence sums the issue up nicely
And btw thanks for the fast response

In our case, we mainly are concerned with the performance of our unit tests, thus we had to edit all connection strings to disable connection resiliency. So for now docs are sufficient. However I would expect the .NET / EF Core Api to expose this setting to the user and probably disable resiliency by default for the "EnsureCreated / Deleted" methods, as it could easily lead to performance problems, that are hard to detect. So "temporarily overriding the connection resiliency feature" could be a nice fix, as you suggested.

[ErikEJ]

@divega Would it be an option to connect to master for the Exists check, instead of brute force opening an connection, then look in sys.databases?

 SELECT collation_name FROM sys.databases WHERE state = 0 AND name = <User DB>

See https://msdn.microsoft.com/en-us/library/ms178534.aspx?f=255&MSPPError=-2147217396

[divega]

@ErikEJ we stopped doing that because it didn't always work, i.e. it requires you having access to master, which may not be the case for a couple of different reasons.

For the scenarios in which it works it also requires manipulation of the connection to connect to master instead of the target database, which normally we would do in a separate connection object, so it posses the same challenges re obtaining an original connection string that is still complete and contains a password.

[ErikEJ]

Looks like Azure was the deciding factor in not making ConnectRetryCount default to 0 to prevent breaking changes :sad:

[divega]

Looks like Azure was the deciding factor ...

@ErikEJ do you have some pointers to that?

[ErikEJ]

Yes this: https://blogs.msdn.microsoft.com/dotnet/2015/11/30/net-framework-4-6-1-is-now-available/ - but ConnectRetryCount was introduced before that, I realize now: https://msdn.microsoft.com/en-us/library/dn632678.aspx - and looking at the Word doc from the second link, it looks like "ConnectRetryCount=0" was considered the way of opting out.

[divega]

@Inspyro I have created https://github.com/dotnet/corefx/issues/14644 to cover what we believe is the best solution for this issue: have SqlClient expose a way to programmatically disable connection resiliency that we can leverage when checking for database existence. Feel free to up-vote it if you think it is a good idea.

[ErikEJ]

@divega sadly, that will not solve the issue for .net 4.5.1 to 4.6.x

[ajcvickers]

Bringing this back to triage since SqlClient now have a way for us to opt out--see dotnet/SqlClient#29 (comment)

[ajcvickers]

We now have the new SqlClient dependency, so this can now be implemented. #20805

[ajcvickers]

With recent versions of SqlClient I'm seeing the old behavior again. I need to use ConnectRetryCount=0 to stop SqlClient hanging.

[ajcvickers]

Re-opened the SqlClient issue: dotnet/SqlClient#29

[ajcvickers]

Tracking regression with #33399