Failed `ServerSession` is not returned to `ConnectionPool`, eventually causing ConnectTimeout

[geieredgar]

Software versions
MySqlConnector version: 2.4.0
Server type (MySQL, MariaDB, Aurora, etc.) and version: MariaDB 10.4
.NET version: net8.0
(Optional) ORM NuGet packages and versions:

Describe the bug
When the tcp connection is broken and MySqlConnection.Open() is called to reconnect to the database, the failed ServerSession is not returned to the ConnectionPool, which eventually leads to a ConnectTimeout.

Exception

MySqlConnector.MySqlException (0x80004005): Connect Timeout expired. All pooled connections are in use.
   at MySqlConnector.MySqlConnection.CreateSessionAsync(ConnectionPool pool, Int64 startingTimestamp, Activity activity, Nullable`1 ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlConnection.cs:line 1109
   at MySqlConnector.MySqlConnection.OpenAsync(Nullable`1 ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlConnection.cs:line 567
   at MySqlConnector.MySqlConnection.Open() in /_/src/MySqlConnector/MySqlConnection.cs:line 522
   at Program.<>c__DisplayClass0_1.<<Main>$>b__1() in Program.cs:line 34

Code sample

using MySqlConnector;

const string connString = "server=localhost;port=3306;user=root;password=pass;Character Set=utf8mb4;";
const string sql = @"SELECT 1;";

using var barrier = new Barrier(52);

for (var i = 0; i < 51; i++)
{
    new Thread(() =>
    {
        using (var conn = new MySqlConnection(connString))
        {
            conn.Open();
            barrier.SignalAndWait(); //Signal main thread that connection is open
            barrier.SignalAndWait(); //Wait for main thread
            try
            {
                using var cmd = new MySqlCommand(sql, conn);
                cmd.ExecuteScalar();
            }
            catch (Exception)
            {
                //Reconnect
                if (conn.State == System.Data.ConnectionState.Closed)
                {
                    try
                    {
                        conn.Open();
                    }
                    catch (Exception ex)
                    {
                        Console.WriteLine(ex);
                    }
                }
                else
                {
                    //The bug is only reproducible if Open() is called, which requires the Closed state.
                    Console.WriteLine($"Connection is in state {conn.State}, expected Closed");
                }

            }
            barrier.SignalAndWait(); //Signal main thread and wait for other threads
        }
    }).Start();
}

barrier.SignalAndWait(); //Wait for other threads to open connections
Console.Write("Reconnect database, then press Enter to continue...");
Console.ReadLine();
barrier.SignalAndWait(); //Signal other threads
barrier.SignalAndWait(); //Wait for other threads to finish

Expected behavior
The sample program exits without an exception.

Additional context
Restarting the database via docker restart is not enough, because that leaves the MySqlConnection in the Open state. To reproduce the bug, I had to use docker network disconnect followed by docker network connect.

My assumption of what happens here:

• ServerSession calls SetFailed due to the broken TCP connection, setting the state of the owning MySqlConnection to Closed without returning the failed ServerSession back to the ConnectionPool.
• MySqlConnection.Open() does not check if m_session is already set and simply overwrites it, causing the ServerSession to "leak", permanently blocking a slot in the ConnectionPool.

[andrewtscan]

I am encountering this in production at the moment (reduced now down from constant failures during the AWS network issues).
To replicate the above example locally I had to use docker restart NOT network connect/disconnect described above. I also replicated via OpenAsync() by adjusting the test harness above.
Pooling=false in connection string resolves as expected as temporary workaround

[bgrainger]

Possibly related to #1198.

[bgrainger]

@geieredgar is this the pattern you use in your production code?

            catch (Exception)
            {
                //Reconnect
                if (conn.State == System.Data.ConnectionState.Closed)
                {
                    try
                    {
                        conn.Open();

The MySqlConnection object is designed to be very lightweight. I would recommend (upon receiving an exception) that you Dispose the failed MySqlConnection object and open a new one. (Rather than trying to re-Open a connection that has become closed.)

I'll leave this issue open to handle the situation that a Closed connection should be able to be Opened, but for now I'd recommend avoiding that pattern.

[bgrainger]

Arguably, this line

MySqlConnector/src/MySqlConnector/Core/ServerSession.cs

Line 1965 in 76f8c51

connection.SetState(ConnectionState.Closed);

should be setting the state to ConnectionState.Broken.

Then your code would need to write:

if (conn.State == ConnectionState.Broken)
	conn.Close();

before reopening a broken connection. (In my testing, this resolves the problem.)

[bgrainger]

This could be a breaking change for some users (although their code is probably already broken in that it leaks connections). It could be mitigated in MySqlConnector by changing

MySqlConnector/src/MySqlConnector/MySqlConnection.cs

Lines 527 to 528 in fd5aa32

	if (State != ConnectionState.Closed)
	throw new InvalidOperationException($"Cannot Open when State is {State}.");

to the following:

		// backwards compatibility for v2.4.0 behavior: previously, a failed connection would be put in 'Closed' state and could
		// be reopened; now it will be 'Broken' so we allow this to be recovered by closing it here first (with changeState:false so
		// it will transition from Broken to Connecting)
		if (State == ConnectionState.Broken)
			await CloseAsync(changeState: false, ioBehavior ?? (m_connectionSettings?.ForceSynchronous is true ? IOBehavior.Synchronous : IOBehavior.Asynchronous)).ConfigureAwait(false);
		else if (State != ConnectionState.Closed)
			throw new InvalidOperationException($"Cannot Open when State is {State}.");

For now, I'm not certain that I want to make that change but might wait for user feedback.