Skip to content

Commit

Permalink
Revert "SNOW-984600 Avoid closing expired sessions synchronously with…
Browse files Browse the repository at this point in the history
… obtained lock (#830)"

This reverts commit 645659a.
  • Loading branch information
sfc-gh-knozderko committed Dec 15, 2023
1 parent 49cb77d commit 8f35e5d
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 39 deletions.
8 changes: 4 additions & 4 deletions Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1780,8 +1780,8 @@ public void TestAsyncLoginTimeout()

// Should timeout after the defined timeout since retry count is infinite
Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 - delta);
// But never more than 3 sec (buffer time) after the defined timeout
Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 3) * 1000);
// But never more than 1 sec (buffer time) after the defined timeout
Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000);

Assert.AreEqual(ConnectionState.Closed, conn.State);
Assert.AreEqual(timeoutSec, conn.ConnectionTimeout);
Expand Down Expand Up @@ -1818,8 +1818,8 @@ public void TestAsyncLoginTimeoutWithRetryTimeoutLesserThanConnectionTimeout()

// Should timeout after the defined timeout since retry count is infinite
Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, retryTimeout * 1000 - delta);
// But never more than 2 sec (buffer time) after the defined timeout
Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (retryTimeout + 2) * 1000);
// But never more than 1 sec (buffer time) after the defined timeout
Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (retryTimeout + 1) * 1000);

Assert.AreEqual(ConnectionState.Closed, conn.State);
Assert.AreEqual(retryTimeout, conn.ConnectionTimeout);
Expand Down
27 changes: 0 additions & 27 deletions Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
using Snowflake.Data.Client;
using Snowflake.Data.Log;
using NUnit.Framework;
using Snowflake.Data.Core.Session;

namespace Snowflake.Data.Tests.IntegrationTests
{
Expand Down Expand Up @@ -396,31 +395,5 @@ public void TestConnectionPoolTurnOff()
SnowflakeDbConnectionPool.SetPooling(false);
//Put a breakpoint at SFSession close function, after connection pool is off, it will send close session request.
}

[Test]
public void TestCloseSessionAfterTimeout()
{
// arrange
const int SessionTimeoutSeconds = 2;
const int TimeForBackgroundSessionCloseMillis = 2000;
SnowflakeDbConnectionPool.SetTimeout(SessionTimeoutSeconds);
var conn1 = new SnowflakeDbConnection(ConnectionString);
conn1.Open();
var session = conn1.SfSession;
conn1.Close();
Assert.IsTrue(session.IsEstablished());
Thread.Sleep(SessionTimeoutSeconds * 1000); // wait until the session is expired
var conn2 = new SnowflakeDbConnection(ConnectionString);

// act
conn2.Open(); // it gets a session from the caching pool firstly closing session of conn1 in background
Thread.Sleep(TimeForBackgroundSessionCloseMillis); // wait for closing expired session

// assert
Assert.IsFalse(session.IsEstablished());

// cleanup
conn2.Close();
}
}
}
8 changes: 3 additions & 5 deletions Snowflake.Data/Core/Session/SFSession.cs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ internal async Task OpenAsync(CancellationToken cancellationToken)
internal void close()
{
// Nothing to do if the session is not open
if (!IsEstablished()) return;
if (null == sessionToken) return;

stopHeartBeatForThisSession();

Expand Down Expand Up @@ -274,7 +274,7 @@ internal void close()
internal async Task CloseAsync(CancellationToken cancellationToken)
{
// Nothing to do if the session is not open
if (!IsEstablished()) return;
if (null == sessionToken) return;

stopHeartBeatForThisSession();

Expand Down Expand Up @@ -303,8 +303,6 @@ internal async Task CloseAsync(CancellationToken cancellationToken)
sessionToken = null;
}

internal bool IsEstablished() => sessionToken != null;

internal void renewSession()
{
logger.Info("Renew the session.");
Expand Down Expand Up @@ -506,7 +504,7 @@ internal void heartbeat()
logger.Debug("heartbeat");

bool retry = false;
if (IsEstablished())
if (sessionToken != null)
{
do
{
Expand Down
6 changes: 3 additions & 3 deletions Snowflake.Data/Core/Session/SessionPool.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ private void CleanExpiredSessions()
{
if (item.IsExpired(_timeout, timeNow))
{
Task.Run(() => item.close());
_sessions.Remove(item);
item.close();

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L61 was not covered by tests
}
}
}
Expand Down Expand Up @@ -96,7 +96,7 @@ private SFSession GetIdleSession(string connStr)
long timeNow = DateTimeOffset.UtcNow.ToUnixTimeSeconds();
if (session.IsExpired(_timeout, timeNow))
{
Task.Run(() => session.close());
session.close();
i--;
}
else
Expand Down Expand Up @@ -187,7 +187,7 @@ internal void ClearAllPools()
{
foreach (SFSession session in _sessions)
{
session.close(); // it is left synchronously here because too much async tasks slows down testing
session.close();
}
_sessions.Clear();
}
Expand Down

0 comments on commit 8f35e5d

Please sign in to comment.