Skip to content

Commit

Permalink
Changes after review
Browse files Browse the repository at this point in the history
  • Loading branch information
@sfc-gh-knozderko
sfc-gh-knozderko committed Nov 24, 2023
1 parent 9be3464 commit 6dced6c
Show file tree
Hide file tree
Showing 8 changed files with 57 additions and 51 deletions.
1 change: 1 addition & 0 deletions Snowflake.Data.Tests/IntegrationTests/ConnectionMultiplePoolsIT.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ public void TestBusyAndIdleConnectionsCountedInPoolSize()
}

[Test]
[Ignore("Enable when disabling pooling in connection string enabled - SNOW-902632")]
public void TestConnectionPoolNotPossibleToDisableForAllPools()
{
// act
Expand Down
2 changes: 1 addition & 1 deletion Snowflake.Data.Tests/IntegrationTests/ConnectionSinglePoolCacheIT.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public void TestPoolContainsAtMostMaxPoolSizeConnections() // old name: TestConn
}

[Test]
public void TestConnectionPoolDisableFromHighLevel()
public void TestConnectionPoolDisableFromPoolManagerLevel()
{
// arrange
SnowflakeDbConnectionPool.SetPooling(false);
Expand Down
1 change: 1 addition & 0 deletions Snowflake.Data.Tests/UnitTests/ConnectionPoolManagerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ public void TestSetTimeoutForAllPools()
}

[Test]
[Ignore("Enable when disabling pooling in connection string enabled - SNOW-902632")]
public void TestSetPoolingDisabledForAllPoolsNotPossible()
{
// Arrange
Expand Down
35 changes: 21 additions & 14 deletions Snowflake.Data.Tests/UnitTests/Session/NonNegativeCounterTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,33 +11,33 @@ public void TestInitialZero()
{
// arrange
var counter = new NonNegativeCounter();

// act
var count = counter.Count();

// assert
Assert.AreEqual(0, count);
}

[Test]
public void TestIncrease()
{
// arrange
var counter = new NonNegativeCounter();

// act
counter.Increase();

// assert
Assert.AreEqual(1, counter.Count());

// act
counter.Increase();

// assert
Assert.AreEqual(2, counter.Count());
}


[Test]
public void TestDecrease()
Expand All @@ -46,24 +46,31 @@ public void TestDecrease()
var counter = new NonNegativeCounter();
counter.Increase();
counter.Increase();

// act
counter.Decrease();

// assert
Assert.AreEqual(1, counter.Count());

// act
counter.Decrease();

// assert
Assert.AreEqual(0, counter.Count());

}

[Test]
public void TestDecreaseDoesNotGoBelowZero()
{
// arrange
var counter = new NonNegativeCounter();

// act
counter.Decrease();

// assert
Assert.AreEqual(0, counter.Count());
}
}
}
}
2 changes: 1 addition & 1 deletion Snowflake.Data/Core/Session/ConnectionCacheManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ internal sealed class ConnectionCacheManager : IConnectionManager
public Task<SFSession> GetSessionAsync(string connectionString, SecureString password, CancellationToken cancellationToken)
=> _sessionPool.GetSessionAsync(connectionString, password, cancellationToken);
public bool AddSession(SFSession session) => _sessionPool.AddSession(session);
public void ClearAllPools() => _sessionPool.ClearAllPools();
public void ClearAllPools() => _sessionPool.ClearIdleSessions();
public void SetMaxPoolSize(int maxPoolSize) => _sessionPool.SetMaxPoolSize(maxPoolSize);
public int GetMaxPoolSize() => _sessionPool.GetMaxPoolSize();
public void SetTimeout(long connectionTimeout) => _sessionPool.SetTimeout(connectionTimeout);
Expand Down
8 changes: 4 additions & 4 deletions Snowflake.Data/Core/Session/ConnectionPoolManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public void ClearAllPools()
s_logger.Debug("ConnectionPoolManager::ClearAllPools");
foreach (var sessionPool in _pools.Values)
{
sessionPool.ClearAllPools();
sessionPool.ClearIdleSessions();
}
_pools.Clear();
}
Expand Down Expand Up @@ -102,9 +102,9 @@ public int GetCurrentPoolSize()

public bool SetPooling(bool poolingEnabled)
{
if (!poolingEnabled)
throw new Exception(
"Could not disable pooling for all connections. You could disable pooling by given connection string instead.");
// if (!poolingEnabled) // TODO: enable when disabling pooling in connection string completed SNOW-902632
// throw new Exception(
// "Could not disable pooling for all connections. You could disable pooling by given connection string instead.");
s_logger.Debug("ConnectionPoolManager::SetPooling for all pools");
return _pools.Values
.Select(pool => pool.SetPooling(poolingEnabled))
Expand Down
2 changes: 1 addition & 1 deletion Snowflake.Data/Core/Session/ICounter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ internal interface ICounter

void Decrease();
}
}
}
57 changes: 27 additions & 30 deletions Snowflake.Data/Core/Session/SessionPool.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ namespace Snowflake.Data.Core.Session
sealed class SessionPool : IDisposable
{
private static readonly SFLogger s_logger = SFLoggerFactory.GetLogger<SessionPool>();
private static readonly object s_sessionPoolLock = new object();
private readonly object _sessionPoolLock = new object();
private static ISessionFactory s_sessionFactory = new SessionFactory();

private readonly List<SFSession> _idleSessions;
Expand All @@ -32,27 +32,23 @@ sealed class SessionPool : IDisposable

private SessionPool()
{
lock (s_sessionPoolLock)
{
_idleSessions = new List<SFSession>();
_maxPoolSize = MaxPoolSize;
_timeout = Timeout;
_busySessionsCounter = new FixedZeroCounter();
}
// acquiring a lock not needed because one is already acquired in SnowflakeDbConnectionPool
_idleSessions = new List<SFSession>();
_maxPoolSize = MaxPoolSize;
_timeout = Timeout;
_busySessionsCounter = new FixedZeroCounter();
}

private SessionPool(string connectionString, SecureString password)
{
lock (s_sessionPoolLock)
{
_idleSessions = new List<SFSession>();
_maxPoolSize = MaxPoolSize;
_timeout = Timeout;
_busySessionsCounter = new NonNegativeCounter();
ConnectionString = connectionString;
Password = password;
_allowExceedMaxPoolSize = false; // TODO: SNOW-937190
}
// acquiring a lock not needed because one is already acquired in ConnectionPoolManager
_idleSessions = new List<SFSession>();
_maxPoolSize = MaxPoolSize;
_timeout = Timeout;
_busySessionsCounter = new NonNegativeCounter();
ConnectionString = connectionString;
Password = password;
_allowExceedMaxPoolSize = false; // TODO: SNOW-937190
}

internal static SessionPool CreateSessionCache() => new SessionPool();
Expand All @@ -69,7 +65,7 @@ internal static SessionPool CreateSessionPool(string connectionString, SecureStr

public void Dispose()
{
ClearAllPools();
ClearIdleSessions();

Check warning on line 68 in Snowflake.Data/Core/Session/SessionPool.cs

View check run for this annotation

Codecov / codecov/patch

Snowflake.Data/Core/Session/SessionPool.cs#L68 

Added line #L68 was not covered by tests
}

internal static ISessionFactory SessionFactory
Expand All @@ -80,7 +76,7 @@ internal static ISessionFactory SessionFactory
private void CleanExpiredSessions()
{
s_logger.Debug("SessionPool::CleanExpiredSessions");
lock (s_sessionPoolLock)
lock (_sessionPoolLock)
{
long timeNow = DateTimeOffset.UtcNow.ToUnixTimeSeconds();

Expand Down Expand Up @@ -121,7 +117,7 @@ internal Task<SFSession> GetSessionAsync(CancellationToken cancellationToken) =>
private SFSession GetIdleSession(string connStr)
{
s_logger.Debug("SessionPool::GetIdleSession");
lock (s_sessionPoolLock)
lock (_sessionPoolLock)
{
for (int i = 0; i < _idleSessions.Count; i++)
{
Expand Down Expand Up @@ -153,9 +149,9 @@ private SFSession NewSession(String connectionString, SecureString password)
try
{
var session = s_sessionFactory.NewSession(connectionString, password);
session.Open();
if (_pooling)
_busySessionsCounter.Increase();
session.Open();
return session;
}
catch (Exception e)
Expand All @@ -175,8 +171,6 @@ private Task<SFSession> NewSessionAsync(String connectionString, SecureString pa
{
s_logger.Debug("SessionPool::NewSessionAsync");
var session = s_sessionFactory.NewSession(connectionString, password);
if (_pooling)
_busySessionsCounter.Increase();
return session
.OpenAsync(cancellationToken)
.ContinueWith(previousTask =>
Expand All @@ -190,6 +184,9 @@ private Task<SFSession> NewSessionAsync(String connectionString, SecureString pa
SFError.INTERNAL_ERROR,
"Failure while opening session async");

if (_pooling)
_busySessionsCounter.Increase();

return session;
}, TaskContinuationOptions.NotOnCanceled);
}
Expand All @@ -202,14 +199,14 @@ internal bool AddSession(SFSession session)
long timeNow = DateTimeOffset.UtcNow.ToUnixTimeSeconds();
if (session.IsNotOpen() || session.IsExpired(_timeout, timeNow))
{
lock (s_sessionPoolLock)
lock (_sessionPoolLock)
{
_busySessionsCounter.Decrease();
}
return false;
}

lock (s_sessionPoolLock)
lock (_sessionPoolLock)
{
_busySessionsCounter.Decrease();
CleanExpiredSessions();
Expand All @@ -225,10 +222,10 @@ internal bool AddSession(SFSession session)
}
}

internal void ClearAllPools()
internal void ClearIdleSessions()
{
s_logger.Debug("SessionPool::ClearAllPools");
lock (s_sessionPoolLock)
s_logger.Debug("SessionPool::ClearIdleSessions");
lock (_sessionPoolLock)
{
foreach (SFSession session in _idleSessions)
{
Expand Down Expand Up @@ -281,7 +278,7 @@ public bool SetPooling(bool isEnable)
_pooling = isEnable;
if (!_pooling)
{
ClearAllPools();
ClearIdleSessions();
}
return true;
}
Expand Down

0 comments on commit 6dced6c

Please sign in to comment.