From 3def3c38faf3e41bc20cdcb2c0a51f7858fb3691 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Mon, 11 Dec 2023 10:52:44 +0100 Subject: [PATCH 1/8] SNOW-984600 Avoid closing sessions synchronously with obtained lock --- Snowflake.Data/Core/Session/SessionPool.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Snowflake.Data/Core/Session/SessionPool.cs b/Snowflake.Data/Core/Session/SessionPool.cs index a7eae7726..439afbd48 100644 --- a/Snowflake.Data/Core/Session/SessionPool.cs +++ b/Snowflake.Data/Core/Session/SessionPool.cs @@ -57,8 +57,8 @@ private void CleanExpiredSessions() { if (item.IsExpired(_timeout, timeNow)) { + Task.Run(() => item.close()); _sessions.Remove(item); - item.close(); } } } @@ -96,7 +96,7 @@ private SFSession GetIdleSession(string connStr) long timeNow = DateTimeOffset.UtcNow.ToUnixTimeSeconds(); if (session.IsExpired(_timeout, timeNow)) { - session.close(); + Task.Run(() => session.close()); i--; } else @@ -187,7 +187,7 @@ internal void ClearAllPools() { foreach (SFSession session in _sessions) { - session.close(); + Task.Run(() => session.close()); } _sessions.Clear(); } From 3a13a6a51d9ba1ecd919367de4c9dc1411ec0ac9 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Mon, 11 Dec 2023 13:15:05 +0100 Subject: [PATCH 2/8] add tests --- .../IntegrationTests/SFConnectionPoolIT.cs | 27 +++++++++++++++++++ Snowflake.Data/Core/Session/SFSession.cs | 8 +++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs index 5c4529225..7df31bc71 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs @@ -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 { @@ -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); + 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); + + // assert + Assert.IsFalse(session.IsEstablished()); + + // cleanup + conn2.Close(); + } } } diff --git a/Snowflake.Data/Core/Session/SFSession.cs b/Snowflake.Data/Core/Session/SFSession.cs index d153ffff4..cb9e4f0fe 100755 --- a/Snowflake.Data/Core/Session/SFSession.cs +++ b/Snowflake.Data/Core/Session/SFSession.cs @@ -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(); @@ -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(); @@ -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."); @@ -504,7 +506,7 @@ internal void heartbeat() logger.Debug("heartbeat"); bool retry = false; - if (sessionToken != null) + if (IsEstablished()) { do { From 8051b516e66f666a8aa18a63bed8ff0b836881fa Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Mon, 11 Dec 2023 13:46:48 +0100 Subject: [PATCH 3/8] increase delay tolerance for async login in tests --- Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index eea852af2..d15e3ce2f 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -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 2 sec (buffer time) after the defined timeout + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 2) * 1000); Assert.AreEqual(ConnectionState.Closed, conn.State); Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); @@ -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); From 129f0778c034e788ba80f291fdbc86f0f321b74d Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Mon, 11 Dec 2023 14:58:27 +0100 Subject: [PATCH 4/8] increase delay tolarance more --- Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index d15e3ce2f..c13cd684b 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -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 2 sec (buffer time) after the defined timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 2) * 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); From c9031cfa4d2d554ebe474b25dfd8a2b09b2578ca Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Mon, 11 Dec 2023 18:12:26 +0100 Subject: [PATCH 5/8] ClearAllPools with synchronous session closing --- Snowflake.Data/Core/Session/SessionPool.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Snowflake.Data/Core/Session/SessionPool.cs b/Snowflake.Data/Core/Session/SessionPool.cs index 439afbd48..14ad70848 100644 --- a/Snowflake.Data/Core/Session/SessionPool.cs +++ b/Snowflake.Data/Core/Session/SessionPool.cs @@ -187,7 +187,7 @@ internal void ClearAllPools() { foreach (SFSession session in _sessions) { - Task.Run(() => session.close()); + session.close(); // it is left synchronously here because too much async tasks slows down testing } _sessions.Clear(); } From b3e9a619ca40bf7966b92b0ed47d409195b6f658 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Tue, 12 Dec 2023 10:47:29 +0100 Subject: [PATCH 6/8] try to decrease tolarance --- Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index c13cd684b..d15e3ce2f 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -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 2 sec (buffer time) after the defined timeout + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 2) * 1000); Assert.AreEqual(ConnectionState.Closed, conn.State); Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); From 45970970653f18516f2839513073e8c07678e322 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Tue, 12 Dec 2023 11:13:18 +0100 Subject: [PATCH 7/8] Revert "try to decrease tolarance" This reverts commit b3e9a619ca40bf7966b92b0ed47d409195b6f658. --- Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index d15e3ce2f..c13cd684b 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -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 2 sec (buffer time) after the defined timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 2) * 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); From 6be094a9ca69d299659c666f44389f86fc56e86e Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Wed, 13 Dec 2023 12:00:27 +0100 Subject: [PATCH 8/8] add comments --- Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs index 7df31bc71..2a403521e 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionPoolIT.cs @@ -409,12 +409,12 @@ public void TestCloseSessionAfterTimeout() var session = conn1.SfSession; conn1.Close(); Assert.IsTrue(session.IsEstablished()); - Thread.Sleep(SessionTimeoutSeconds * 1000); + 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); + Thread.Sleep(TimeForBackgroundSessionCloseMillis); // wait for closing expired session // assert Assert.IsFalse(session.IsEstablished());