Skip to content

Replace internal connection pool state enum with a boolean #4796

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.

Sign up for GitHub

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
priyankatiwari08 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Replace internal connection pool state enum with a boolean #4796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 24 additions & 25 deletions ...oft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
using Microsoft.Data.Common;
using Microsoft.Data.ProviderBase;
using Microsoft.Data.SqlClient.Diagnostics;
using static Microsoft.Data.SqlClient.ConnectionPool.DbConnectionPoolState;
using Microsoft.Data.SqlClient.Internal;

#nullable enable
Expand Down Expand Up @@ -111,6 +110,9 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable
/// </summary>
private int _shutdownInitiated;

// Read by acquisition and maintenance threads without taking the shutdown guard.
private volatile bool _isRunning;

/// <summary>
/// Optional concurrency limiter that throttles the number of concurrent physical connection
/// creation attempts. When null, no rate limiting is applied. A non-null limiter is
Expand Down Expand Up @@ -205,7 +207,7 @@ internal ChannelDbConnectionPool(

Reclaimer = new PoolReclaimer(this, _timeProvider);

State = Running;
_isRunning = true;

SqlClientEventSource.Log.TryPoolerTraceEvent(
"ChannelDbConnectionPool.ChannelDbConnectionPool | INFO | {0}, Constructed. MinPoolSize={1}, MaxPoolSize={2}",
Expand Down Expand Up @@ -256,7 +258,7 @@ public ConcurrentDictionary<
public DbConnectionPoolIdentity Identity { get; }

/// <inheritdoc />
public bool IsRunning => State == Running;
public bool IsRunning => _isRunning;

/// <inheritdoc />
public TimeSpan LoadBalanceTimeout => PoolGroupOptions.LoadBalanceTimeout;
Expand All @@ -270,9 +272,6 @@ public ConcurrentDictionary<
/// <inheritdoc />
public DbConnectionPoolProviderInfo ProviderInfo { get; }

/// <inheritdoc />
public DbConnectionPoolState State { get; private set; }

/// <inheritdoc />
public TransactedConnectionPool TransactedConnectionPool { get; }

Expand Down Expand Up @@ -375,7 +374,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection)
// NOTE: no locking is required here because if we're in this method we can safely
// presume that the caller is the only one using the connection, that all pre-push logic
// has been done, and that all transactions have ended.
if (State is Running && connection.CanBePooled)
if (IsRunning && connection.CanBePooled)
{
SqlClientEventSource.Log.TryPoolerTraceEvent(
"ChannelDbConnectionPool.PutObjectFromTransactedPool | INFO | {0}, Connection {1}, Transaction has ended; returning connection to pool.",
Expand Down Expand Up @@ -468,8 +467,8 @@ public DbConnectionInternal ReplaceConnection(
// try below are intentionally excluded -- the server proved reachable -- matching the
// WaitHandle pool, where PrepareConnection runs outside CreateObject's error-state catch.
// We exclude OperationCanceledException (caller-side timeout/cancellation, not a physical
// failure) and only enter while Running, mirroring OpenNewInternalConnection.
if (State == Running)
// failure) and only enter while running, mirroring OpenNewInternalConnection.
if (IsRunning)
{
_errorState?.Enter(ex);
}
Expand Down Expand Up @@ -579,15 +578,15 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti
}

// Note: this logic mirrors WaitHandleDbConnectionPool.ReturnObject, minus one dead
// branch. Its Running path also checks IsTransactionRoot && Pool == null -> SetInStasis,
// branch. Its running path also checks IsTransactionRoot && Pool == null -> SetInStasis,
// under its own "how did we get here if the pool is null?" TODO. A connection cannot
// arrive here without a pool, because this method is called through the connection's own
// Pool reference. The branch is redundant in any case: putting a transaction root in
// stasis is exactly what the first case below does when the connection cannot be pooled.
ReturnDisposition disposition;
lock (connection)
{
if (State is not Running || !connection.CanBePooled)
if (!IsRunning || !connection.CanBePooled)
{
// A transaction root that cannot be pooled must be put in stasis rather than
// closed. Closing it would orphan the root transaction with no means to promote
Expand Down Expand Up @@ -720,7 +719,7 @@ private void PutConnectionInIdleChannel(DbConnectionInternal connection, bool pr
if (!_idleChannel.TryWrite(connection))
{
// The channel has been completed (pool is shutting down). Race window
// between the State check by the caller and TryWrite: destroy instead of pooling.
// between the IsRunning check by the caller and TryWrite: destroy instead of pooling.
RemoveConnection(connection);
}
}
Expand All @@ -737,13 +736,13 @@ public void Shutdown()
SqlClientEventSource.Log.TryPoolerTraceEvent(
"ChannelDbConnectionPool.Shutdown | INFO | {0}", Id);

// Transition to ShuttingDown. After this point, ReturnInternalConnection
// Stop accepting new requests. After this point, ReturnInternalConnection
// routes returning connections to RemoveConnection.
State = ShuttingDown;
_isRunning = false;

// Cancel any in-flight background warmup/replenishment so no new connections are
// created after shutdown begins (Story 4). The warmup loop also observes the
// State transition above; cancelling here stops the loop promptly by tripping its
// running flag above; cancelling here stops the loop promptly by tripping its
// await points and pre-create checks. It does not abort an already in-progress
// synchronous physical open (see _warmupCts field docs). Cancel is idempotent.
try
Expand Down Expand Up @@ -874,7 +873,7 @@ public void Shutdown()
/// <inheritdoc />
public void Startup()
{
// State is set to Running in the constructor, and PoolPruner (when present, i.e.
// The pool is running from construction, and PoolPruner (when present, i.e.
// MinPoolSize < MaxPoolSize) is also constructed eagerly there; its timer arms/disarms
// via UpdateTimer() calls from OpenNewInternalConnection and RemoveConnection as the
// pool grows/shrinks.
Expand Down Expand Up @@ -918,15 +917,15 @@ public bool TryGetConnection(
TimeoutTimer timeout,
out DbConnectionInternal? connection)
{
// Short-circuit when the pool is not Running (i.e., shut down or never started).
// Short-circuit when the pool is not running.
// Returning (true, null) matches WaitHandleDbConnectionPool.TryGetConnection and tells
// the caller "completed; no connection available" without entering the channel path,
// which would otherwise reserve a slot, attempt to open a fresh physical connection,
// and then immediately destroy it on return because State == ShuttingDown.
if (State is not Running)
// and then immediately destroy it on return because the pool is not running.
if (!IsRunning)
{
SqlClientEventSource.Log.TryPoolerTraceEvent(
"ChannelDbConnectionPool.TryGetConnection | INFO | {0}, State != Running.", Id);
"ChannelDbConnectionPool.TryGetConnection | INFO | {0}, Pool is not running.", Id);
connection = null;
return true;
}
Expand Down Expand Up @@ -1243,13 +1242,13 @@ _connectionCreationRateLimiter is not null &&
// must not poison the pool into fast-fail/backoff for other callers.
// FR-006, FR-007.
//
// Only enter the error state while the pool is still Running. A synchronous physical
// Only enter the error state while the pool is still running. A synchronous physical
// open cannot be cancelled once it is in progress (see _warmupCts field docs), so a
// warmup/user open that began before Shutdown can complete with a failure after
// Shutdown has already disposed _errorState. Entering here would re-arm the exit
// timer and keep callbacks/logging alive past teardown, so we skip it once shutdown
// has begun; the throw below still propagates the failure to the caller/warmup loop.
if (State == Running)
if (IsRunning)
{
_errorState?.Enter(ex);
}
Expand Down Expand Up @@ -1407,7 +1406,7 @@ private void RemoveConnection(DbConnectionInternal connection)
// shared serial, rate-limited warmup path (Story 5). This is the single choke point for
// every below-minimum event: connections destroyed on return (broken/lifetime), idle
// timeout eviction, and pruning all funnel through RemoveConnection. RequestWarmup is a
// no-op when MinPoolSize == 0, the pool is not Running, or a warmup loop is already
// no-op when MinPoolSize == 0, the pool is not running, or a warmup loop is already
// running/queued, so calling it unconditionally here is cheap.
RequestWarmup();
}
Expand Down Expand Up @@ -1878,7 +1877,7 @@ internal void RequestWarmup()
{
// No-op when there is nothing to pre-create (MinPoolSize == 0), the pool is not running,
// or shutdown has cancelled background activity.
if (MinPoolSize == 0 || State != Running || _warmupCts.IsCancellationRequested)
if (MinPoolSize == 0 || !IsRunning || _warmupCts.IsCancellationRequested)
{
return;
}
Expand Down Expand Up @@ -1972,7 +1971,7 @@ private async Task RunWarmupLoopAsync()
// WaitHandle pool, which skips replenishment while blocking. (Warmup still
// participates in the error state on its own creations - entering it on failure and
// clearing it on success - via OpenNewInternalConnection.)
while (State == Running
while (IsRunning
&& !token.IsCancellationRequested
&& !ErrorOccurred
&& _connectionSlots.ReservationCount < MinPoolSize)
Expand Down
12 changes: 0 additions & 12 deletions ...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolState.cs
View file
Open in desktop

This file was deleted.

12 changes: 6 additions & 6 deletions ...Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ internal interface IDbConnectionPool
/// <summary>
/// Indicates whether the connection pool is currently running.
/// </summary>
/// <remarks>
/// Pools are running from construction. Shutdown makes this false permanently;
/// neither Startup nor Clear makes a retired pool running again.
/// This flag does not indicate whether shutdown cleanup has finished.
/// </remarks>
bool IsRunning { get; }

/// <summary>
Expand All @@ -102,11 +107,6 @@ internal interface IDbConnectionPool
/// </summary>
DbConnectionPoolProviderInfo ProviderInfo { get; }

/// <summary>
/// The current state of the connection pool.
/// </summary>
DbConnectionPoolState State { get; }

/// <summary>
/// Holds connections that are currently enlisted in a transaction.
/// </summary>
Expand All @@ -120,7 +120,7 @@ internal interface IDbConnectionPool

#region Methods
/// <summary>
/// Clears the connection pool, releasing all connections and resetting the state.
/// Clears the connection pool without changing whether it is running.
/// </summary>
/// <remarks>
/// Clearing the pool is an expensive operation and should only be used if required.
Expand Down
43 changes: 20 additions & 23 deletions ....Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
using Microsoft.Data.Common;
using Microsoft.Data.ProviderBase;
using Microsoft.Data.SqlClient.Diagnostics;
using static Microsoft.Data.SqlClient.ConnectionPool.DbConnectionPoolState;
using Microsoft.Data.SqlClient.Internal;

namespace Microsoft.Data.SqlClient.ConnectionPool
Expand Down Expand Up @@ -54,7 +53,8 @@ internal sealed class WaitHandleDbConnectionPool : IDbConnectionPool

public int Id => Interlocked.Increment(ref _objectTypeCount);

public DbConnectionPoolState State { get; set; }
// Read by acquisition and maintenance threads without locking the pool.
private volatile bool _isRunning;

// This class is a way to stash our cloned Tx key for later disposal when it's no longer needed.
// We can't get at the key in the dictionary without enumerating entries, so we stash an extra
Expand Down Expand Up @@ -271,7 +271,7 @@ internal WaitHandleDbConnectionPool(
_transactedConnectionPool = new TransactedConnectionPool(this, Metrics);

_poolCreateRequest = new WaitCallback(PoolCreateRequest); // used by CleanupCallback
State = Running;
_isRunning = true;
SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.DbConnectionPool|RES|CPOOL> {0}, Constructed.", Id);

//_cleanupTimer & QueuePoolCreateRequest is delayed until DbConnectionPoolGroup calls
Expand Down Expand Up @@ -304,7 +304,7 @@ private bool NeedToReplenish
{
get
{
if (State is not Running) // Don't allow connection create when not running.
if (!IsRunning) // Don't allow connection create when not running.
{
return false;
}
Expand All @@ -331,10 +331,7 @@ private bool NeedToReplenish

public DbConnectionPoolIdentity Identity => _identity;

public bool IsRunning
{
get { return State is Running; }
}
public bool IsRunning => _isRunning;

internal int MaxPoolSize => PoolGroupOptions.MaxPoolSize;

Expand All @@ -359,10 +356,10 @@ public bool IsRunning

internal void CleanupCallback(object state)
{
// If the pool is not Running, skip work. Shutdown disposes the timer, but
// If the pool is not running, skip work. Shutdown disposes the timer, but
// a callback may already be in-flight when Shutdown runs; this guard ensures it does
// not perform pruning or re-arm pool create requests.
if (State is not Running)
if (!IsRunning)
{
return;
}
Expand Down Expand Up @@ -618,7 +615,7 @@ private void DeactivateObject(DbConnectionInternal obj)
// transaction object will ensure that it is owned (not lost),
// and it will be certain to put it back into the pool.

if (State is ShuttingDown)
if (!IsRunning)
{
if (obj.IsTransactionRoot)
{
Expand Down Expand Up @@ -656,7 +653,7 @@ private void DeactivateObject(DbConnectionInternal obj)
Transaction transaction = obj.EnlistedTransaction;
if (transaction != null)
{
// NOTE: we're not locking on _state, so it's possible that its
// NOTE: we're not locking around IsRunning, so it's possible that its
// value could change between the conditional check and here.
// Although perhaps not ideal, this is OK because the
// DelegatedTransactionEnded event will clean up the
Expand Down Expand Up @@ -882,9 +879,9 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource<DbC
allowCreate = true;
}

if (State is not Running)
if (!IsRunning)
{
SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetConnection|RES|CPOOL> {0}, DbConnectionInternal State != Running.", Id);
SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetConnection|RES|CPOOL> {0}, Pool is not running.", Id);
connection = null;
return true;
}
Expand All @@ -902,10 +899,10 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource<DbC

// Before queueing, the factory can still redirect this request to a new pool.
// Once queued, the request must finish on this pool, even if it is retired.
if (State is not Running)
if (!IsRunning)
{
SqlClientEventSource.Log.TryPoolerTraceEvent(
"<prov.DbConnectionPool.GetConnection|RES|CPOOL> {0}, Pool not Running after inner TryGetConnection; short-circuit async path.", Id);
"<prov.DbConnectionPool.GetConnection|RES|CPOOL> {0}, Pool is not running after inner TryGetConnection; short-circuit async path.", Id);
connection = null;
return true;
}
Expand Down Expand Up @@ -1264,7 +1261,7 @@ private void PoolCreateRequest(object state)
long scopeID = SqlClientEventSource.Log.TryPoolerScopeEnterEvent("<prov.DbConnectionPool.PoolCreateRequest|RES|INFO|CPOOL> {0}", Id);
try
{
if (State is Running)
if (IsRunning)
{
// in case WaitForPendingOpen ever failed with no subsequent OpenAsync calls,
// start it back up again
Expand Down Expand Up @@ -1443,7 +1440,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal obj)
// done and all transactions are ended.
SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.PutObjectFromTransactedPool|RES|CPOOL> {0}, Connection {1}, Transaction has ended.", Id, obj.ObjectID);

if (State is Running && obj.CanBePooled)
if (IsRunning && obj.CanBePooled)
{
obj.ResetConnection();
PutNewObject(obj);
Expand All @@ -1457,7 +1454,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal obj)

private void QueuePoolCreateRequest()
{
if (State is Running)
if (IsRunning)
{
// Make sure we're at quota by posting a callback to the threadpool.
ThreadPool.QueueUserWorkItem(_poolCreateRequest);
Expand Down Expand Up @@ -1545,7 +1542,7 @@ public void Startup()
// Shutdown() would create a fresh _cleanupTimer and queue a PoolCreateRequest
// against a pool that will never accept connections back, leaking the timer
// and scheduling background work that immediately short-circuits.
if (State is not Running)
if (!IsRunning)
{
return;
}
Expand All @@ -1562,12 +1559,12 @@ public void Shutdown()
{
SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.Shutdown|RES|INFO|CPOOL> {0}", Id);

// Idempotent: subsequent calls observe ShuttingDown and bail.
if (State == ShuttingDown)
// Idempotent: subsequent calls observe that the pool is not running and bail.
if (!IsRunning)
{
return;
}
State = ShuttingDown;
_isRunning = false;

// Stop maintenance, but let admitted requests finish. Their connections are
// destroyed by DeactivateObject when returned to this retired pool.
Expand Down
Loading
Loading