Get/Return pooled connections #3404

mdaigle · 2025-06-09T19:11:03Z

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 · 2025-06-09T20:46:54Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs

@@ -97,6 +97,8 @@ internal DbConnectionInternal(ConnectionState state, bool hidePassword, bool all

        #region Properties

+        internal DateTime CreateTime => _createTime;


Exposes creation time for load balancing and max connection lifetime enforcement.

Why to make getter and setter internal ?

It looks like this class DbConnectionInternal is managing its creation time, and this value isn’t exposed to derived classes.

If there’s any chance that this responsibility might shift in the future, or if derived classes may need to update the creation time, I recommend adding a protected method (e.g., UpdateCreationTime(DateTime newTime)) rather than exposing a protected setter or property.

My reasoning is:

Updating creation time should be an explicit, intentional action, not just a simple property assignment. An explicit method makes this clear in the API and signals that it’s a special operation as most of the logic is still in the base.

Let me know your thoughts.

Agreed. Other classes shouldn't be updating this property. This syntax will only expose a getter for the property: https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties#expression-body-definitions

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

mdaigle · 2025-06-10T16:25:05Z

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

benrr101

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

src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

...lient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs

paulmedynski

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.

tools/props/Versions.props

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

paulmedynski · 2025-06-13T12:09:16Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+            for (var numConnections = _numConnections; numConnections < MaxPoolSize; numConnections = _numConnections)
+            {
+                // Note that we purposefully don't use SpinWait for this: https://github.com/dotnet/coreclr/pull/21437
+                if (Interlocked.CompareExchange(ref _numConnections, numConnections + 1, numConnections) != numConnections)


This is complicated enough to warrant an English explanation of what's happening.

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

paulmedynski · 2025-06-13T12:13:12Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+
+                    return Task.FromResult<DbConnectionInternal?>(newConnection);
+                }
+                catch


Catch only what is documented to be thrown.

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

imasud00 · 2025-06-17T08:27:03Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+        /// </summary>
+        private static int _instanceCount;
+
+        private readonly int _instanceID = Interlocked.Increment(ref _instanceCount);


Interlocked.Increment(ref _instanceCount);

does it matter if it becomes negative ?

it starts at zero and only ever goes up

Counters are naturally unsigned, so can we use uint or ulong instead? I'd be proud if those ever wrapped :)

Also, should this be _instanceId ?

There's no Interlocked.Increment() overload for uint. It doesn't matter if it goes negative. It's only used for identification of unique pools in trace messages.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

imasud00 · 2025-06-17T19:05:51Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

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


ChannelDbConnectionPool

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

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

benrr101

Sorry, needs another round... But the other first round was pretty good!

benrr101 · 2025-06-18T23:48:09Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs

+        /// </summary>
+        internal DateTime CreateTime
+        {
+            get;


nit: these can/should be on the same line

benrr101 · 2025-06-18T23:49:53Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+        /// </summary>
+        private static int _instanceCount;
+
+        private readonly int _instanceID = Interlocked.Increment(ref _instanceCount);


it starts at zero and only ever goes up

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

benrr101 · 2025-06-18T23:51:19Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+        /// <inheritdoc />
+        public DbConnectionPoolIdentity Identity
+        {
+            get;


Ok I rescind my Nit designation - let's get these on the same line

benrr101 · 2025-06-18T23:52:07Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+            }
+
+            /* 
+                * This is ugly, but async anti-patterns above and below us in the stack necessitate a fresh task to be


nit: I think your indentation got messed up here

benrr101 · 2025-06-18T23:53:11Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+        }
+
+        /// <inheritdoc/>
+        internal Task<DbConnectionInternal?> OpenNewInternalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken)


Please chomp this biggie of a line :)

benrr101 · 2025-06-19T00:01:40Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

@@ -389,7 +500,7 @@ private async Task<DbConnectionInternal> GetInternalConnection(DbConnection owni
            try
            {
                // Continue looping around until we create/retrieve a connection or the timeout expires.
-                while (true)
+                while (true && !finalToken.IsCancellationRequested)


.... true is completely redundant now, lol

I'd rather have a while(true) than while(!finalToken.IsCancellationRequested). At least I know intuitively that while(true) implies we'll break out somewhere inside. The latter implies we run this code until timeout... And the combination is just, well redundant 😂

It would be nice if there was a way to have the condition of the while be while(connection is null && finalToken.IsCancellationRequested). Which I think is what we were working towards by suggesting a do/while loop - basically make at least one attempt to get a connection, and loop around if we haven't. I'd suggest then making it like:

do { finalToken.ThrowIfCancelled(); // do stuff to set connection } while(connection is null); return connection;

the latest refactor should help with this. coming soon!

samsharma2700 · 2025-06-19T19:53:57Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+        private readonly ChannelWriter<DbConnectionInternal?> _idleConnectionWriter;
+
+        // Counts the total number of open connections tracked by the pool.
+        private volatile int _numConnections;


When will this be different than the size of _connections array?

This counts the number of slots in the array that have actually been reserved. It starts at 0 and increments each time a connection is added to the pool for tracking. It's decremented every time a connection is destroyed and removed from the pool. The only time it will equal the size of the _connections array is when we're at max pool size.

samsharma2700 · 2025-06-19T19:54:11Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+            // We enforce Max Pool Size, so no need to create a bounded channel (which is less efficient)
+            // On the consuming side, we have the multiplexing write loop but also non-multiplexing Rents
+            // On the producing side, we have connections being released back into the pool (both multiplexing and not)
+            var idleChannel = Channel.CreateUnbounded<DbConnectionInternal?>();


I agree with the efficiency argument but have we compared the difference in performance for our specific use case implementation for Bounded vs Unbounded Channel. Bounded channel can offload the part of us worrying about Max/Min poolsize, flooding and starvation in exchange for some throughput loss.

With a bounded channel alone, we won't respect max pool size. The channel only stores connections that are "idle" (sitting unused in the pool). Connections that are in active use are not represented in the channel. So, if we used the bounded size of the channel to enforce max pool size, we wouldn't take into account any connections that are in active use.

samsharma2700 · 2025-06-19T22:45:54Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+        /// Tracks all connections currently managed by this pool, whether idle or busy.
+        /// Only updated rarely - when physical connections are opened/closed - but is read in perf-sensitive contexts.
+        /// </summary>
+        private readonly DbConnectionInternal?[] _connections;


So in essence _numConnections is doing the reservation part for us. Does the array itself provide any thing that Concurrent Bag/Dictionary/Queue will not provide. SemaphoreSlim does look seem to be the lightest, thread-safe solution here.

paulmedynski

A new round of comments, questions, and suggestions :)

src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs

paulmedynski · 2025-06-20T15:03:22Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+        /// </summary>
+        private static int _instanceCount;
+
+        private readonly int _instanceID = Interlocked.Increment(ref _instanceCount);


Counters are naturally unsigned, so can we use uint or ulong instead? I'd be proud if those ever wrapped :)

paulmedynski · 2025-06-20T15:05:43Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+            PoolGroupOptions = connectionPoolGroup.PoolGroupOptions;
+            ProviderInfo = connectionPoolProviderInfo;
+            Identity = identity;
+            AuthenticationContexts = new();


What's our take on initializing fields/properties that don't depend on a constructor body? Do we prefer to do it in each constructor, or do it on the field/property declaration? I prefer the latter. We've got a mix in this class so far (see _instanceID line 40).

I don't really have a preference here. Maybe a slight preference towards doing it in the constructor for instance variables, doing it outside the constructor (where possible) for statics. But I think I have more of a preference for not having a preference :)

paulmedynski · 2025-06-20T15:06:01Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+        /// </summary>
+        private static int _instanceCount;
+
+        private readonly int _instanceID = Interlocked.Increment(ref _instanceCount);


Also, should this be _instanceId ?

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

…nnot be modified and should be set regardless of pooling status.

mdaigle · 2025-06-26T23:39:14Z

...icrosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolState.cs

@@ -6,7 +6,6 @@ namespace Microsoft.Data.SqlClient.ConnectionPool
 {
    internal enum DbConnectionPoolState
    {
-        Initializing,


This was only used inside the constructor while fields were being initialized. You can't get a reference to a pool until the constructor completes, so we don't need this state.

mdaigle · 2025-06-26T23:39:38Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs

@@ -81,7 +83,7 @@ internal interface IDbConnectionPool
        /// <summary>
        /// The current state of the connection pool.
        /// </summary>
-        DbConnectionPoolState State { get; set; }
+        DbConnectionPoolState State { get; }


State should only be modifiable by the pool itself.

mdaigle · 2025-06-26T23:54:53Z

tools/specs/Microsoft.Data.SqlClient.nuspec

@@ -76,6 +77,7 @@
        <dependency id="System.Configuration.ConfigurationManager" version="9.0.4" exclude="Compile" />
        <dependency id="System.Security.Cryptography.Pkcs" version="9.0.4" />
        <dependency id="System.Text.Json" version="9.0.5" />
+        <dependency id="System.Threading.Channels" version="8.0.0" />


The System.Threading.Channels namespace isn't included in the netstandard API, but the package provided System.Threading.Channels targets netstandard2.0

vonzshik · 2025-06-27T14:53:52Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+                        async: true,
+                        timeout
+                    ).ConfigureAwait(false);
+                    taskCompletionSource.SetResult(connection);


Any particular reason to not use TrySetResult and TrySetException to avoid unnecessary exceptions?

I like this idea, done!

vonzshik · 2025-06-27T14:59:52Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+                // We're potentially on a new thread, so we need to properly set the ambient transaction.
+                // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState
+                // so that we can access it here. Read: area for improvement.
+                ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction);


Looking at the current source code of WaitHandleDbConnectionPool, it seems like everything between ADP.SetCurrentTransaction and taskCompletionSource.SetResult is synchronous, unlike here, where GetInternalConnection can go async. I wonder whether it's to make the default TransactionScope work (you have to pass a parameter to support async) with OpenAsync, although it's kinda weird to do so anyway.

Thanks, this is an important point. I haven't included transaction support in the scope of work for this PR and included this line prematurely. This line is incorrect for the reason you stated. When we go down the async path, we may end up creating/retrieving a connection in a continuation and that continuation would have no reference to the ambient transaction.

This does make me wonder whether we need any additional hygiene measures to make sure we reset ambient transaction state on threads. We wouldn't want a managed thread to hold onto the ambient transaction state and incorrectly enlist other connections.

…et-return-pooled-connections

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

… connection disposal when opening new connection.

paulmedynski

We're getting closer!

paulmedynski · 2025-06-20T15:32:12Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+        /// <returns>Returns true if the connection is live and unexpired, otherwise returns false.</returns>
+        private bool CheckConnection(DbConnectionInternal connection)
+        {
+            if (connection == null)


I don't think we need this check.

I refactored this a bit so that it makes sense based on the nullability constraints and to remove the side effects from this method.

paulmedynski · 2025-07-02T15:51:31Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs

-            get;
-            private set;
-        }
+        internal DateTime CreateTime { get; init; }


init means "Must be assigned during construction", but doesn't restrict that assignment to a constructor body. A caller could do this:

DbConnectionInternal conn = new(...) { CreateTime = DateTime.UnixEpoch; }

The initializer body runs after the constructor body, but still during construction. This isn't what we want.

I think all you need is:

internal DateTime CreateTime { get; }

That will make CrateTime immutable - you will be forced to assign it in all constructor bodies, and then prohibited from assigning to it anywhere else.

I suspect this applies to all of your use of init in this PR.

Makes sense, I didn't realize the init properties could be set in that way.

paulmedynski · 2025-07-02T15:53:42Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs

+{
+    /// <summary>
+    /// A thread-safe array that allows reservations.
+    /// A reservation *must* be made before adding a connection to the collection.


Is this an array or a collection? Let's use the most precise term consistently.

paulmedynski · 2025-07-02T15:55:06Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs

+    /// A thread-safe array that allows reservations.
+    /// A reservation *must* be made before adding a connection to the collection.
+    /// Exceptions *must* be handled by the caller when trying to add connections
+    /// and the caller *must* release the reservation.


Are you trying to explain that callers must release a reservation if adding a connection via that reservation fails?

I'll have to see how this works. Leaving a breadcrumb for myself...

paulmedynski · 2025-07-02T15:55:41Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs

+    ///     try {
+    ///         var connection = OpenConnection();
+    ///         slots.Add(connection);
+    ///     catch (InvalidOperationException ex)


Missing }.

paulmedynski · 2025-07-02T16:24:12Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

-            }
-
-            if (!CheckConnection(connection))
+            // Calling PrePush prevents the object from being reclaimed


Stale comment - does this belong inside ValidatOwnershipAndSetPoolingState() ?

paulmedynski · 2025-07-02T16:26:21Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+                        // We were able to get a connection, but the task was cancelled out from under us.
+                        // This can happen if the caller's CancellationToken is cancelled while we're waiting for a connection.
+                        // Check the success to avoid an unnecessary exception.
+                        this.ReturnInternalConnection(connection, owningObject);


Do we need this. here?

paulmedynski · 2025-07-02T16:31:21Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

+                    // attempts on the idle connection channel.
+                    if (newConnection is not null)
+                    {
+                        RemoveConnection(newConnection, trackedInSlots: false);


It seems fragile to separate the reservation and un-reservation across methods.

Could we make a reservation a first-class object and use disposal to have it un-reserve itself? And then have a Reservation.Keep() method that you call if you want to keep it (this would just ignore disposal). In fact, maybe only ConnectionSlots.Add() is allowed to call Reservation.Keep(), then all the paperwork is done for you by IDisposable and ConnectionSlots.

There is usually a single "keep" path, and multiple "don't keep" paths, so its safer to let the compiler worry about the "don't keep" ones for you.

I prefer this failsafe-by-default approach when acquiring resources.

You could go one step further, and only allow Add()inga connection to its reserved slot via a method on Reservation. Then you avoid the problem where you can call Add() on ConnectionSlots without having made a reservation. And once it's impossible to Add without a Reservation, you can just always decrement _reservations when you Remove() a connection that you found in the slots.

Essentially, you're using the lifetime of a Reservation object to:
a) guarantee you can Add(), and
b) guarentee someone else can't add if there are no free slots.

👍 I'll take a look at this and see what I can do. A failsafe API definitely sounds nicer, I just want to make sure we don't force extra allocations if we can avoid it. It's probably fine as long as we only allocate when opening and closing connections and not every time we get/return.

it wouldn't be a big allocation anyway

paulmedynski · 2025-07-02T18:16:58Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

-            if (!found)
-            {
-                return;
+            if (trackedInSlots) {


Hmm, is this parameter worth it? Now I can make a mistake and pass trackedInSlots = false when the connection is indeed in a slot.

In the false case, isn't that an exceptional condition anyway (i.e. we couldn't do some part of the "make a new connection" flow)? Does it really matter if we spin through our slots just to find out it wasn't there?

I think I agree. Like you mentioned, it's on an exception path anyway, so not as critical.

paulmedynski · 2025-07-02T18:29:27Z

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs

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


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 ....

mdaigle added 4 commits June 9, 2025 13:44

Add get flow and basic tests.

87a1129

Add tests. Clean up diff.

4233f94

Add additional tests. Refactor to test error behavior.

fb8c9d0

Implement return flow and add return/reuse tests.

73a379b

mdaigle force-pushed the dev/mdaigle/get-return-pooled-connections branch from 9d95355 to 73a379b Compare June 9, 2025 20:45

mdaigle commented Jun 9, 2025

View reviewed changes

mdaigle added 2 commits June 9, 2025 15:19

Clean up comments and todos.

7ac22f6

Add test cases.

7c75930

mdaigle mentioned this pull request Jun 5, 2025

Redesign the SqlClient Connection Pool to Improve Performance and Async Support #3356

Open

mdaigle changed the title ~~Dev/mdaigle/get return pooled connections~~ Get/Return pooled connections Jun 9, 2025

mdaigle marked this pull request as ready for review June 9, 2025 23:25

mdaigle requested a review from a team as a code owner June 9, 2025 23:25

edwardneal reviewed Jun 10, 2025

View reviewed changes

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Outdated Show resolved Hide resolved

benrr101 requested changes Jun 11, 2025

View reviewed changes

mdaigle added 6 commits June 12, 2025 10:40

Enable init property accessor in netfx.

6dde5b9

Switch to auto properties.

9999bf7

Reorder class members.

931ac03

Address review comments.

7d734f9

Merge main

12a8121

Fix channels package resolution.

ec27e63

paulmedynski requested changes Jun 13, 2025

View reviewed changes

Review changes.

c4d5808

imasud00 reviewed Jun 17, 2025

View reviewed changes

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Show resolved Hide resolved

imasud00 reviewed Jun 17, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs Outdated Show resolved Hide resolved

vonzshik reviewed Jun 17, 2025

View reviewed changes

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Outdated Show resolved Hide resolved

imasud00 reviewed Jun 17, 2025

View reviewed changes

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Outdated Show resolved Hide resolved

imasud00 reviewed Jun 17, 2025

View reviewed changes

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Outdated Show resolved Hide resolved

mdaigle commented Jun 17, 2025

View reviewed changes

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Outdated Show resolved Hide resolved

imasud00 reviewed Jun 18, 2025

View reviewed changes

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Outdated Show resolved Hide resolved

imasud00 reviewed Jun 18, 2025

View reviewed changes

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Show resolved Hide resolved

benrr101 requested changes Jun 19, 2025

View reviewed changes

samsharma2700 reviewed Jun 19, 2025

View reviewed changes

paulmedynski requested changes Jun 20, 2025

View reviewed changes

mdaigle added 7 commits June 20, 2025 13:22

Set CreateTime in constructor. This makes sense becuase CreateTime ca…

0451b50

…nnot be modified and should be set regardless of pooling status.

Naming

3f79f99

Fix potential connection leak when setting TaskCompletionSource result.

2d260bb

Fix package reference for central package management.

20adc00

Fix formatting. Refactor get connection loop.

a440e10

Move concurrent data structures out to dedicated class.

456ea1f

Refactor data structure to remove generics, revert to previous logic.

46dd516

mdaigle commented Jun 26, 2025

View reviewed changes

mdaigle added 3 commits June 26, 2025 17:00

Cleanup

7613b2c

Fix timeout exception handling for async path.

f7b9465

Merge main

4b79754

vonzshik reviewed Jun 27, 2025

View reviewed changes

mdaigle added 5 commits June 30, 2025 10:49

Review comments. Fix merge conflict.

67d0487

Merge branch 'main' of github.com:dotnet/SqlClient into dev/mdaigle/g…

0d9b606

…et-return-pooled-connections

Change object reference to full type. Fix tab.

7377e85

cleanup

901ba47

Add link to github issue for async pathways.

142f0ca

imasud00 reviewed Jul 1, 2025

View reviewed changes

...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Show resolved Hide resolved

Remove side effects from IsLiveConnection. Fix exception handling and…

5769604

… connection disposal when opening new connection.

paulmedynski requested changes Jul 2, 2025

View reviewed changes

mdaigle added 2 commits July 7, 2025 14:12

Minor review feedback. Make tests more reliable.

bc8b094

Fix compilation

92528d3

		@@ -97,6 +97,8 @@ internal DbConnectionInternal(ConnectionState state, bool hidePassword, bool all

		#region Properties

		internal DateTime CreateTime => _createTime;

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

Get/Return pooled connections #3404

Are you sure you want to change the base?

Get/Return pooled connections #3404

Uh oh!

Conversation

mdaigle commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdaigle commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mdaigle commented Jun 9, 2025 •

edited

Loading

mdaigle commented Jun 10, 2025 •

edited

Loading