Skip to content

Get/Return pooled connections #3404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

mdaigle
Copy link
Contributor

@mdaigle mdaigle commented Jun 9, 2025

Description

This PR adds Get and Return functionality to the new ChannelDbConnectionPool class.
A channel and corresponding channel reader and writer are added to underpin these operations.
An array is added to hold references to all connections managed by the pool.
Logic is included to respect max pool size in all cases.

Not included: pool warm up (including respecting min pool size), pool pruning, transactions, tracing/logs. These will come in follow-up PRs.

Also includes extensive unit test coverage for the implemented functionality. Integration testing is reserved for a later state when more of the pool functionality is available.

Issues

#3356

Testing

Tests focus on the Get and Return flows with special focus on the following:

  • async behavior
  • max pool size respected
  • connections successfully reused
  • requests successfully queued and queue order respected
  • stress testing with many parallel operations

@mdaigle mdaigle force-pushed the dev/mdaigle/get-return-pooled-connections branch from 9d95355 to 73a379b Compare June 9, 2025 20:45
@mdaigle mdaigle changed the title Dev/mdaigle/get return pooled connections Get/Return pooled connections Jun 9, 2025
@mdaigle mdaigle marked this pull request as ready for review June 9, 2025 23:25
@mdaigle mdaigle requested a review from a team as a code owner June 9, 2025 23:25
@mdaigle
Copy link
Contributor Author

mdaigle commented Jun 10, 2025

@roji tagging you here if you have any time to review. Thanks!

Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it looks good. A bunch of things I'd like addressed, but you know I'll accept valid arguments against fixing them :)

Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the ChannelDbConnectionPool changes. I will look at the tests once we have converged on an implementation. It might be a good idea to host another meeting to walk through the commentary here.

@@ -20,52 +25,136 @@ namespace Microsoft.Data.SqlClient.ConnectionPool
/// </summary>
internal sealed class ChannelDbConnectionPool : IDbConnectionPool

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChannelDbConnectionPool

it might be good to add some documentation on how this class works and how this is different than the WaitHandleDbConnectionPool.

… connection disposal when opening new connection.
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're getting closer!

{
cancellationToken.ThrowIfCancellationRequested();
Debug.Assert(finalToken.IsCancellationRequested);
// We didn't find an idle connection, try to open a new one.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ??= comments are a bit misleading. At line 466 we don't know whether or not we got an idle connection. We only know that if we hit the await part. Maybe this would be clearer as If we didn't find ..., then try to ....

benrr101
benrr101 previously approved these changes Jul 8, 2025
Copy link

codecov bot commented Jul 16, 2025

Codecov Report

❌ Patch coverage is 89.57529% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.78%. Comparing base (5354e83) to head (cf418ab).
⚠️ Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
...qlClient/ConnectionPool/ChannelDbConnectionPool.cs 89.28% 21 Missing ⚠️
...icrosoft/Data/ProviderBase/DbConnectionInternal.cs 33.33% 2 Missing ⚠️
...ta/SqlClient/ConnectionPool/ConnectionPoolSlots.cs 95.91% 2 Missing ⚠️
...c/Microsoft/Data/SqlClient/SqlConnectionFactory.cs 80.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (5354e83) and HEAD (cf418ab). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (5354e83) HEAD (cf418ab)
netcore 1 0
addons 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3404      +/-   ##
==========================================
- Coverage   69.75%   63.78%   -5.97%     
==========================================
  Files         276      201      -75     
  Lines       62192    44159   -18033     
==========================================
- Hits        43381    28169   -15212     
+ Misses      18811    15990    -2821     
Flag Coverage Δ
addons ?
netcore ?
netfx 63.78% <89.57%> (-5.64%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

benrr101
benrr101 previously approved these changes Jul 17, 2025
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looking for some changes from previous reviews. Good changes in these last 4 commits though.

Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Will look at tests shortly. Going to loop through older comments first.

Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look at the slot tests. I'll spin through the pool tests once this set of comments is resolved.

}
catch (OutOfMemoryException)
{
// OutOfMemoryException is acceptable when trying to allocate an array of int.MaxValue size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How long does this test take to run? Does the runtime recover from this gracefully, and the parallel and subsequent tests run fine?

Is there a way to perform the argument checks without the huge allocation work?

Is it actually reasonable to have 2 billion slots?

Maybe we want an internal static property that defaults to a reasonable max slots, say 10,000. Apps would use that value, and you can confirm it in a test. But this test could set it much smaller to test the limit checks, and then reset it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather just remove this test. Having a pool of Int.MaxValue capacity doesn't make sense. Our biggest customers in azure aren't using more than a few thousand. This test is really only exercising OutOfMemory exception throwing, we can trust the runtime to do that properly.

[InlineData(10u)]
[InlineData(100u)]
[InlineData(1000u)]
public void Constructor_ValidCapacityValues_SetsReservationCountToZero(uint capacity)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've already sufficiently verified this in the above tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified to construct a large pool, no longer covered after removing the test above.


// Act
var connection = poolSlots.Add(
createCallback: state => new MockDbConnectionInternal(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These callbacks should track how many times they are called, and then you verify expectations below.

For createCallback, you could also save the connection it returns and verify that the same instance is returned from Add().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these callbacks should also verify they are called with the expected arguments

MockDbConnectionInternal expectedConnection = new();

var connection = poolSlots.Add(
  createCallback: (state) =>
  {
    Assert.Equal("test", state);
    return expectedConnection;
  },
  ...);

Assert.Same(connection, expectedConnection);

Copy link
Contributor Author

@mdaigle mdaigle Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a test that specifically checks that callbacks receive the proper state value.

// Act
var connection = poolSlots.Add(
createCallback: state => new MockDbConnectionInternal(),
cleanupCallback: (conn, state) => { },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this particular test, you don't expect the cleanupCallback to be called, so its body could be an Assert.Fail() instead of a counter.

cleanupState: "cleanup"));

Assert.Contains("Failed to create or add connection", exception.Message);
Assert.True(cleanupCalled);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, it's important to count the number of calls to these callbacks rather than just tracking if they've been called.

}

[Fact]
public void TryRemove_MultipleConnections_RemovesCorrectConnection()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, you don't know if TryRemove() removed the correct connection. You're verifying that it remove a connection (indirectly via the reservation count).

Not sure if there's a nice way to test what you actually describe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test that covers removing the same connection twice when another connection is also in the slots. The second remove should fail and reservation count should remain at 1. It shows that the slots distinguish between connections.

int index = i;
removeTasks[i] = Task.Run(() =>
{
if (connections[index] != null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would connections[index] be null here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

using System.Data;
using System.Data.Common;
using System.Transactions;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's enable nullable.

}

[Fact]
public void Add_WithNullCleanupCallback_ThrowsArgumentNullException()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't document this exception in the Add() API, which I think is fine. Your API says "Don't pass null for the callbacks", which means callers should expect undefined behaviour if they misuse your API.

Maybe add a comment to this test noting the above. It's not testing any API promise, but rather confirming undefined behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this test. Like you said, it's enforced by the type system.

}

[Fact]
public void Constructor_BoundaryValue_MaxInt_WorksCorrectly()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could benefit from a configurable max.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in favor of a large capacity test.

@Copilot Copilot AI review requested due to automatic review settings July 25, 2025 23:01
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements core Get and Return functionality for the new ChannelDbConnectionPool class, establishing the foundation for channel-based connection pooling. The implementation introduces a novel approach using System.Threading.Channels for managing idle connections and a custom ConnectionPoolSlots class for thread-safe connection tracking.

  • Adds channel-based connection pooling with Get/Return operations that respect max pool size limits
  • Implements ConnectionPoolSlots for thread-safe connection management with reservation-based slot allocation
  • Includes comprehensive unit test coverage for both connection pooling logic and slot management

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tools/specs/Microsoft.Data.SqlClient.nuspec Adds System.Threading.Channels dependency for both .NET 6+ and .NET 8+ target frameworks
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlotsTest.cs New comprehensive unit tests for ConnectionPoolSlots covering concurrent operations, capacity limits, and edge cases
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs Extensive unit tests for ChannelDbConnectionPool covering sync/async operations, timeout handling, connection reuse, and stress testing
src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs Adds explanatory comment for C# 9.0 init accessor polyfill
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs Makes constructor protected and refactors connection creation methods to support pooling scenarios
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs Updates ReturnInternalConnection signature and removes redundant validation logic
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs Updates interface signatures for nullable references and standardizes parameter types
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolState.cs Removes Initializing state from enum
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs New thread-safe collection with fixed capacity using reservation-based slot allocation
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Implements core connection pooling with channel-based idle connection management
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs Exposes CreateTime property and updates PrePush signature for stronger typing
Various project files Adds System.Threading.Channels package references across different target frameworks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants