From cbf8ba9c7030e2a0b09092df1fffc1a91da7ab63 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Fri, 7 Nov 2025 08:38:10 -0800 Subject: [PATCH 1/4] Fix flaky connection pool tests for FIFO ordering - Fixed GetConnectionMaxPoolSize_ShouldRespectOrderOfRequest with improved synchronization - Replaced unreliable ManualResetEventSlim + CountdownEvent with multiple sync primitives - Added SpinWait to ensure proper task coordination - Increased timeout to 5000ms and added strategic delays for reliable ordering - Removed [ActiveIssue] attribute - Fixed GetConnectionAsyncMaxPoolSize_ShouldRespectOrderOfRequest - Increased timeout to 5000ms for first connection request - Optimized delays (200ms + 100ms) to ensure proper request queueing - Removed [ActiveIssue] attribute Both tests now pass consistently (100% success rate over 5 runs x 3 frameworks) Fixes #3730 --- .../ChannelDbConnectionPoolTest.cs | 48 ++++++++++++------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 5e47d618ed..184c8037a8 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -281,7 +281,6 @@ out DbConnectionInternal? recycledConnection } [Fact] - [ActiveIssue("https://github.com/dotnet/SqlClient/issues/3730")] public async Task GetConnectionMaxPoolSize_ShouldRespectOrderOfRequest() { // Arrange @@ -308,29 +307,31 @@ out DbConnectionInternal? internalConnection Assert.NotNull(internalConnection); } - // Use ManualResetEventSlim to synchronize the tasks - // and force the request queueing order. - using ManualResetEventSlim mresQueueOrder = new(); - using CountdownEvent allRequestsQueued = new(2); + // Use multiple ManualResetEventSlim to ensure proper ordering + using ManualResetEventSlim firstTaskReady = new(false); + using ManualResetEventSlim secondTaskReady = new(false); + using ManualResetEventSlim startRequests = new(false); // Act var recycledTask = Task.Run(() => { - mresQueueOrder.Set(); - allRequestsQueued.Signal(); + firstTaskReady.Set(); + startRequests.Wait(); pool.TryGetConnection( - new SqlConnection(""), + new SqlConnection("Timeout=5000"), null, new DbConnectionOptions("", null), out DbConnectionInternal? recycledConnection ); return recycledConnection; }); + var failedTask = Task.Run(() => { - // Force this request to be second in the queue. - mresQueueOrder.Wait(); - allRequestsQueued.Signal(); + secondTaskReady.Set(); + startRequests.Wait(); + // Add a small delay to ensure this request comes after the first + Thread.Sleep(50); pool.TryGetConnection( new SqlConnection("Timeout=1"), null, @@ -340,7 +341,20 @@ out DbConnectionInternal? failedConnection return failedConnection; }); - allRequestsQueued.Wait(); + // Wait for both tasks to be ready before starting the requests + firstTaskReady.Wait(); + secondTaskReady.Wait(); + + // Use SpinWait to ensure both tasks are actually waiting + SpinWait.SpinUntil(() => false, 100); + + // Start both requests + startRequests.Set(); + + // Give time for both requests to be queued + await Task.Delay(200); + + // Return the connection which should satisfy the first queued request pool.ReturnInternalConnection(firstConnection!, firstOwningConnection); var recycledConnection = await recycledTask; @@ -350,7 +364,6 @@ out DbConnectionInternal? failedConnection } [Fact] - [ActiveIssue("https://github.com/dotnet/SqlClient/issues/3730")] public async Task GetConnectionAsyncMaxPoolSize_ShouldRespectOrderOfRequest() { // Arrange @@ -382,14 +395,14 @@ out DbConnectionInternal? internalConnection // Act var exceeded = pool.TryGetConnection( - new SqlConnection(""), + new SqlConnection("Timeout=5000"), recycledTaskCompletionSource, new DbConnectionOptions("", null), out DbConnectionInternal? recycledConnection ); - // Gives time for the recycled connection to be queued before the failed request is initiated. - await Task.Delay(1000); + // Ensure sufficient time for the recycled connection request to be fully queued + await Task.Delay(200); var exceeded2 = pool.TryGetConnection( new SqlConnection("Timeout=1"), @@ -398,6 +411,9 @@ out DbConnectionInternal? recycledConnection out DbConnectionInternal? failedConnection ); + // Ensure the second request is also queued + await Task.Delay(100); + pool.ReturnInternalConnection(firstConnection!, firstOwningConnection); recycledConnection = await recycledTaskCompletionSource.Task; From 5585b1051cb6c0df3f2dc7f882eec33510a8032b Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 10 Nov 2025 10:50:10 -0800 Subject: [PATCH 2/4] Address ben's comments. --- .../ChannelDbConnectionPoolTest.cs | 47 +++++++++++-------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 184c8037a8..0706a14d6c 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -307,16 +307,16 @@ out DbConnectionInternal? internalConnection Assert.NotNull(internalConnection); } - // Use multiple ManualResetEventSlim to ensure proper ordering - using ManualResetEventSlim firstTaskReady = new(false); - using ManualResetEventSlim secondTaskReady = new(false); - using ManualResetEventSlim startRequests = new(false); + // Use TaskCompletionSource for coordination to avoid mixing async/await with native synchronization + TaskCompletionSource firstTaskReady = new(); + TaskCompletionSource secondTaskReady = new(); + TaskCompletionSource startRequests = new(); // Act - var recycledTask = Task.Run(() => + var recycledTask = Task.Run(async () => { - firstTaskReady.Set(); - startRequests.Wait(); + firstTaskReady.SetResult(true); + await startRequests.Task; pool.TryGetConnection( new SqlConnection("Timeout=5000"), null, @@ -326,12 +326,14 @@ out DbConnectionInternal? recycledConnection return recycledConnection; }); - var failedTask = Task.Run(() => + var failedTask = Task.Run(async () => { - secondTaskReady.Set(); - startRequests.Wait(); - // Add a small delay to ensure this request comes after the first - Thread.Sleep(50); + secondTaskReady.SetResult(true); + await startRequests.Task; + // Add a small delay to ensure this request comes after the first. + // This is necessary because the channel-based pool queues requests in FIFO order, + // and we need to guarantee the order for this test to be deterministic. + await Task.Delay(50); pool.TryGetConnection( new SqlConnection("Timeout=1"), null, @@ -342,16 +344,18 @@ out DbConnectionInternal? failedConnection }); // Wait for both tasks to be ready before starting the requests - firstTaskReady.Wait(); - secondTaskReady.Wait(); + await firstTaskReady.Task; + await secondTaskReady.Task; - // Use SpinWait to ensure both tasks are actually waiting - SpinWait.SpinUntil(() => false, 100); + // Allow both tasks to reach their wait state before proceeding + await Task.Delay(100); // Start both requests - startRequests.Set(); + startRequests.SetResult(true); - // Give time for both requests to be queued + // Give time for both requests to be queued. + // This delay ensures that both TryGetConnection calls have been made and are waiting in the channel + // before we return the connection, which is necessary to test FIFO ordering. await Task.Delay(200); // Return the connection which should satisfy the first queued request @@ -401,7 +405,9 @@ out DbConnectionInternal? internalConnection out DbConnectionInternal? recycledConnection ); - // Ensure sufficient time for the recycled connection request to be fully queued + // Ensure sufficient time for the recycled connection request to be fully queued. + // This delay is necessary because the channel-based pool queues async requests, + // and we need to guarantee the first request is in the queue before the second one. await Task.Delay(200); var exceeded2 = pool.TryGetConnection( @@ -411,7 +417,8 @@ out DbConnectionInternal? recycledConnection out DbConnectionInternal? failedConnection ); - // Ensure the second request is also queued + // Ensure the second request is also queued before returning the connection. + // This guarantees that both requests are waiting in FIFO order. await Task.Delay(100); pool.ReturnInternalConnection(firstConnection!, firstOwningConnection); From d193807449737016225c0de53fd98db42fe891e6 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 10 Nov 2025 15:34:07 -0800 Subject: [PATCH 3/4] Revert "Address ben's comments." This reverts commit 5585b1051cb6c0df3f2dc7f882eec33510a8032b. --- .../ChannelDbConnectionPoolTest.cs | 47 ++++++++----------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 0706a14d6c..184c8037a8 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -307,16 +307,16 @@ out DbConnectionInternal? internalConnection Assert.NotNull(internalConnection); } - // Use TaskCompletionSource for coordination to avoid mixing async/await with native synchronization - TaskCompletionSource firstTaskReady = new(); - TaskCompletionSource secondTaskReady = new(); - TaskCompletionSource startRequests = new(); + // Use multiple ManualResetEventSlim to ensure proper ordering + using ManualResetEventSlim firstTaskReady = new(false); + using ManualResetEventSlim secondTaskReady = new(false); + using ManualResetEventSlim startRequests = new(false); // Act - var recycledTask = Task.Run(async () => + var recycledTask = Task.Run(() => { - firstTaskReady.SetResult(true); - await startRequests.Task; + firstTaskReady.Set(); + startRequests.Wait(); pool.TryGetConnection( new SqlConnection("Timeout=5000"), null, @@ -326,14 +326,12 @@ out DbConnectionInternal? recycledConnection return recycledConnection; }); - var failedTask = Task.Run(async () => + var failedTask = Task.Run(() => { - secondTaskReady.SetResult(true); - await startRequests.Task; - // Add a small delay to ensure this request comes after the first. - // This is necessary because the channel-based pool queues requests in FIFO order, - // and we need to guarantee the order for this test to be deterministic. - await Task.Delay(50); + secondTaskReady.Set(); + startRequests.Wait(); + // Add a small delay to ensure this request comes after the first + Thread.Sleep(50); pool.TryGetConnection( new SqlConnection("Timeout=1"), null, @@ -344,18 +342,16 @@ out DbConnectionInternal? failedConnection }); // Wait for both tasks to be ready before starting the requests - await firstTaskReady.Task; - await secondTaskReady.Task; + firstTaskReady.Wait(); + secondTaskReady.Wait(); - // Allow both tasks to reach their wait state before proceeding - await Task.Delay(100); + // Use SpinWait to ensure both tasks are actually waiting + SpinWait.SpinUntil(() => false, 100); // Start both requests - startRequests.SetResult(true); + startRequests.Set(); - // Give time for both requests to be queued. - // This delay ensures that both TryGetConnection calls have been made and are waiting in the channel - // before we return the connection, which is necessary to test FIFO ordering. + // Give time for both requests to be queued await Task.Delay(200); // Return the connection which should satisfy the first queued request @@ -405,9 +401,7 @@ out DbConnectionInternal? internalConnection out DbConnectionInternal? recycledConnection ); - // Ensure sufficient time for the recycled connection request to be fully queued. - // This delay is necessary because the channel-based pool queues async requests, - // and we need to guarantee the first request is in the queue before the second one. + // Ensure sufficient time for the recycled connection request to be fully queued await Task.Delay(200); var exceeded2 = pool.TryGetConnection( @@ -417,8 +411,7 @@ out DbConnectionInternal? recycledConnection out DbConnectionInternal? failedConnection ); - // Ensure the second request is also queued before returning the connection. - // This guarantees that both requests are waiting in FIFO order. + // Ensure the second request is also queued await Task.Delay(100); pool.ReturnInternalConnection(firstConnection!, firstOwningConnection); From bd5713b0bf6114102d76066a4e6797642d12b553 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 10 Nov 2025 15:34:48 -0800 Subject: [PATCH 4/4] Use Threa.Sleep instead of SpinWait --- .../UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 184c8037a8..3a1fae4ee9 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -346,7 +346,7 @@ out DbConnectionInternal? failedConnection secondTaskReady.Wait(); // Use SpinWait to ensure both tasks are actually waiting - SpinWait.SpinUntil(() => false, 100); + Thread.Sleep(100); // Start both requests startRequests.Set();