Skip to content
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

SNOW-984600 Avoid closing expired sessions synchronously with obtained lock #830

Merged
merged 8 commits into from
Dec 13, 2023
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 1 sec (buffer time) after the defined timeout
Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000);
// But never more than 3 sec (buffer time) after the defined timeout
Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 3) * 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 1 sec (buffer time) after the defined timeout
Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (retryTimeout + 1) * 1000);
// But never more than 2 sec (buffer time) after the defined timeout
Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (retryTimeout + 2) * 1000);

Assert.AreEqual(ConnectionState.Closed, conn.State);
Assert.AreEqual(retryTimeout, conn.ConnectionTimeout);
Expand Down
27 changes: 27 additions & 0 deletions Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
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 @@ -395,5 +396,31 @@ 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);
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved

// 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: 5 additions & 3 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 (null == sessionToken) return;
if (!IsEstablished()) 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 (null == sessionToken) return;
if (!IsEstablished()) return;

stopHeartBeatForThisSession();

Expand Down Expand Up @@ -303,6 +303,8 @@ 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 @@ -504,7 +506,7 @@ internal void heartbeat()
logger.Debug("heartbeat");

bool retry = false;
if (sessionToken != null)
if (IsEstablished())
{
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 @@
{
if (item.IsExpired(_timeout, timeNow))
{
Task.Run(() => item.close());

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

View check run for this annotation

Codecov / codecov/patch

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

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