-
Notifications
You must be signed in to change notification settings - Fork 310
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
base: main
Are you sure you want to change the base?
Get/Return pooled connections #3404
Conversation
9d95355
to
73a379b
Compare
@@ -97,6 +97,8 @@ internal DbConnectionInternal(ConnectionState state, bool hidePassword, bool all | |||
|
|||
#region Properties | |||
|
|||
internal DateTime CreateTime => _createTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposes creation time for load balancing and max connection lifetime enforcement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Show resolved
Hide resolved
@roji tagging you here if you have any time to review. Thanks! |
There was a problem hiding this 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 :)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...lient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs
Show resolved
Hide resolved
There was a problem hiding this 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.
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is complicated enough to warrant an English explanation of what's happening.
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Outdated
Show resolved
Hide resolved
|
||
return Task.FromResult<DbConnectionInternal?>(newConnection); | ||
} | ||
catch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catch only what is documented to be thrown.
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Show resolved
Hide resolved
/// </summary> | ||
private static int _instanceCount; | ||
|
||
private readonly int _instanceID = Interlocked.Increment(ref _instanceCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it starts at zero and only ever goes up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counters are naturally unsigned, so can we use uint or ulong instead? I'd be proud if those ever wrapped :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should this be _instanceId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Show resolved
Hide resolved
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Show resolved
Hide resolved
@@ -20,52 +25,136 @@ namespace Microsoft.Data.SqlClient.ConnectionPool | |||
/// </summary> | |||
internal sealed class ChannelDbConnectionPool : IDbConnectionPool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
/// <inheritdoc/> | ||
internal Task<DbConnectionInternal?> OpenNewInternalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we refactor this code for better readbility.
shared refactor code. Compisition here making this code easy
internal Task<DbConnectionInternal?> OpenNewInternalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken)
{
while (TryReserveConnectionSlot())
{
try
{
var connection = CreateAndTrackPhysicalConnection(owningConnection, userOptions);
return Task.FromResult<DbConnectionInternal?>(connection);
}
catch
{
ReleaseConnectionSlot();
SignalWaiters();
throw;
}
}
return Task.FromResult<DbConnectionInternal?>(null);
}
|
||
try | ||
{ | ||
// We've managed to increase the open counter, open a physical connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've managed to increase the open counter, open a physical connection.
private bool TryReserveConnectionSlot()
{
int current;
while ((current = _numConnections) < MaxPoolSize)
{
if (Interlocked.CompareExchange(ref _numConnections, current + 1, current) == current)
{
return true;
}
}
return false;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private void ReleaseConnectionSlot()
{
Interlocked.Decrement(ref _numConnections);
}
private void SignalWaiters()
{
_idleConnectionWriter.TryWrite(null);
}
* throughput high than to queue all of our opens onto a single worker thread. Add an async path | ||
* when this support is added to DbConnectionInternal. | ||
*/ | ||
DbConnectionInternal? newConnection = ConnectionFactory.CreatePooledConnection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private DbConnectionInternal CreateAndTrackPhysicalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions)
{
DbConnectionInternal? newConnection = ConnectionFactory.CreatePooledConnection(
this,
owningConnection,
PoolGroup.ConnectionOptions,
PoolGroup.PoolKey,
userOptions);
if (newConnection == null)
{
throw ADP.InternalError(ADP.InternalErrorCode.CreateObjectReturnedNull);
}
if (!newConnection.CanBePooled)
{
throw ADP.InternalError(ADP.InternalErrorCode.NewObjectCannotBePooled);
}
newConnection.PrePush(null);
if (!TryTrackConnectionInSlot(newConnection))
{
ReleaseConnectionSlot();
SignalWaiters();
throw new InvalidOperationException("No available slot to track new connection.");
}
return newConnection;
}
// We've managed to increase the open counter, open a physical connection. | ||
var startTime = Stopwatch.GetTimestamp(); | ||
|
||
/* TODO: This blocks the thread for several network calls! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have bug that is tracking to make this change ?
Debug.Assert(i < MaxPoolSize, $"Could not find free slot in {_connections} when opening."); | ||
if (i == MaxPoolSize) | ||
{ | ||
//TODO: generic exception? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, maybe we should close the connection and pretend we never created it
throw ADP.InternalError(ADP.InternalErrorCode.NewObjectCannotBePooled); | ||
} | ||
|
||
newConnection.PrePush(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider wrapping in a more meaningful name
/// </summary> | ||
/// <param name="connection"></param> | ||
/// <returns>Returns true if the connection is live and unexpired, otherwise returns false.</returns> | ||
private bool CheckConnection(DbConnectionInternal connection) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// This connection was tracked by the pool, so closing it has opened a free spot in the pool. | ||
// Write a null to the idle connection channel to wake up a waiter, who can now open a new | ||
// connection. Statement order is important since we have synchronous completions on the channel. | ||
_idleConnectionWriter.TryWrite(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of CloseConnetion. I will save RemoveConnection (could you add bool dipose which could be true by default or false)
I think the logic should be inversed.
We should first remove this conneciton from the slots and then call dispose(). This ensures that pool get cleared and dispose is best effort even if for any reason dispose throuws an exception we are good that we have cleaned up the slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, needs another round... But the other first round was pretty good!
/// </summary> | ||
internal DateTime CreateTime | ||
{ | ||
get; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these can/should be on the same line
/// </summary> | ||
private static int _instanceCount; | ||
|
||
private readonly int _instanceID = Interlocked.Increment(ref _instanceCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it starts at zero and only ever goes up
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Show resolved
Hide resolved
/// <inheritdoc /> | ||
public DbConnectionPoolIdentity Identity | ||
{ | ||
get; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I rescind my Nit designation - let's get these on the same line
} | ||
|
||
/* | ||
* This is ugly, but async anti-patterns above and below us in the stack necessitate a fresh task to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think your indentation got messed up here
} | ||
|
||
/// <inheritdoc/> | ||
internal Task<DbConnectionInternal?> OpenNewInternalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please chomp this biggie of a line :)
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.... 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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the latest refactor should help with this. coming soon!
private readonly ChannelWriter<DbConnectionInternal?> _idleConnectionWriter; | ||
|
||
// Counts the total number of open connections tracked by the pool. | ||
private volatile int _numConnections; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When will this be different than the size of _connections array?
// 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?>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new round of comments, questions, and suggestions :)
|
||
using System.ComponentModel; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra blank line.
using System.ComponentModel; | ||
|
||
|
||
// This class enables the use of the `init` property accessor in .NET framework. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link to documentation on why this is necessary? I found this:
/// </summary> | ||
private static int _instanceCount; | ||
|
||
private readonly int _instanceID = Interlocked.Increment(ref _instanceCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counters are naturally unsigned, so can we use uint or ulong instead? I'd be proud if those ever wrapped :)
PoolGroupOptions = connectionPoolGroup.PoolGroupOptions; | ||
ProviderInfo = connectionPoolProviderInfo; | ||
Identity = identity; | ||
AuthenticationContexts = new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
/// </summary> | ||
private static int _instanceCount; | ||
|
||
private readonly int _instanceID = Interlocked.Increment(ref _instanceCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should this be _instanceId
?
...rosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
Show resolved
Hide resolved
public DbConnectionPoolIdentity Identity | ||
{ | ||
get; | ||
private set; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be init too?
/// <inheritdoc /> | ||
public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource<DbConnectionInternal> taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection) | ||
{ | ||
// If taskCompletionSource is null, we are in a sync context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use ? notation on the argument type here, even though it's not specified on the IDbConnectionPool method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work fine, but I made the interface method use ? as well for clarity.
|
||
/// <inheritdoc /> | ||
public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource<DbConnectionInternal> taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection) | ||
/// <returns>Returns true if a valid idle connection is found, otherwise returns false.</returns> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method doesn't return bool.
Description
This PR adds
Get
andReturn
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: