MySQL Bugs: #111759: 8.0.33 Removes Important Locks

Bug #111759	8.0.33 Removes Important Locks
Submitted:	14 Jul 2023 14:11	Modified:	30 Sep 2024 20:25
Reporter:	Stuart Lang (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / NET	Severity:	S2 (Serious)
Version:	8.0.33	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	Contribution

Description:
This change in this commit re-implemented many of the locks in the MySQL .NET client:
https://github.com/mysql/mysql-connector-net/commit/291c2f31fa9d7bf1b3c02bd7da19db1b66f89a...

The change in many places accidentally changes the locks for effectively a no-op implementation by creating a new SemaphoreSlim for every invocation, instead of reusing a field or static instance.

Example: https://github.com/mysql/mysql-connector-net/commit/291c2f31fa9d7bf1b3c02bd7da19db1b66f89a...

This results in unsafe concurrent execution and a lack of correctness. For our application it crashed in production very quickly with exceptions.

Exception and partial stack trace:

ystem.Data.Entity.Core.EntityException: The underlying provider failed on Open.
 ---> System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
   at System.Collections.Generic.Dictionary`2.FindValue(TKey key)
   at MySql.Data.Common.Ssl.StartSSLAsync(Stream baseStream, Encoding encoding, String connectionString, CancellationToken cancellationToken, Boolean execAsync)
   at MySql.Data.MySqlClient.NativeDriver.OpenAsync(Boolean execAsync, CancellationToken cancellationToken)
   at MySql.Data.MySqlClient.Driver.OpenAsync(Boolean execAsync, CancellationToken cancellationToken)
   at MySql.Data.MySqlClient.Driver.CreateAsync(MySqlConnectionStringBuilder settings, Boolean execAsync, CancellationToken cancellationToken)
   at MySql.Data.MySqlClient.Driver.CreateAsync(MySqlConnectionStringBuilder settings, Boolean execAsync, CancellationToken cancellationToken)
   at MySql.Data.MySqlClient.MySqlPool.CreateNewPooledConnectionAsync(Boolean execAsync, CancellationToken cancellationToken)
   at MySql.Data.MySqlClient.MySqlPool.GetPooledConnectionAsync(Boolean execAsync, CancellationToken cancellationToken)
   at MySql.Data.MySqlClient.MySqlPool.TryToGetDriverAsync(Boolean execAsync, CancellationToken cancellationToken)
   at MySql.Data.MySqlClient.MySqlPool.GetConnectionAsync(Boolean execAsync, CancellationToken cancellationToken)
   at MySql.Data.MySqlClient.MySqlConnection.OpenAsync(Boolean execAsync, CancellationToken cancellationToken)
   at System.Data.Entity.Core.EntityClient.EntityConnection.OpenAsync(CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at System.Data.Entity.Core.EntityClient.EntityConnection.OpenAsync(CancellationToken cancellationToken)
   at System.Data.Entity.Core.Objects.ObjectContext.EnsureConnectionAsync(Boolean shouldMonitorTransactions, CancellationToken cancellationToken)
   at System.Data.Entity.Core.Objects.ObjectContext.ExecuteInTransactionAsync[T](Func`1 func, IDbExecutionStrategy executionStrategy, Boolean startLocalTransaction, Boolean releaseConnectionOnSuccess, CancellationToken cancellationToken)
   at System.Data.Entity.Utilities.TaskExtensions.CultureAwaiter`1.GetResult()
   at System.Data.Entity.Core.Objects.ObjectQuery`1.GetResultsAsync(Nullable`1 forMergeOption, IDbExecutionStrategy executionStrategy, CancellationToken cancellationToken)
   at System.Data.Entity.Utilities.TaskExtensions.CultureAwaiter`1.GetResult()
   at System.Data.Entity.Internal.LazyAsyncEnumerator`1.FirstMoveNextAsync(CancellationToken cancellationToken)
   at System.Data.Entity.Infrastructure.IDbAsyncEnumerableExtensions.ForEachAsync[T](IDbAsyncEnumerator`1 enumerator, Action`1 action, CancellationToken cancellationToken)

How to repeat:
Create multiple connections currently.

Suggested fix:
Revert commit and carefully re-implement.

I started a PR to fix the issue, but the change was getting quite large so I do feel that it's better to revert it completely and do it again under careful review.

This problem was initially noted in bug #110717; the suggested fix was "Change GetPoolAsync to use a shared static SemaphoreSlim object (and wait on that), instead of creating a local variable."

However, the issue of the fundamentally incorrect use of SemaphoreSlim (in multiple locations) was not explicitly called out in that bug report.

I've pushed a PR, I don't necessarily think it's the right solution, but should highlight the various areas that need to be looked at:
https://github.com/mysql/mysql-connector-net/pull/56

Contribution submitted via Github - Try to re-add some synchronization (*) Contribution by Stuart Lang (Github slang25, mysql-connector-net/pull/56#issuecomment-1653718120): I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: git_patch_1439232418.txt (text/plain), 51.55 KiB.

This bug is also in version 8.1.0.
We can reproduce by create connection in multiple threads.
Will the PR release recentlly?

Thanks.

Hi :)

Eventually simply replace the Dictionary by ConcurrentDictionary:

private static readonly ConcurrentDictionary<string, SslProtocols> tlsConnectionRef = new();
private static readonly ConcurrentDictionary<string, int> tlsRetry = new();

and fix the "Remove" calls by "TryRemove"

Kind regards

Will this issue fix in https://bugs.mysql.com/bug.php?id=110717?

Thanks

Hello Stuart Lang,

Thank you for the bug report and contribution.

Regards,
Ashwini Patil

Posted by developer:
 
Fixed as of the upcoming MySQL Connector/NET 9.1.0 release, and here's the proposed changelog entry from the documentation team for review:

Made improvements to MySqlPoolManager.GetPoolAsync related fixes that
were made in Connector/NET 8.2.0.

Our thanks to Stuart Lang for the contribution.

Thank you for the bug report.