Remove strong references. Fixes #306

The code to recover leaked sessions depends on MySqlConnection being GCed. If MySqlSession holds strong references to MySqlConnection (via MySqlCommand or MySqlDataReader), then failure to dispose a MySqlDataReader will leak the connection.

Author: bgrainger

src/MySqlConnector/MySqlClient/MySqlCommand.cs

@@ -32,6 +32,7 @@ public MySqlCommand(string commandText, MySqlConnection connection)
 
 		public MySqlCommand(string commandText, MySqlConnection connection, MySqlTransaction transaction)
 		{
+			CommandId = Interlocked.Increment(ref s_commandId);
 			CommandText = commandText;
 			DbConnection = connection;
 			DbTransaction = transaction;
@@ -166,6 +167,8 @@ internal IDisposable RegisterCancel(CancellationToken token)
 			return token.Register(m_cancelAction);
 		}
 
+		internal int CommandId { get; }
+
 		private void VerifyNotDisposed()
 		{
 			if (m_parameterCollection == null)
@@ -190,6 +193,8 @@ private bool IsValid(out Exception exception)
 
 		internal void ReaderClosed() => (m_commandExecutor as StoredProcedureCommandExecutor)?.SetParams();
 
+		static int s_commandId = 1;
+
 		MySqlParameterCollection m_parameterCollection;
 		CommandType m_commandType;
 		ICommandExecutor m_commandExecutor;

src/MySqlConnector/MySqlClient/MySqlConnection.cs

@@ -290,14 +290,28 @@ internal async Task<CachedProcedure> GetCachedProcedure(IOBehavior ioBehavior, s
 		}
 
 		internal MySqlTransaction CurrentTransaction { get; set; }
-		internal MySqlDataReader ActiveReader => m_session.ActiveReader;
 		internal bool AllowUserVariables => m_connectionSettings.AllowUserVariables;
 		internal bool BufferResultSets => m_connectionSettings.BufferResultSets;
 		internal bool ConvertZeroDateTime => m_connectionSettings.ConvertZeroDateTime;
 		internal bool OldGuids => m_connectionSettings.OldGuids;
 		internal bool TreatTinyAsBoolean => m_connectionSettings.TreatTinyAsBoolean;
 		internal IOBehavior AsyncIOBehavior => m_connectionSettings.ForceSynchronous ? IOBehavior.Synchronous : IOBehavior.Asynchronous;
 
+		internal void SetActiveReader(MySqlDataReader dataReader)
+		{
+			if (dataReader == null)
+				throw new ArgumentNullException(nameof(dataReader));
+			if (m_activeReader != null)
+				throw new InvalidOperationException("Can't replace active reader.");
+			m_activeReader = dataReader;
+		}
+
+		internal void FinishQuerying()
+		{
+			m_session.FinishQuerying();
+			m_activeReader = null;
+		}
+
 		private async Task<MySqlSession> CreateSessionAsync(IOBehavior ioBehavior, CancellationToken cancellationToken)
 		{
 			var connectTimeout = m_connectionSettings.ConnectionTimeout == 0 ? Timeout.InfiniteTimeSpan : TimeSpan.FromSeconds(checked((int) m_connectionSettings.ConnectionTimeout));
@@ -382,7 +396,7 @@ private void DoClose()
 		private void CloseDatabase()
 		{
 			m_cachedProcedures = null;
-			Session?.ActiveReader?.Dispose();
+			m_activeReader?.Dispose();
 			if (CurrentTransaction != null && m_session.IsConnected)
 			{
 				CurrentTransaction.Dispose();
@@ -398,5 +412,6 @@ private void CloseDatabase()
 		bool m_isDisposed;
 		bool m_shouldCloseWhenUnenlisted;
 		Dictionary<string, CachedProcedure> m_cachedProcedures;
+		MySqlDataReader m_activeReader;
 	}
 }

src/MySqlConnector/MySqlClient/MySqlDataReader.cs

@@ -227,7 +227,7 @@ protected override void Dispose(bool disposing)
 		internal static async Task<MySqlDataReader> CreateAsync(MySqlCommand command, CommandBehavior behavior, IOBehavior ioBehavior)
 		{
 			var dataReader = new MySqlDataReader(command, behavior);
-			command.Connection.Session.SetActiveReader(dataReader);
+			command.Connection.SetActiveReader(dataReader);
 
 			try
 			{
@@ -248,7 +248,7 @@ internal static async Task<MySqlDataReader> CreateAsync(MySqlCommand command, Co
 			finally
 			{
 				if (command.Connection.BufferResultSets)
-					command.Connection.Session.FinishQuerying();
+					command.Connection.FinishQuerying();
 			}
 		}
 
@@ -289,7 +289,7 @@ private void DoClose()
 
 				var connection = Command.Connection;
 				if (!connection.BufferResultSets)
-					connection.Session.FinishQuerying();
+					connection.FinishQuerying();
 
 				Command.ReaderClosed();
 				if ((m_behavior & CommandBehavior.CloseConnection) != 0)

src/MySqlConnector/Serialization/MySqlSession.cs

@@ -66,7 +66,7 @@ public bool TryStartCancel(MySqlCommand command)
 		{
 			lock (m_lock)
 			{
-				if (m_activeCommand != command)
+				if (m_activeCommandId != command.CommandId)
 					return false;
 				VerifyState(State.Querying, State.CancelingQuery);
 				if (m_state != State.Querying)
@@ -81,12 +81,12 @@ public void DoCancel(MySqlCommand commandToCancel, MySqlCommand killCommand)
 		{
 			lock (m_lock)
 			{
-				if (m_activeCommand != commandToCancel)
+				if (m_activeCommandId != commandToCancel.CommandId)
 					return;
 
 				// NOTE: This command is executed while holding the lock to prevent race conditions during asynchronous cancellation.
-				// For example, if the lock weren't held, the current command could finish and the other thread could set m_activeCommand
-				// to null, then start executing a new command. By the time this "KILL QUERY" command reached the server, the wrong
+				// For example, if the lock weren't held, the current command could finish and the other thread could set m_activeCommandId
+				// to zero, then start executing a new command. By the time this "KILL QUERY" command reached the server, the wrong
 				// command would be killed (because "KILL QUERY" specifies the connection whose command should be killed, not
 				// a unique identifier of the command itself). As a mitigation, we set the CommandTimeout to a low value to avoid
 				// blocking the other thread for an extended duration.
@@ -99,7 +99,7 @@ public void AbortCancel(MySqlCommand command)
 		{
 			lock (m_lock)
 			{
-				if (m_activeCommand == command && m_state == State.CancelingQuery)
+				if (m_activeCommandId == command.CommandId && m_state == State.CancelingQuery)
 					m_state = State.Querying;
 			}
 		}
@@ -113,22 +113,10 @@ public void StartQuerying(MySqlCommand command)
 
 				VerifyState(State.Connected);
 				m_state = State.Querying;
-				m_activeCommand = command;
+				m_activeCommandId = command.CommandId;
 			}
 		}
 
-		public MySqlDataReader ActiveReader => m_activeReader;
-
-		public void SetActiveReader(MySqlDataReader dataReader)
-		{
-			VerifyState(State.Querying, State.CancelingQuery);
-			if (dataReader == null)
-				throw new ArgumentNullException(nameof(dataReader));
-			if (m_activeReader != null)
-				throw new InvalidOperationException("Can't replace active reader.");
-			m_activeReader = dataReader;
-		}
-
 		public void FinishQuerying()
 		{
 			bool clearConnection = false;
@@ -156,8 +144,7 @@ public void FinishQuerying()
 			{
 				VerifyState(State.Querying, State.ClearingPendingCancellation);
 				m_state = State.Connected;
-				m_activeReader = null;
-				m_activeCommand = null;
+				m_activeCommandId = 0;
 			}
 		}
 
@@ -879,8 +866,7 @@ private enum State
 		IDisposable m_serverCertificate;
 #endif
 		IPayloadHandler m_payloadHandler;
-		MySqlCommand m_activeCommand;
-		MySqlDataReader m_activeReader;
+		int m_activeCommandId;
 		bool m_useCompression;
 		bool m_isSecureConnection;
 		bool m_supportsConnectionAttributes;

tests/SideBySide/ConnectionPool.cs

@@ -149,6 +149,32 @@ public void LeakConnections()
 			}
 		}
 
+#if BASELINE
+		[Fact(Skip = "Throws MySqlException - error connecting: Timeout expired")]
+#else
+		[Fact]
+#endif
+		public void LeakReaders()
+		{
+			var csb = AppConfig.CreateConnectionStringBuilder();
+			csb.Pooling = true;
+			csb.MinimumPoolSize = 0;
+			csb.MaximumPoolSize = 6;
+			csb.ConnectionTimeout = 3u;
+
+			for (int i = 0; i < csb.MaximumPoolSize + 2; i++)
+			{
+				var connection = new MySqlConnection(csb.ConnectionString);
+				connection.Open();
+				var cmd = connection.CreateCommand();
+				cmd.CommandText = "SELECT 1";
+				var reader = cmd.ExecuteReader();
+				Assert.True(reader.Read());
+
+				// have to GC for leaked connections to be removed from the pool
+				GC.Collect();
+			}
+		}
 
 		[Fact]
 		public async Task WaitTimeout()