From 68e4dc1b84b33e5ad301828e3ef11b17f4d06ab5 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 26 Oct 2023 11:12:59 -0700 Subject: [PATCH 01/21] SNOW-878067: Add retry strategy for supported endpoints --- Snowflake.Data/Core/HttpUtil.cs | 74 ++++++++++++++++--- Snowflake.Data/Core/RestRequest.cs | 11 +++ Snowflake.Data/Core/Session/SFSession.cs | 4 +- .../Core/Session/SFSessionProperty.cs | 8 +- 4 files changed, 86 insertions(+), 11 deletions(-) diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 77d3807ff..b56a62d6f 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -13,6 +13,7 @@ using System.Web; using System.Security.Authentication; using System.Runtime.InteropServices; +using System.Linq; namespace Snowflake.Data.Core { @@ -73,8 +74,18 @@ public HttpClientConfig( public sealed class HttpUtil { static internal readonly int MAX_BACKOFF = 16; + private static readonly int s_baseBackOffTime = 1; + private static readonly int s_baseLoginBackOffTime = 4; + private static readonly int s_exponentialFactor = 2; private static readonly SFLogger logger = SFLoggerFactory.GetLogger(); + private static readonly List s_supportedEndpointsForRetryPolicy = new List + { + RestPath.SF_LOGIN_PATH, + RestPath.SF_AUTHENTICATOR_REQUEST_PATH, + RestPath.SF_TOKEN_REQUEST_PATH + }; + private HttpUtil() { // This value is used by AWS SDK and can cause deadlock, @@ -332,7 +343,8 @@ protected override async Task SendAsync(HttpRequestMessage CancellationToken cancellationToken) { HttpResponseMessage response = null; - int backOffInSec = 1; + bool isLoginRequest = IsLoginEndpoint(requestMessage.RequestUri.AbsolutePath); + int backOffInSec = isLoginRequest ? s_baseLoginBackOffTime : s_baseBackOffTime; int totalRetryTime = 0; ServicePoint p = ServicePointManager.FindServicePoint(requestMessage.RequestUri); @@ -438,20 +450,29 @@ protected override async Task SendAsync(HttpRequestMessage requestMessage.RequestUri = updater.Update(errorReason); + // Add jitter for login requests + if (isLoginRequest) + { + backOffInSec += (int)GetJitter(backOffInSec); + } + + if ((restTimeout.TotalSeconds > 0) && (totalRetryTime + backOffInSec > restTimeout.TotalSeconds)) + { + // No need to wait more than necessary if it can be avoided. + // If the rest timeout will be reached before the next back-off, + // then use the remaining connection timeout + backOffInSec = Math.Min(backOffInSec, (int)restTimeout.TotalSeconds - totalRetryTime); + } + logger.Debug($"Sleep {backOffInSec} seconds and then retry the request, retryCount: {retryCount}"); await Task.Delay(TimeSpan.FromSeconds(backOffInSec), cancellationToken).ConfigureAwait(false); totalRetryTime += backOffInSec; - // Set next backoff time - backOffInSec = backOffInSec >= MAX_BACKOFF ? - MAX_BACKOFF : backOffInSec * 2; - if ((restTimeout.TotalSeconds > 0) && (totalRetryTime + backOffInSec > restTimeout.TotalSeconds)) + // Set next backoff time + if (isLoginRequest || !isLoginRequest && backOffInSec < MAX_BACKOFF) { - // No need to wait more than necessary if it can be avoided. - // If the rest timeout will be reached before the next back-off, - // use a smaller one to give the Rest request a chance to timeout early - backOffInSec = Math.Max(1, (int)restTimeout.TotalSeconds - totalRetryTime - 1); + backOffInSec *= s_exponentialFactor; } } } @@ -474,6 +495,41 @@ static public bool isRetryableHTTPCode(int statusCode, bool forceRetryOn404) // Too many requests (statusCode == 429); } + + /// + /// Get the jitter amount based on current wait time. + /// + /// The current retry backoff time. + /// The new jitter amount. + static internal double GetJitter(double curWaitTime) + { + double multiplicationFactor = ChooseRandom(-1, 1); + double jitterAmount = 0.5 * curWaitTime * multiplicationFactor; + return jitterAmount; + } + + /// + /// Randomly generates a number between a given range. + /// + /// The min range (inclusive). + /// The max range (inclusive). + /// The random number. + static double ChooseRandom(int min, int max) + { + var next = new Random().NextDouble(); + + return min + (next * (max - min)); + } + + /// + /// Checks if the endpoint is a login request. + /// + /// The endpoint to check. + /// True if the endpoint is a login request, false otherwise. + static internal bool IsLoginEndpoint(string endpoint) + { + return null != s_supportedEndpointsForRetryPolicy.FirstOrDefault(ep => endpoint.Equals(ep)); + } } } diff --git a/Snowflake.Data/Core/RestRequest.cs b/Snowflake.Data/Core/RestRequest.cs index 77a88323b..bc4ec8d21 100644 --- a/Snowflake.Data/Core/RestRequest.cs +++ b/Snowflake.Data/Core/RestRequest.cs @@ -108,6 +108,9 @@ internal class SFRestRequest : BaseRestRequest, IRestRequest private const string SF_AUTHORIZATION_HEADER = "Authorization"; private const string SF_SERVICE_NAME_HEADER = "X-Snowflake-Service"; + private const string ClientAppId = "CLIENT_APP_ID"; + private const string ClientAppVersion = "CLIENT_APP_VERSION"; + internal SFRestRequest() : base() { RestTimeout = TimeSpan.FromSeconds(DEFAULT_REST_RETRY_SECONDS_TIMEOUT); @@ -124,6 +127,8 @@ internal SFRestRequest() : base() internal bool isPutGet { get; set; } + internal bool _isLogin { get; set; } + public override string ToString() { return String.Format("SFRestRequest {{url: {0}, request body: {1} }}", Url.ToString(), @@ -158,6 +163,12 @@ HttpRequestMessage IRestRequest.ToRequestMessage(HttpMethod method) message.Headers.Accept.Add(applicationSnowflake); } + if (_isLogin) + { + message.Headers.Add(ClientAppId, SFEnvironment.DriverName); + message.Headers.Add(ClientAppVersion, SFEnvironment.DriverVersion); + } + message.Headers.UserAgent.Add(new ProductInfoHeaderValue(SFEnvironment.DriverName, SFEnvironment.DriverVersion)); message.Headers.UserAgent.Add(new ProductInfoHeaderValue(osInfo)); message.Headers.UserAgent.Add(new ProductInfoHeaderValue( diff --git a/Snowflake.Data/Core/Session/SFSession.cs b/Snowflake.Data/Core/Session/SFSession.cs index 49586c08d..e5a570780 100755 --- a/Snowflake.Data/Core/Session/SFSession.cs +++ b/Snowflake.Data/Core/Session/SFSession.cs @@ -362,7 +362,8 @@ internal SFRestRequest getRenewSessionRequest() jsonBody = postBody, Url = BuildUri(RestPath.SF_TOKEN_REQUEST_PATH, parameters), authorizationToken = string.Format(SF_AUTHORIZATION_SNOWFLAKE_FMT, masterToken), - RestTimeout = Timeout.InfiniteTimeSpan + RestTimeout = Timeout.InfiniteTimeSpan, + _isLogin = true }; } @@ -374,6 +375,7 @@ internal SFRestRequest BuildTimeoutRestRequest(Uri uri, Object body) Url = uri, authorizationToken = SF_AUTHORIZATION_BASIC, RestTimeout = connectionTimeout, + _isLogin = true }; } diff --git a/Snowflake.Data/Core/Session/SFSessionProperty.cs b/Snowflake.Data/Core/Session/SFSessionProperty.cs index ade4d516c..acee9c2fd 100755 --- a/Snowflake.Data/Core/Session/SFSessionProperty.cs +++ b/Snowflake.Data/Core/Session/SFSessionProperty.cs @@ -36,7 +36,7 @@ internal enum SFSessionProperty USER, [SFSessionPropertyAttr(required = false)] WAREHOUSE, - [SFSessionPropertyAttr(required = false, defaultValue = "120")] + [SFSessionPropertyAttr(required = false, defaultValue = "300")] CONNECTION_TIMEOUT, [SFSessionPropertyAttr(required = false, defaultValue = "snowflake")] AUTHENTICATOR, @@ -100,6 +100,7 @@ class SFSessionPropertyAttr : Attribute class SFSessionProperties : Dictionary { static private SFLogger logger = SFLoggerFactory.GetLogger(); + internal static readonly int s_connectionTimeoutDefault = 300; // Connection string properties to obfuscate in the log static private List secretProps = @@ -266,6 +267,11 @@ internal static SFSessionProperties parseConnectionString(String connectionStrin // passed on for account_name properties[SFSessionProperty.ACCOUNT] = properties[SFSessionProperty.ACCOUNT].Split('.')[0]; + // The login timeout can only be increased + if (int.Parse(properties[SFSessionProperty.CONNECTION_TIMEOUT]) < s_connectionTimeoutDefault) + { + properties[SFSessionProperty.CONNECTION_TIMEOUT] = s_connectionTimeoutDefault.ToString(); + } return properties; } From b7dc6bd3d731cf4c56de4883d0adc44d3f9e5ed9 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 26 Oct 2023 15:56:25 -0700 Subject: [PATCH 02/21] SNOW-878067: Add tests for retry strategy --- .../IntegrationTests/SFConnectionIT.cs | 72 +++++++++-------- .../IntegrationTests/SFDbCommandIT.cs | 78 +++++++++++++++++++ .../Mock/MockRetryUntilRestTimeout.cs | 11 ++- .../UnitTests/HttpUtilTest.cs | 44 +++++++++++ .../UnitTests/SFSessionPropertyTest.cs | 2 +- .../Session/SFHttpClientPropertiesTest.cs | 35 ++++++--- 6 files changed, 197 insertions(+), 45 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index ee1442b56..29634e214 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -32,7 +32,7 @@ public void TestBasicConnection() conn.Open(); Assert.AreEqual(ConnectionState.Open, conn.State); - Assert.AreEqual(120, conn.ConnectionTimeout); + Assert.AreEqual(SFSessionProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout); // Data source is empty string for now Assert.AreEqual("", ((SnowflakeDbConnection)conn).DataSource); @@ -379,7 +379,8 @@ public void TestLoginTimeout() { using (IDbConnection conn = new MockSnowflakeDbConnection()) { - int timeoutSec = 5; + // Connection timeout can only be increased + int timeoutSec = 5 + SFSessionProperties.s_connectionTimeoutDefault; string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0}", timeoutSec); @@ -395,17 +396,19 @@ public void TestLoginTimeout() } catch (AggregateException e) { - Assert.AreEqual(SFError.REQUEST_TIMEOUT.GetAttribute().errorCode, + // Jitter can cause the request to reach max number of retries before reaching the timeout + Assert.IsTrue(e.InnerException is TaskCanceledException || + SFError.REQUEST_TIMEOUT.GetAttribute().errorCode == ((SnowflakeDbException)e.InnerException).ErrorCode); } stopwatch.Stop(); int detla = 10; //in case server time slower. - //Should timeout before the default timeout (120 sec) * 1000 - Assert.Less(stopwatch.ElapsedMilliseconds, 120 * 1000); - // Should timeout after the defined connection timeout - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 - detla); - Assert.AreEqual(5, conn.ConnectionTimeout); + // Should timeout after the minimum possible timeout with jitter: 14 seconds (50% of 4 * 2) after 7 retries + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 14 * 1000); + // Should timeout before the defined connection timeout + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + detla); + Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); } } } @@ -426,15 +429,20 @@ public void TestLoginWithMaxRetryReached() conn.Open(); Assert.Fail(); } - catch + catch (Exception e) { + // Jitter can cause the request to reach max number of retries before reaching the timeout + Assert.IsTrue(e.InnerException is TaskCanceledException || + SFError.REQUEST_TIMEOUT.GetAttribute().errorCode == + ((SnowflakeDbException)e.InnerException).ErrorCode); } stopwatch.Stop(); - // retry 5 times with backoff 1, 2, 4, 8, 16 seconds - // but should not delay more than another 16 seconds - Assert.Less(stopwatch.ElapsedMilliseconds, 51 * 1000); - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 30 * 1000); + // retry 5 times with starting backoff of 4 seconds with jitter + // but should not delay more than the default connection timeout of 300 seconds + // and should not take less time than 10 (with max negative jitter for 5 retries) seconds + Assert.Less(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000); + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 10 * 1000); } } @@ -1665,8 +1673,8 @@ public void TestCancelLoginBeforeTimeout() { using (var conn = new MockSnowflakeDbConnection()) { - // No timeout - int timeoutSec = 0; + // Add 10 seconds to default timeout + int timeoutSec = 10 + SFSessionProperties.s_connectionTimeoutDefault; string infiniteLoginTimeOut = String.Format(ConnectionString + ";connection_timeout={0};maxHttpRetries=0", timeoutSec); @@ -1678,12 +1686,11 @@ public void TestCancelLoginBeforeTimeout() //Assert.AreEqual(120, conn.ConnectionTimeout); CancellationTokenSource connectionCancelToken = new CancellationTokenSource(); - Task connectTask = conn.OpenAsync(connectionCancelToken.Token); + Task connectTask = conn.OpenAsync(connectionCancelToken.Token); + + // Sleep for the specified timeout minus 10 seconds (just before the actual timeout is reached) + Thread.Sleep((timeoutSec - 10) * 1000); - // Sleep for 130 sec (more than the default connection timeout and the httpclient - // timeout to make sure there are no false positive ) - Thread.Sleep(130*1000); - Assert.AreEqual(ConnectionState.Connecting, conn.State); // Cancel the connection because it will never succeed since there is no @@ -1704,7 +1711,7 @@ public void TestCancelLoginBeforeTimeout() } Assert.AreEqual(ConnectionState.Closed, conn.State); - Assert.AreEqual(0, conn.ConnectionTimeout); + Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); } } @@ -1715,7 +1722,8 @@ public void TestAsyncLoginTimeout() { using (var conn = new MockSnowflakeDbConnection()) { - int timeoutSec = 5; + // Connection timeout can only be increased + int timeoutSec = 5 + SFSessionProperties.s_connectionTimeoutDefault; string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0}", timeoutSec); conn.ConnectionString = loginTimeOut5sec; @@ -1737,13 +1745,13 @@ public void TestAsyncLoginTimeout() stopwatch.Stop(); int detla = 10; //in case server time slower. - // Should timeout after 5sec - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 5 * 1000 - detla); - // But never more than 1 sec (max backoff) after the default timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (6) * 1000); + // Should timeout after the minimum possible timeout with jitter: 14 seconds (50% of 4 * 2) after 7 retries + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 14 * 1000); + // Should timeout before the defined connection timeout + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + detla); Assert.AreEqual(ConnectionState.Closed, conn.State); - Assert.AreEqual(5, conn.ConnectionTimeout); + Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); } } } @@ -1772,13 +1780,13 @@ public void TestAsyncDefaultLoginTimeout() stopwatch.Stop(); int detla = 10; //in case server time slower. - // Should timeout after the default timeout (120 sec) - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 120 * 1000 - detla); - // But never more than 16 sec (max backoff) after the default timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (120 + 16) * 1000); + // Should timeout after the default timeout (300 sec) + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000 - detla); + // But never more because there's no connection timeout remaining + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000 + detla); Assert.AreEqual(ConnectionState.Closed, conn.State); - Assert.AreEqual(120, conn.ConnectionTimeout); + Assert.AreEqual(SFSessionProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout); } } diff --git a/Snowflake.Data.Tests/IntegrationTests/SFDbCommandIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFDbCommandIT.cs index 111094b74..58bf90b46 100755 --- a/Snowflake.Data.Tests/IntegrationTests/SFDbCommandIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFDbCommandIT.cs @@ -16,6 +16,8 @@ namespace Snowflake.Data.Tests.IntegrationTests using System.Diagnostics; using System.Collections.Generic; using System.Globalization; + using Snowflake.Data.Tests.Mock; + using Snowflake.Data.Core; [TestFixture] class SFDbCommandITAsync : SFBaseTestAsync @@ -90,6 +92,44 @@ public void TestCancelExecuteAsync() } } + [Test] + public void TestExecuteAsyncWithMaxRetryReached() + { + var mockRestRequester = new MockRetryUntilRestTimeoutRestRequester() + { + _forceTimeoutForNonLoginRequestsOnly = true + }; + + using (DbConnection conn = new MockSnowflakeDbConnection(mockRestRequester)) + { + string maxRetryConnStr = ConnectionString + "maxHttpRetries=5"; + + conn.ConnectionString = maxRetryConnStr; + conn.Open(); + + Stopwatch stopwatch = Stopwatch.StartNew(); + try + { + using (DbCommand command = conn.CreateCommand()) + { + command.CommandText = "select 1;"; + Task t = command.ExecuteScalarAsync(); + t.Wait(); + } + Assert.Fail(); + } + catch (Exception e) + { + Assert.IsInstanceOf(e.InnerException); + } + stopwatch.Stop(); + + // retry 5 times with backoff 1, 2, 4, 8, 16 seconds + // but should not delay more than another 16 seconds + Assert.Less(stopwatch.ElapsedMilliseconds, 51 * 1000); + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 30 * 1000); + } + } } [TestFixture] @@ -544,6 +584,44 @@ public void TestExecuteScalarNull() } } + [Test] + public void TestExecuteWithMaxRetryReached() + { + var mockRestRequester = new MockRetryUntilRestTimeoutRestRequester() + { + _forceTimeoutForNonLoginRequestsOnly = true + }; + + using (IDbConnection conn = new MockSnowflakeDbConnection(mockRestRequester)) + { + string maxRetryConnStr = ConnectionString + "maxHttpRetries=5"; + + conn.ConnectionString = maxRetryConnStr; + conn.Open(); + + Stopwatch stopwatch = Stopwatch.StartNew(); + try + { + using (IDbCommand command = conn.CreateCommand()) + { + command.CommandText = "select 1;"; + command.ExecuteScalar(); + } + Assert.Fail(); + } + catch (Exception e) + { + Assert.IsInstanceOf(e.InnerException); + } + stopwatch.Stop(); + + // retry 5 times with backoff 1, 2, 4, 8, 16 seconds + // but should not delay more than another 16 seconds + Assert.Less(stopwatch.ElapsedMilliseconds, 51 * 1000); + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 30 * 1000); + } + } + [Test] public void TestCreateCommandBeforeOpeningConnection() { diff --git a/Snowflake.Data.Tests/Mock/MockRetryUntilRestTimeout.cs b/Snowflake.Data.Tests/Mock/MockRetryUntilRestTimeout.cs index c9f9a665a..17b0ffcfc 100644 --- a/Snowflake.Data.Tests/Mock/MockRetryUntilRestTimeout.cs +++ b/Snowflake.Data.Tests/Mock/MockRetryUntilRestTimeout.cs @@ -15,6 +15,8 @@ namespace Snowflake.Data.Tests.Mock class MockRetryUntilRestTimeoutRestRequester : RestRequester, IMockRestRequester { + internal bool _forceTimeoutForNonLoginRequestsOnly = false; + public MockRetryUntilRestTimeoutRestRequester() : base(null) { // Does nothing @@ -30,8 +32,13 @@ protected override async Task SendAsync(HttpRequestMessage CancellationToken externalCancellationToken, string sid = "") { - // Override the http timeout and set to 1ms to force all http request to timeout and retry - message.Properties[BaseRestRequest.HTTP_REQUEST_TIMEOUT_KEY] = TimeSpan.FromTicks(0); + if (!_forceTimeoutForNonLoginRequestsOnly || + _forceTimeoutForNonLoginRequestsOnly && !message.RequestUri.AbsolutePath.Equals(RestPath.SF_LOGIN_PATH)) + { + // Override the http timeout and set to 1ms to force all http request to timeout and retry + message.Properties[BaseRestRequest.HTTP_REQUEST_TIMEOUT_KEY] = TimeSpan.FromTicks(0); + } + return await (base.SendAsync(message, restTimeout, externalCancellationToken).ConfigureAwait(false)); } } diff --git a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs index aad4fe245..bf8063a7f 100644 --- a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs +++ b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs @@ -11,6 +11,7 @@ namespace Snowflake.Data.Tests.UnitTests using RichardSzalay.MockHttp; using System.Threading.Tasks; using System.Net; + using System; [TestFixture] class HttpUtilTest @@ -39,6 +40,49 @@ public async Task TestIsRetryableHTTPCode(HttpStatusCode statusCode, bool forceR Assert.AreEqual(expectedIsRetryable, actualIsRetryable); } + // Parameters: request url, expected value + [TestCase("https://test.snowflakecomputing.com/session/v1/login-request", true)] + [TestCase("https://test.snowflakecomputing.com/session/authenticator-request", true)] + [TestCase("https://test.snowflakecomputing.com/session/token-request", true)] + [TestCase("https://test.snowflakecomputing.com/queries/v1/query-request", false)] + [Test] + public void TestIsLoginUrl(string requestUrl, bool expectedIsLoginEndpoint) + { + // given + var uri = new Uri(requestUrl); + + // when + bool isLoginEndpoint = HttpUtil.IsLoginEndpoint(uri.AbsolutePath); + + // then + Assert.AreEqual(expectedIsLoginEndpoint, isLoginEndpoint); + } + + // Parameters: time in seconds + [TestCase(4)] + [TestCase(8)] + [TestCase(16)] + [TestCase(32)] + [TestCase(64)] + [TestCase(128)] + [Test] + public void TestGetJitter(int seconds) + { + // given + var lowerBound = -(seconds / 2); + var upperBound = seconds / 2; + + double jitter; + for (var i = 0; i < 10; i++) + { + // when + jitter = HttpUtil.GetJitter(seconds); + + // then + Assert.IsTrue(jitter >= lowerBound && jitter <= upperBound); + } + } + [Test] public void ShouldCreateHttpClientHandlerWithProxy() { diff --git a/Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs b/Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs index cdf6296e2..d9856643d 100644 --- a/Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs +++ b/Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs @@ -48,7 +48,7 @@ public static IEnumerable ConnectionStringTestCases() string defHost = "testaccount.snowflakecomputing.com"; string defAuthenticator = "snowflake"; string defScheme = "https"; - string defConnectionTimeout = "120"; + string defConnectionTimeout = "300"; string defBrowserResponseTime = "120"; string defPassword = "123"; string defPort = "443"; diff --git a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs index ea2b5d336..709fc17bb 100644 --- a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs @@ -124,7 +124,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = BaseRestRequest.DEFAULT_REST_RETRY_SECONDS_TIMEOUT, + timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, @@ -138,7 +138,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = false, clientSessionKeepAlive = false, - timeoutInSec = BaseRestRequest.DEFAULT_REST_RETRY_SECONDS_TIMEOUT, + timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, @@ -152,21 +152,35 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = true, - timeoutInSec = BaseRestRequest.DEFAULT_REST_RETRY_SECONDS_TIMEOUT, + timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, maxHttpRetries = 7 } }; - var propertiesWithTimeoutChanged = new PropertiesTestCase() + var propertiesWithTimeoutChangedToAValueAbove300 = new PropertiesTestCase() + { + conectionString = "account=test;user=test;password=test;connection_timeout=600", + expectedProperties = new SFSessionHttpClientProperties() + { + validateDefaultParameters = true, + clientSessionKeepAlive = false, + timeoutInSec = 600, + insecureMode = false, + disableRetry = false, + forceRetryOn404 = false, + maxHttpRetries = 7 + } + }; + var propertiesWithTimeoutChangedToAValueBelow300 = new PropertiesTestCase() { conectionString = "account=test;user=test;password=test;connection_timeout=15", expectedProperties = new SFSessionHttpClientProperties() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = 15, + timeoutInSec = 300, insecureMode = false, disableRetry = false, forceRetryOn404 = false, @@ -180,7 +194,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = BaseRestRequest.DEFAULT_REST_RETRY_SECONDS_TIMEOUT, + timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, insecureMode = true, disableRetry = false, forceRetryOn404 = false, @@ -194,7 +208,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = BaseRestRequest.DEFAULT_REST_RETRY_SECONDS_TIMEOUT, + timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = true, forceRetryOn404 = false, @@ -208,7 +222,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = BaseRestRequest.DEFAULT_REST_RETRY_SECONDS_TIMEOUT, + timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = true, @@ -222,7 +236,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = BaseRestRequest.DEFAULT_REST_RETRY_SECONDS_TIMEOUT, + timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, @@ -234,7 +248,8 @@ public static IEnumerable PropertiesProvider() defaultProperties, propertiesWithValidateDefaultParametersChanged, propertiesWithClientSessionKeepAliveChanged, - propertiesWithTimeoutChanged, + propertiesWithTimeoutChangedToAValueAbove300, + propertiesWithTimeoutChangedToAValueBelow300, propertiesWithInsecureModeChanged, propertiesWithDisableRetryChanged, propertiesWithForceRetryOn404Changed, From 5d7f9e15e51a0e8eed1a1bbeddb3970b7d9c1641 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 26 Oct 2023 16:22:45 -0700 Subject: [PATCH 03/21] SNOW-878067: Fix variable name --- Snowflake.Data/Core/Session/SessionPool.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Snowflake.Data/Core/Session/SessionPool.cs b/Snowflake.Data/Core/Session/SessionPool.cs index 1df54811c..a7eae7726 100644 --- a/Snowflake.Data/Core/Session/SessionPool.cs +++ b/Snowflake.Data/Core/Session/SessionPool.cs @@ -196,11 +196,11 @@ internal void ClearAllPools() internal async void ClearAllPoolsAsync() { s_logger.Debug("SessionPool::ClearAllPoolsAsync"); - foreach (SFSession session in _sessionPool) + foreach (SFSession session in _sessions) { await session.CloseAsync(CancellationToken.None).ConfigureAwait(false); } - _sessionPool.Clear(); + _sessions.Clear(); } public void SetMaxPoolSize(int size) From 77611e6facb028576fb51b604ae22caa1bfb3694 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 26 Oct 2023 16:36:03 -0700 Subject: [PATCH 04/21] SNOW-878067: Increase delta for async timeout test --- Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 29634e214..d58bfe53c 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -1778,12 +1778,12 @@ public void TestAsyncDefaultLoginTimeout() ((SnowflakeDbException)e.InnerException).ErrorCode); } stopwatch.Stop(); - int detla = 10; //in case server time slower. + int delta = 100; // in case server time slower. // Should timeout after the default timeout (300 sec) - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000 - detla); + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000 - delta); // But never more because there's no connection timeout remaining - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000 + detla); + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000 + delta); Assert.AreEqual(ConnectionState.Closed, conn.State); Assert.AreEqual(SFSessionProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout); From 9639f4346d0aa453af80f04fcd000141ca669451 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 26 Oct 2023 16:40:52 -0700 Subject: [PATCH 05/21] SNOW-878067: Increase delta for async timeout test --- 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 d58bfe53c..ee58ee260 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -1743,12 +1743,12 @@ public void TestAsyncLoginTimeout() } stopwatch.Stop(); - int detla = 10; //in case server time slower. + int delta = 100; // in case server time slower. // Should timeout after the minimum possible timeout with jitter: 14 seconds (50% of 4 * 2) after 7 retries Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 14 * 1000); // Should timeout before the defined connection timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + detla); + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + delta); Assert.AreEqual(ConnectionState.Closed, conn.State); Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); From a4411228b28c10c38477719d366b6cf5f50f0c8d Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 26 Oct 2023 16:57:01 -0700 Subject: [PATCH 06/21] SNOW-878067: Increase delta for timeout test --- 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 ee58ee260..a8ba78e50 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -402,12 +402,12 @@ public void TestLoginTimeout() ((SnowflakeDbException)e.InnerException).ErrorCode); } stopwatch.Stop(); - int detla = 10; //in case server time slower. + int delta = 100; // in case server time slower. // Should timeout after the minimum possible timeout with jitter: 14 seconds (50% of 4 * 2) after 7 retries Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 14 * 1000); // Should timeout before the defined connection timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + detla); + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + delta); Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); } } From ddd961e8d47fb254ed2bd35b54e80825392e6b83 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 26 Oct 2023 17:38:12 -0700 Subject: [PATCH 07/21] SNOW-878067: Refactor variable for connection timeout --- .../IntegrationTests/SFConnectionIT.cs | 16 ++++++++-------- .../Session/SFHttpClientPropertiesTest.cs | 16 ++++++++-------- .../Session/SFSessionHttpClientProperties.cs | 11 +++++++---- Snowflake.Data/Core/Session/SFSessionProperty.cs | 7 ------- 4 files changed, 23 insertions(+), 27 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index a8ba78e50..9f254821e 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -32,7 +32,7 @@ public void TestBasicConnection() conn.Open(); Assert.AreEqual(ConnectionState.Open, conn.State); - Assert.AreEqual(SFSessionProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout); + Assert.AreEqual(SFSessionHttpClientProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout); // Data source is empty string for now Assert.AreEqual("", ((SnowflakeDbConnection)conn).DataSource); @@ -380,7 +380,7 @@ public void TestLoginTimeout() using (IDbConnection conn = new MockSnowflakeDbConnection()) { // Connection timeout can only be increased - int timeoutSec = 5 + SFSessionProperties.s_connectionTimeoutDefault; + int timeoutSec = 5 + SFSessionHttpClientProperties.s_connectionTimeoutDefault; string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0}", timeoutSec); @@ -441,7 +441,7 @@ public void TestLoginWithMaxRetryReached() // retry 5 times with starting backoff of 4 seconds with jitter // but should not delay more than the default connection timeout of 300 seconds // and should not take less time than 10 (with max negative jitter for 5 retries) seconds - Assert.Less(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000); + Assert.Less(stopwatch.ElapsedMilliseconds, SFSessionHttpClientProperties.s_connectionTimeoutDefault * 1000); Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 10 * 1000); } } @@ -1674,7 +1674,7 @@ public void TestCancelLoginBeforeTimeout() using (var conn = new MockSnowflakeDbConnection()) { // Add 10 seconds to default timeout - int timeoutSec = 10 + SFSessionProperties.s_connectionTimeoutDefault; + int timeoutSec = 10 + SFSessionHttpClientProperties.s_connectionTimeoutDefault; string infiniteLoginTimeOut = String.Format(ConnectionString + ";connection_timeout={0};maxHttpRetries=0", timeoutSec); @@ -1723,7 +1723,7 @@ public void TestAsyncLoginTimeout() using (var conn = new MockSnowflakeDbConnection()) { // Connection timeout can only be increased - int timeoutSec = 5 + SFSessionProperties.s_connectionTimeoutDefault; + int timeoutSec = 5 + SFSessionHttpClientProperties.s_connectionTimeoutDefault; string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0}", timeoutSec); conn.ConnectionString = loginTimeOut5sec; @@ -1781,12 +1781,12 @@ public void TestAsyncDefaultLoginTimeout() int delta = 100; // in case server time slower. // Should timeout after the default timeout (300 sec) - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000 - delta); + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, SFSessionHttpClientProperties.s_connectionTimeoutDefault * 1000 - delta); // But never more because there's no connection timeout remaining - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, SFSessionProperties.s_connectionTimeoutDefault * 1000 + delta); + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, SFSessionHttpClientProperties.s_connectionTimeoutDefault * 1000 + delta); Assert.AreEqual(ConnectionState.Closed, conn.State); - Assert.AreEqual(SFSessionProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout); + Assert.AreEqual(SFSessionHttpClientProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout); } } diff --git a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs index 709fc17bb..dd3da7084 100644 --- a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs @@ -32,7 +32,7 @@ public void ShouldConvertToMapOnly2Properties( { validateDefaultParameters = validateDefaultParameters, clientSessionKeepAlive = clientSessionKeepAlive, - timeoutInSec = BaseRestRequest.DEFAULT_REST_RETRY_SECONDS_TIMEOUT, + timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, @@ -124,7 +124,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, @@ -138,7 +138,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = false, clientSessionKeepAlive = false, - timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, @@ -152,7 +152,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = true, - timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, @@ -194,7 +194,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, insecureMode = true, disableRetry = false, forceRetryOn404 = false, @@ -208,7 +208,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = true, forceRetryOn404 = false, @@ -222,7 +222,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = true, @@ -236,7 +236,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, diff --git a/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs b/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs index 57a6cfe14..65b751ff9 100644 --- a/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs +++ b/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs @@ -8,7 +8,7 @@ namespace Snowflake.Data.Core internal class SFSessionHttpClientProperties { - private static int recommendedMinTimeoutSec = BaseRestRequest.DEFAULT_REST_RETRY_SECONDS_TIMEOUT; + internal static readonly int s_connectionTimeoutDefault = 300; private static readonly SFLogger logger = SFLoggerFactory.GetLogger(); internal bool validateDefaultParameters; @@ -23,10 +23,13 @@ internal class SFSessionHttpClientProperties internal void WarnOnTimeout() { - if (timeoutInSec < recommendedMinTimeoutSec) + if (timeoutInSec < s_connectionTimeoutDefault) { - logger.Warn($"Connection timeout provided is less than recommended minimum value of" + - $" {recommendedMinTimeoutSec}"); + logger.Warn($"Connection timeout provided is less than the allowed minimum value of" + + $" {s_connectionTimeoutDefault}"); + + // The login timeout can only be increased from the default value + timeoutInSec = s_connectionTimeoutDefault; } if (timeoutInSec < 0) diff --git a/Snowflake.Data/Core/Session/SFSessionProperty.cs b/Snowflake.Data/Core/Session/SFSessionProperty.cs index acee9c2fd..ddb7e6f14 100755 --- a/Snowflake.Data/Core/Session/SFSessionProperty.cs +++ b/Snowflake.Data/Core/Session/SFSessionProperty.cs @@ -100,7 +100,6 @@ class SFSessionPropertyAttr : Attribute class SFSessionProperties : Dictionary { static private SFLogger logger = SFLoggerFactory.GetLogger(); - internal static readonly int s_connectionTimeoutDefault = 300; // Connection string properties to obfuscate in the log static private List secretProps = @@ -267,12 +266,6 @@ internal static SFSessionProperties parseConnectionString(String connectionStrin // passed on for account_name properties[SFSessionProperty.ACCOUNT] = properties[SFSessionProperty.ACCOUNT].Split('.')[0]; - // The login timeout can only be increased - if (int.Parse(properties[SFSessionProperty.CONNECTION_TIMEOUT]) < s_connectionTimeoutDefault) - { - properties[SFSessionProperty.CONNECTION_TIMEOUT] = s_connectionTimeoutDefault.ToString(); - } - return properties; } From a203e197ea8370fca5d8e10eb9455726a55c6ba1 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 26 Oct 2023 17:44:50 -0700 Subject: [PATCH 08/21] SNOW-878067: Refactor variable for connection timeout --- .../UnitTests/Session/SFHttpClientPropertiesTest.cs | 1 + Snowflake.Data/Core/Session/SFSession.cs | 2 +- Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs index dd3da7084..2f2f36f04 100644 --- a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs @@ -102,6 +102,7 @@ public void ShouldExtractProperties(PropertiesTestCase testCase) // when var extractedProperties = extractor.ExtractProperties(properties); + extractedProperties.CheckTimeoutIsValid(); // then Assert.AreEqual(testCase.expectedProperties.validateDefaultParameters, extractedProperties.validateDefaultParameters); diff --git a/Snowflake.Data/Core/Session/SFSession.cs b/Snowflake.Data/Core/Session/SFSession.cs index e5a570780..033925881 100755 --- a/Snowflake.Data/Core/Session/SFSession.cs +++ b/Snowflake.Data/Core/Session/SFSession.cs @@ -157,7 +157,7 @@ internal SFSession( InsecureMode = extractedProperties.insecureMode; _HttpClient = HttpUtil.Instance.GetHttpClient(httpClientConfig); restRequester = new RestRequester(_HttpClient); - extractedProperties.WarnOnTimeout(); + extractedProperties.CheckTimeoutIsValid(); connectionTimeout = extractedProperties.TimeoutDuration(); properties.TryGetValue(SFSessionProperty.CLIENT_CONFIG_FILE, out var easyLoggingConfigFile); _easyLoggingStarter.Init(easyLoggingConfigFile); diff --git a/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs b/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs index 65b751ff9..6309824f4 100644 --- a/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs +++ b/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs @@ -21,7 +21,7 @@ internal class SFSessionHttpClientProperties internal bool includeRetryReason; internal SFSessionHttpClientProxyProperties proxyProperties; - internal void WarnOnTimeout() + internal void CheckTimeoutIsValid() { if (timeoutInSec < s_connectionTimeoutDefault) { From da0d1c6befb30148e9ae5be4c0c6e1ddc010101e Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf <115584722+sfc-gh-ext-simba-lf@users.noreply.github.com> Date: Tue, 31 Oct 2023 12:12:47 -0700 Subject: [PATCH 09/21] Update CONNECTION_TIMEOUT default value from 120 to 300 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 440cd0e0a..5605b77dd 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,7 @@ The following table lists all valid connection properties: | SCHEMA | No | | | USER | Depends | If AUTHENTICATOR is set to `externalbrowser` this is optional. For native SSO through Okta, set this to the login name for your identity provider (IdP). | | WAREHOUSE | No | | -| CONNECTION_TIMEOUT | No | Total timeout in seconds when connecting to Snowflake. The default is 120 seconds | +| CONNECTION_TIMEOUT | No | Total timeout in seconds when connecting to Snowflake. The default is 300 seconds | | MAXHTTPRETRIES | No | Maximum number of times to retry failed HTTP requests (default: 7). You can set `MAXHTTPRETRIES=0` to remove the retry limit, but doing so runs the risk of the .NET driver infinitely retrying failed HTTP calls. | | CLIENT_SESSION_KEEP_ALIVE | No | Whether to keep the current session active after a period of inactivity, or to force the user to login again. If the value is `true`, Snowflake keeps the session active indefinitely, even if there is no activity from the user. If the value is `false`, the user must log in again after four hours of inactivity. The default is `false`. Setting this value overrides the server session property for the current session. | | DISABLERETRY | No | Set this property to `true` to prevent the driver from reconnecting automatically when the connection fails or drops. The default value is `false`. | From 8007e5e4aeeb03f655de352c41bc2ce293103a44 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 2 Nov 2023 18:23:48 -0700 Subject: [PATCH 10/21] SNOW-878067: Update retry logic --- .../IntegrationTests/SFConnectionIT.cs | 62 ++++---- .../UnitTests/HttpUtilTest.cs | 2 + .../UnitTests/SFSessionPropertyTest.cs | 8 + .../Session/SFHttpClientPropertiesTest.cs | 149 +++++++++++++----- Snowflake.Data/Core/HttpUtil.cs | 56 ++++--- Snowflake.Data/Core/Session/SFSession.cs | 2 +- .../Session/SFSessionHttpClientProperties.cs | 41 ++++- .../Core/Session/SFSessionProperty.cs | 2 + 8 files changed, 226 insertions(+), 96 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 9f254821e..dbbf1bc3c 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -32,7 +32,7 @@ public void TestBasicConnection() conn.Open(); Assert.AreEqual(ConnectionState.Open, conn.State); - Assert.AreEqual(SFSessionHttpClientProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout); + Assert.AreEqual(SFSessionHttpClientProperties.s_retryTimeoutDefault, conn.ConnectionTimeout); // Data source is empty string for now Assert.AreEqual("", ((SnowflakeDbConnection)conn).DataSource); @@ -379,8 +379,7 @@ public void TestLoginTimeout() { using (IDbConnection conn = new MockSnowflakeDbConnection()) { - // Connection timeout can only be increased - int timeoutSec = 5 + SFSessionHttpClientProperties.s_connectionTimeoutDefault; + int timeoutSec = 5; string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0}", timeoutSec); @@ -392,7 +391,6 @@ public void TestLoginTimeout() { conn.Open(); Assert.Fail(); - } catch (AggregateException e) { @@ -404,8 +402,8 @@ public void TestLoginTimeout() stopwatch.Stop(); int delta = 100; // in case server time slower. - // Should timeout after the minimum possible timeout with jitter: 14 seconds (50% of 4 * 2) after 7 retries - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 14 * 1000); + // Should timeout after the minimum possible timeout with jitter + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000); // Should timeout before the defined connection timeout Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + delta); Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); @@ -438,11 +436,11 @@ public void TestLoginWithMaxRetryReached() } stopwatch.Stop(); - // retry 5 times with starting backoff of 4 seconds with jitter - // but should not delay more than the default connection timeout of 300 seconds - // and should not take less time than 10 (with max negative jitter for 5 retries) seconds - Assert.Less(stopwatch.ElapsedMilliseconds, SFSessionHttpClientProperties.s_connectionTimeoutDefault * 1000); - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 10 * 1000); + // retry 5 times with starting backoff of 1 second + // but should not delay more than the max possible seconds after 5 retries + // and should not take less time than the minimum possible seconds after 5 retries + Assert.Less(stopwatch.ElapsedMilliseconds, 79 * 1000); + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000); } } @@ -454,8 +452,8 @@ public void TestDefaultLoginTimeout() { conn.ConnectionString = ConnectionString; - // Default timeout is 120 sec - Assert.AreEqual(120, conn.ConnectionTimeout); + // Default timeout is 300 sec + Assert.AreEqual(SFSessionHttpClientProperties.s_retryTimeoutDefault, conn.ConnectionTimeout); Assert.AreEqual(conn.State, ConnectionState.Closed); Stopwatch stopwatch = Stopwatch.StartNew(); @@ -472,10 +470,12 @@ public void TestDefaultLoginTimeout() ((SnowflakeDbException)e.InnerException).ErrorCode); stopwatch.Stop(); - // Should timeout after the default timeout (120 sec) - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 120 * 1000); - // But never more than 16 sec (max backoff) after the default timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (120 + 16) * 1000); + int delta = 100; // in case server time slower. + + // Should timeout after the default timeout (300 sec) + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, conn.ConnectionTimeout * 1000 - delta); + // But never more because there's no connection timeout remaining + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, conn.ConnectionTimeout * 1000 + delta); } } } @@ -1532,7 +1532,7 @@ public void TestEscapeChar() conn.Open(); Assert.AreEqual(ConnectionState.Open, conn.State); - Assert.AreEqual(120, conn.ConnectionTimeout); + Assert.AreEqual(SFSessionHttpClientProperties.s_retryTimeoutDefault, conn.ConnectionTimeout); // Data source is empty string for now Assert.AreEqual("", ((SnowflakeDbConnection)conn).DataSource); @@ -1559,7 +1559,7 @@ public void TestEscapeChar1() conn.Open(); Assert.AreEqual(ConnectionState.Open, conn.State); - Assert.AreEqual(120, conn.ConnectionTimeout); + Assert.AreEqual(SFSessionHttpClientProperties.s_retryTimeoutDefault, conn.ConnectionTimeout); // Data source is empty string for now Assert.AreEqual("", ((SnowflakeDbConnection)conn).DataSource); @@ -1673,8 +1673,8 @@ public void TestCancelLoginBeforeTimeout() { using (var conn = new MockSnowflakeDbConnection()) { - // Add 10 seconds to default timeout - int timeoutSec = 10 + SFSessionHttpClientProperties.s_connectionTimeoutDefault; + // No timeout + int timeoutSec = 0; string infiniteLoginTimeOut = String.Format(ConnectionString + ";connection_timeout={0};maxHttpRetries=0", timeoutSec); @@ -1683,13 +1683,14 @@ public void TestCancelLoginBeforeTimeout() Assert.AreEqual(conn.State, ConnectionState.Closed); // At this point the connection string has not been parsed, it will return the // default value - //Assert.AreEqual(120, conn.ConnectionTimeout); + //Assert.AreEqual(SFSessionHttpClientProperties.s_retryTimeoutDefault, conn.ConnectionTimeout); CancellationTokenSource connectionCancelToken = new CancellationTokenSource(); Task connectTask = conn.OpenAsync(connectionCancelToken.Token); - // Sleep for the specified timeout minus 10 seconds (just before the actual timeout is reached) - Thread.Sleep((timeoutSec - 10) * 1000); + // Sleep for the default timeout minus 10 seconds (just before the actual timeout is reached) + // to make sure there are no false positive + Thread.Sleep((SFSessionHttpClientProperties.s_retryTimeoutDefault - 10) * 1000); Assert.AreEqual(ConnectionState.Connecting, conn.State); @@ -1722,8 +1723,7 @@ public void TestAsyncLoginTimeout() { using (var conn = new MockSnowflakeDbConnection()) { - // Connection timeout can only be increased - int timeoutSec = 5 + SFSessionHttpClientProperties.s_connectionTimeoutDefault; + int timeoutSec = 5; string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0}", timeoutSec); conn.ConnectionString = loginTimeOut5sec; @@ -1745,8 +1745,8 @@ public void TestAsyncLoginTimeout() stopwatch.Stop(); int delta = 100; // in case server time slower. - // Should timeout after the minimum possible timeout with jitter: 14 seconds (50% of 4 * 2) after 7 retries - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 14 * 1000); + // Should timeout after the minimum possible timeout with jitter + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000); // Should timeout before the defined connection timeout Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + delta); @@ -1781,12 +1781,12 @@ public void TestAsyncDefaultLoginTimeout() int delta = 100; // in case server time slower. // Should timeout after the default timeout (300 sec) - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, SFSessionHttpClientProperties.s_connectionTimeoutDefault * 1000 - delta); + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, conn.ConnectionTimeout * 1000 - delta); // But never more because there's no connection timeout remaining - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, SFSessionHttpClientProperties.s_connectionTimeoutDefault * 1000 + delta); + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, conn.ConnectionTimeout * 1000 + delta); Assert.AreEqual(ConnectionState.Closed, conn.State); - Assert.AreEqual(SFSessionHttpClientProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout); + Assert.AreEqual(SFSessionHttpClientProperties.s_retryTimeoutDefault, conn.ConnectionTimeout); } } diff --git a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs index bf8063a7f..4ac01d134 100644 --- a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs +++ b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs @@ -96,6 +96,7 @@ public void ShouldCreateHttpClientHandlerWithProxy() "localhost", false, false, + 300, 7 ); @@ -120,6 +121,7 @@ public void ShouldCreateHttpClientHandlerWithoutProxy() null, false, false, + 300, 0 ); diff --git a/Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs b/Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs index d9856643d..dd31f1c16 100644 --- a/Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs +++ b/Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs @@ -57,6 +57,7 @@ public static IEnumerable ConnectionStringTestCases() string defProxyPort = "1234"; string defNonProxyHosts = "localhost"; + string defRetryTimeout = "300"; string defMaxHttpRetries = "7"; string defIncludeRetryReason = "true"; string defDisableQueryContextCache = "false"; @@ -82,6 +83,7 @@ public static IEnumerable ConnectionStringTestCases() { SFSessionProperty.CLIENT_SESSION_KEEP_ALIVE, "false" }, { SFSessionProperty.FORCEPARSEERROR, "false" }, { SFSessionProperty.BROWSER_RESPONSE_TIMEOUT, defBrowserResponseTime }, + { SFSessionProperty.RETRY_TIMEOUT, defRetryTimeout }, { SFSessionProperty.MAXHTTPRETRIES, defMaxHttpRetries }, { SFSessionProperty.INCLUDERETRYREASON, defIncludeRetryReason }, { SFSessionProperty.DISABLEQUERYCONTEXTCACHE, defDisableQueryContextCache } @@ -107,6 +109,7 @@ public static IEnumerable ConnectionStringTestCases() { SFSessionProperty.CLIENT_SESSION_KEEP_ALIVE, "false" }, { SFSessionProperty.FORCEPARSEERROR, "false" }, { SFSessionProperty.BROWSER_RESPONSE_TIMEOUT, "180" }, + { SFSessionProperty.RETRY_TIMEOUT, defRetryTimeout }, { SFSessionProperty.MAXHTTPRETRIES, defMaxHttpRetries }, { SFSessionProperty.INCLUDERETRYREASON, defIncludeRetryReason }, { SFSessionProperty.DISABLEQUERYCONTEXTCACHE, defDisableQueryContextCache } @@ -135,6 +138,7 @@ public static IEnumerable ConnectionStringTestCases() { SFSessionProperty.PROXYPORT, defProxyPort }, { SFSessionProperty.NONPROXYHOSTS, defNonProxyHosts }, { SFSessionProperty.BROWSER_RESPONSE_TIMEOUT, defBrowserResponseTime }, + { SFSessionProperty.RETRY_TIMEOUT, defRetryTimeout }, { SFSessionProperty.MAXHTTPRETRIES, defMaxHttpRetries }, { SFSessionProperty.INCLUDERETRYREASON, defIncludeRetryReason }, { SFSessionProperty.DISABLEQUERYCONTEXTCACHE, defDisableQueryContextCache } @@ -165,6 +169,7 @@ public static IEnumerable ConnectionStringTestCases() { SFSessionProperty.PROXYPORT, defProxyPort }, { SFSessionProperty.NONPROXYHOSTS, defNonProxyHosts }, { SFSessionProperty.BROWSER_RESPONSE_TIMEOUT, defBrowserResponseTime }, + { SFSessionProperty.RETRY_TIMEOUT, defRetryTimeout }, { SFSessionProperty.MAXHTTPRETRIES, defMaxHttpRetries }, { SFSessionProperty.INCLUDERETRYREASON, defIncludeRetryReason }, { SFSessionProperty.DISABLEQUERYCONTEXTCACHE, defDisableQueryContextCache } @@ -193,6 +198,7 @@ public static IEnumerable ConnectionStringTestCases() { SFSessionProperty.CLIENT_SESSION_KEEP_ALIVE, "false" }, { SFSessionProperty.FORCEPARSEERROR, "false" }, { SFSessionProperty.BROWSER_RESPONSE_TIMEOUT, defBrowserResponseTime }, + { SFSessionProperty.RETRY_TIMEOUT, defRetryTimeout }, { SFSessionProperty.MAXHTTPRETRIES, defMaxHttpRetries }, { SFSessionProperty.FILE_TRANSFER_MEMORY_THRESHOLD, "25" }, { SFSessionProperty.INCLUDERETRYREASON, defIncludeRetryReason }, @@ -220,6 +226,7 @@ public static IEnumerable ConnectionStringTestCases() { SFSessionProperty.CLIENT_SESSION_KEEP_ALIVE, "false" }, { SFSessionProperty.FORCEPARSEERROR, "false" }, { SFSessionProperty.BROWSER_RESPONSE_TIMEOUT, defBrowserResponseTime }, + { SFSessionProperty.RETRY_TIMEOUT, defRetryTimeout }, { SFSessionProperty.MAXHTTPRETRIES, defMaxHttpRetries }, { SFSessionProperty.INCLUDERETRYREASON, "false" }, { SFSessionProperty.DISABLEQUERYCONTEXTCACHE, defDisableQueryContextCache } @@ -245,6 +252,7 @@ public static IEnumerable ConnectionStringTestCases() { SFSessionProperty.CLIENT_SESSION_KEEP_ALIVE, "false" }, { SFSessionProperty.FORCEPARSEERROR, "false" }, { SFSessionProperty.BROWSER_RESPONSE_TIMEOUT, defBrowserResponseTime }, + { SFSessionProperty.RETRY_TIMEOUT, defRetryTimeout }, { SFSessionProperty.MAXHTTPRETRIES, defMaxHttpRetries }, { SFSessionProperty.INCLUDERETRYREASON, defIncludeRetryReason }, { SFSessionProperty.DISABLEQUERYCONTEXTCACHE, "true" } diff --git a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs index 2f2f36f04..8337a3b66 100644 --- a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs @@ -32,10 +32,11 @@ public void ShouldConvertToMapOnly2Properties( { validateDefaultParameters = validateDefaultParameters, clientSessionKeepAlive = clientSessionKeepAlive, - timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, maxHttpRetries = 7, proxyProperties = proxyProperties }; @@ -69,6 +70,7 @@ public void ShouldBuildHttpClientConfig() insecureMode = TestDataGenarator.NextBool(), disableRetry = TestDataGenarator.NextBool(), forceRetryOn404 = TestDataGenarator.NextBool(), + retryTimeout = TestDataGenarator.NextInt(300, 600), maxHttpRetries = TestDataGenarator.NextInt(0, 15), proxyProperties = proxyProperties }; @@ -85,6 +87,7 @@ public void ShouldBuildHttpClientConfig() Assert.AreEqual(properties.proxyProperties.nonProxyHosts, config.NoProxyList); Assert.AreEqual(properties.disableRetry, config.DisableRetry); Assert.AreEqual(properties.forceRetryOn404, config.ForceRetryOn404); + Assert.AreEqual(properties.retryTimeout, config.RetryTimeout); Assert.AreEqual(properties.maxHttpRetries, config.MaxHttpRetries); } @@ -102,7 +105,7 @@ public void ShouldExtractProperties(PropertiesTestCase testCase) // when var extractedProperties = extractor.ExtractProperties(properties); - extractedProperties.CheckTimeoutIsValid(); + extractedProperties.CheckPropertiesAreValid(); // then Assert.AreEqual(testCase.expectedProperties.validateDefaultParameters, extractedProperties.validateDefaultParameters); @@ -111,6 +114,7 @@ public void ShouldExtractProperties(PropertiesTestCase testCase) Assert.AreEqual(testCase.expectedProperties.insecureMode, extractedProperties.insecureMode); Assert.AreEqual(testCase.expectedProperties.disableRetry, extractedProperties.disableRetry); Assert.AreEqual(testCase.expectedProperties.forceRetryOn404, extractedProperties.forceRetryOn404); + Assert.AreEqual(testCase.expectedProperties.retryTimeout, extractedProperties.retryTimeout); Assert.AreEqual(testCase.expectedProperties.maxHttpRetries, extractedProperties.maxHttpRetries); Assert.AreEqual(proxyProperties, extractedProperties.proxyProperties); proxyExtractorMock.Verify(e => e.ExtractProperties(properties), Times.Once); @@ -125,11 +129,12 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, - maxHttpRetries = 7 + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault } }; var propertiesWithValidateDefaultParametersChanged = new PropertiesTestCase() @@ -139,11 +144,12 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = false, clientSessionKeepAlive = false, - timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, - maxHttpRetries = 7 + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault } }; var propertiesWithClientSessionKeepAliveChanged = new PropertiesTestCase() @@ -153,39 +159,27 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = true, - timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, - maxHttpRetries = 7 + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault } }; - var propertiesWithTimeoutChangedToAValueAbove300 = new PropertiesTestCase() - { - conectionString = "account=test;user=test;password=test;connection_timeout=600", - expectedProperties = new SFSessionHttpClientProperties() - { - validateDefaultParameters = true, - clientSessionKeepAlive = false, - timeoutInSec = 600, - insecureMode = false, - disableRetry = false, - forceRetryOn404 = false, - maxHttpRetries = 7 - } - }; - var propertiesWithTimeoutChangedToAValueBelow300 = new PropertiesTestCase() + var propertiesWithTimeoutChanged = new PropertiesTestCase() { conectionString = "account=test;user=test;password=test;connection_timeout=15", expectedProperties = new SFSessionHttpClientProperties() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = 300, + timeoutInSec = 15, insecureMode = false, disableRetry = false, forceRetryOn404 = false, - maxHttpRetries = 7 + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault } }; var propertiesWithInsecureModeChanged = new PropertiesTestCase() @@ -195,11 +189,12 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, insecureMode = true, disableRetry = false, forceRetryOn404 = false, - maxHttpRetries = 7 + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault } }; var propertiesWithDisableRetryChanged = new PropertiesTestCase() @@ -209,11 +204,12 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, insecureMode = false, disableRetry = true, forceRetryOn404 = false, - maxHttpRetries = 7 + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault } }; var propertiesWithForceRetryOn404Changed = new PropertiesTestCase() @@ -223,38 +219,119 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = true, - maxHttpRetries = 7 + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault + } + }; + var propertiesWithRetryTimeoutChangedToAValueAbove300 = new PropertiesTestCase() + { + conectionString = "account=test;user=test;password=test;retry_timeout=600", + expectedProperties = new SFSessionHttpClientProperties() + { + validateDefaultParameters = true, + clientSessionKeepAlive = false, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, + insecureMode = false, + disableRetry = false, + forceRetryOn404 = false, + retryTimeout = 600, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault + } + }; + var propertiesWithRetryTimeoutChangedToAValueBelow300 = new PropertiesTestCase() + { + conectionString = "account=test;user=test;password=test;retry_timeout=15", + expectedProperties = new SFSessionHttpClientProperties() + { + validateDefaultParameters = true, + clientSessionKeepAlive = false, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, + insecureMode = false, + disableRetry = false, + forceRetryOn404 = false, + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault } }; - var propertiesWithMaxHttpRetiesChanged = new PropertiesTestCase() + var propertiesWithRetryTimeoutChangedToZero = new PropertiesTestCase() + { + conectionString = "account=test;user=test;password=test;retry_timeout=0", + expectedProperties = new SFSessionHttpClientProperties() + { + validateDefaultParameters = true, + clientSessionKeepAlive = false, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, + insecureMode = false, + disableRetry = false, + forceRetryOn404 = false, + retryTimeout = 0, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault + } + }; + var propertiesWithMaxHttpRetriesChangedToAValueAbove7 = new PropertiesTestCase() { conectionString = "account=test;user=test;password=test;maxHttpRetries=10", expectedProperties = new SFSessionHttpClientProperties() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionHttpClientProperties.s_connectionTimeoutDefault, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, insecureMode = false, disableRetry = false, forceRetryOn404 = false, + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, maxHttpRetries = 10 } }; + var propertiesWithMaxHttpRetriesChangedToAValueBelow7 = new PropertiesTestCase() + { + conectionString = "account=test;user=test;password=test;maxHttpRetries=5", + expectedProperties = new SFSessionHttpClientProperties() + { + validateDefaultParameters = true, + clientSessionKeepAlive = false, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, + insecureMode = false, + disableRetry = false, + forceRetryOn404 = false, + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = SFSessionHttpClientProperties.s_maxHttpRetriesDefault + } + }; + var propertiesWithMaxHttpRetriesChangedToZero = new PropertiesTestCase() + { + conectionString = "account=test;user=test;password=test;maxHttpRetries=0", + expectedProperties = new SFSessionHttpClientProperties() + { + validateDefaultParameters = true, + clientSessionKeepAlive = false, + timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, + insecureMode = false, + disableRetry = false, + forceRetryOn404 = false, + retryTimeout = SFSessionHttpClientProperties.s_retryTimeoutDefault, + maxHttpRetries = 0 + } + }; return new [] { defaultProperties, propertiesWithValidateDefaultParametersChanged, propertiesWithClientSessionKeepAliveChanged, - propertiesWithTimeoutChangedToAValueAbove300, - propertiesWithTimeoutChangedToAValueBelow300, + propertiesWithTimeoutChanged, propertiesWithInsecureModeChanged, propertiesWithDisableRetryChanged, propertiesWithForceRetryOn404Changed, - propertiesWithMaxHttpRetiesChanged + propertiesWithRetryTimeoutChangedToAValueAbove300, + propertiesWithRetryTimeoutChangedToAValueBelow300, + propertiesWithRetryTimeoutChangedToZero, + propertiesWithMaxHttpRetriesChangedToAValueAbove7, + propertiesWithMaxHttpRetriesChangedToAValueBelow7, + propertiesWithMaxHttpRetriesChangedToZero }; } diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index b56a62d6f..ef5d98db0 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -28,6 +28,7 @@ public HttpClientConfig( string noProxyList, bool disableRetry, bool forceRetryOn404, + int retryTimeout, int maxHttpRetries, bool includeRetryReason = true) { @@ -39,6 +40,7 @@ public HttpClientConfig( NoProxyList = noProxyList; DisableRetry = disableRetry; ForceRetryOn404 = forceRetryOn404; + RetryTimeout = retryTimeout; MaxHttpRetries = maxHttpRetries; IncludeRetryReason = includeRetryReason; @@ -52,6 +54,7 @@ public HttpClientConfig( noProxyList, disableRetry.ToString(), forceRetryOn404.ToString(), + retryTimeout.ToString(), maxHttpRetries.ToString(), includeRetryReason.ToString()}); } @@ -64,6 +67,7 @@ public HttpClientConfig( public readonly string NoProxyList; public readonly bool DisableRetry; public readonly bool ForceRetryOn404; + public readonly int RetryTimeout; public readonly int MaxHttpRetries; public readonly bool IncludeRetryReason; @@ -75,7 +79,6 @@ public sealed class HttpUtil { static internal readonly int MAX_BACKOFF = 16; private static readonly int s_baseBackOffTime = 1; - private static readonly int s_baseLoginBackOffTime = 4; private static readonly int s_exponentialFactor = 2; private static readonly SFLogger logger = SFLoggerFactory.GetLogger(); @@ -117,7 +120,7 @@ private HttpClient RegisterNewHttpClientIfNecessary(HttpClientConfig config) logger.Debug("Http client not registered. Adding."); var httpClient = new HttpClient( - new RetryHandler(SetupCustomHttpHandler(config), config.DisableRetry, config.ForceRetryOn404, config.MaxHttpRetries, config.IncludeRetryReason)) + new RetryHandler(SetupCustomHttpHandler(config), config.DisableRetry, config.ForceRetryOn404, config.RetryTimeout, config.MaxHttpRetries, config.IncludeRetryReason)) { Timeout = Timeout.InfiniteTimeSpan }; @@ -328,14 +331,16 @@ private class RetryHandler : DelegatingHandler private bool disableRetry; private bool forceRetryOn404; - private int maxRetry; + private int retryTimeout; + private int maxRetryCount; private bool includeRetryReason; - internal RetryHandler(HttpMessageHandler innerHandler, bool disableRetry, bool forceRetryOn404, int maxRetry, bool includeRetryReason) : base(innerHandler) + internal RetryHandler(HttpMessageHandler innerHandler, bool disableRetry, bool forceRetryOn404, int retryTimeout, int maxRetryCount, bool includeRetryReason) : base(innerHandler) { this.disableRetry = disableRetry; this.forceRetryOn404 = forceRetryOn404; - this.maxRetry = maxRetry; + this.retryTimeout = retryTimeout; + this.maxRetryCount = maxRetryCount; this.includeRetryReason = includeRetryReason; } @@ -344,7 +349,7 @@ protected override async Task SendAsync(HttpRequestMessage { HttpResponseMessage response = null; bool isLoginRequest = IsLoginEndpoint(requestMessage.RequestUri.AbsolutePath); - int backOffInSec = isLoginRequest ? s_baseLoginBackOffTime : s_baseBackOffTime; + int backOffInSec = s_baseBackOffTime; int totalRetryTime = 0; ServicePoint p = ServicePointManager.FindServicePoint(requestMessage.RequestUri); @@ -353,7 +358,10 @@ protected override async Task SendAsync(HttpRequestMessage p.ConnectionLimit = 20; // Default value is 2, we need more connections for performing multiple parallel queries TimeSpan httpTimeout = (TimeSpan)requestMessage.Properties[SFRestRequest.HTTP_REQUEST_TIMEOUT_KEY]; - TimeSpan restTimeout = (TimeSpan)requestMessage.Properties[SFRestRequest.REST_REQUEST_TIMEOUT_KEY]; + // Use the lower timeout value between rest timeout and max retry timeout + TimeSpan restTimeout = ((TimeSpan)requestMessage.Properties[SFRestRequest.REST_REQUEST_TIMEOUT_KEY]).TotalSeconds < retryTimeout ? + (TimeSpan)requestMessage.Properties[SFRestRequest.REST_REQUEST_TIMEOUT_KEY] : + TimeSpan.FromSeconds(retryTimeout); if (logger.IsDebugEnabled()) { @@ -409,6 +417,7 @@ protected override async Task SendAsync(HttpRequestMessage } int errorReason = 0; + if (response != null) { if (response.IsSuccessStatusCode) @@ -435,14 +444,14 @@ protected override async Task SendAsync(HttpRequestMessage } retryCount++; - if ((maxRetry > 0) && (retryCount > maxRetry)) + if ((maxRetryCount > 0) && (retryCount > maxRetryCount)) { - logger.Debug($"stop retry as maxHttpRetries {maxRetry} reached"); + logger.Debug($"stop retry as maxHttpRetries {maxRetryCount} reached"); if (response != null) { return response; } - throw new OperationCanceledException($"http request failed and max retry {maxRetry} reached"); + throw new OperationCanceledException($"http request failed and max retry {maxRetryCount} reached"); } // Disposing of the response if not null now that we don't need it anymore @@ -450,10 +459,23 @@ protected override async Task SendAsync(HttpRequestMessage requestMessage.RequestUri = updater.Update(errorReason); - // Add jitter for login requests - if (isLoginRequest) + if (retryCount > 1) { - backOffInSec += (int)GetJitter(backOffInSec); + var jitter = GetJitter(backOffInSec); + + // Set backoff time + if (isLoginRequest) + { + // Choose between previous sleep time and new base sleep time for login requests + backOffInSec = (int)ChooseRandom( + backOffInSec + jitter, + Math.Pow(s_exponentialFactor, retryCount) + jitter); + } + else if (backOffInSec < MAX_BACKOFF) + { + // Multiply sleep by 2 for non-login requests + backOffInSec *= 2; + } } if ((restTimeout.TotalSeconds > 0) && (totalRetryTime + backOffInSec > restTimeout.TotalSeconds)) @@ -468,12 +490,6 @@ protected override async Task SendAsync(HttpRequestMessage await Task.Delay(TimeSpan.FromSeconds(backOffInSec), cancellationToken).ConfigureAwait(false); totalRetryTime += backOffInSec; - - // Set next backoff time - if (isLoginRequest || !isLoginRequest && backOffInSec < MAX_BACKOFF) - { - backOffInSec *= s_exponentialFactor; - } } } } @@ -514,7 +530,7 @@ static internal double GetJitter(double curWaitTime) /// The min range (inclusive). /// The max range (inclusive). /// The random number. - static double ChooseRandom(int min, int max) + static double ChooseRandom(double min, double max) { var next = new Random().NextDouble(); diff --git a/Snowflake.Data/Core/Session/SFSession.cs b/Snowflake.Data/Core/Session/SFSession.cs index 033925881..d153ffff4 100755 --- a/Snowflake.Data/Core/Session/SFSession.cs +++ b/Snowflake.Data/Core/Session/SFSession.cs @@ -157,7 +157,7 @@ internal SFSession( InsecureMode = extractedProperties.insecureMode; _HttpClient = HttpUtil.Instance.GetHttpClient(httpClientConfig); restRequester = new RestRequester(_HttpClient); - extractedProperties.CheckTimeoutIsValid(); + extractedProperties.CheckPropertiesAreValid(); connectionTimeout = extractedProperties.TimeoutDuration(); properties.TryGetValue(SFSessionProperty.CLIENT_CONFIG_FILE, out var easyLoggingConfigFile); _easyLoggingStarter.Init(easyLoggingConfigFile); diff --git a/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs b/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs index 6309824f4..9a404a81f 100644 --- a/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs +++ b/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs @@ -8,7 +8,8 @@ namespace Snowflake.Data.Core internal class SFSessionHttpClientProperties { - internal static readonly int s_connectionTimeoutDefault = 300; + internal static readonly int s_maxHttpRetriesDefault = 7; + internal static readonly int s_retryTimeoutDefault = 300; private static readonly SFLogger logger = SFLoggerFactory.GetLogger(); internal bool validateDefaultParameters; @@ -17,25 +18,47 @@ internal class SFSessionHttpClientProperties internal bool insecureMode; internal bool disableRetry; internal bool forceRetryOn404; + internal int retryTimeout; internal int maxHttpRetries; internal bool includeRetryReason; internal SFSessionHttpClientProxyProperties proxyProperties; - internal void CheckTimeoutIsValid() + internal void CheckPropertiesAreValid() { - if (timeoutInSec < s_connectionTimeoutDefault) + if (timeoutInSec < s_retryTimeoutDefault) { - logger.Warn($"Connection timeout provided is less than the allowed minimum value of" + - $" {s_connectionTimeoutDefault}"); - - // The login timeout can only be increased from the default value - timeoutInSec = s_connectionTimeoutDefault; + logger.Warn($"Connection timeout provided is less than recommended minimum value of" + + $" {s_retryTimeoutDefault}"); } if (timeoutInSec < 0) { logger.Warn($"Connection timeout provided is negative. Timeout will be infinite."); } + + if (retryTimeout > 0 && retryTimeout < s_retryTimeoutDefault) + { + logger.Warn($"Max retry timeout provided is less than the allowed minimum value of" + + $" {s_retryTimeoutDefault}"); + + retryTimeout = s_retryTimeoutDefault; + } + else if (retryTimeout == 0) + { + logger.Warn($"Max retry timeout provided is 0. Timeout will be infinite"); + } + + if (maxHttpRetries > 0 && maxHttpRetries < s_maxHttpRetriesDefault) + { + logger.Warn($"Max retry count provided is less than the allowed minimum value of" + + $" {s_maxHttpRetriesDefault}"); + + maxHttpRetries = s_maxHttpRetriesDefault; + } + else if (maxHttpRetries == 0) + { + logger.Warn($"Max retry count provided is 0. Retry count will be infinite"); + } } internal TimeSpan TimeoutDuration() @@ -54,6 +77,7 @@ internal HttpClientConfig BuildHttpClientConfig() proxyProperties.nonProxyHosts, disableRetry, forceRetryOn404, + retryTimeout, maxHttpRetries, includeRetryReason); } @@ -90,6 +114,7 @@ public SFSessionHttpClientProperties ExtractProperties(SFSessionProperties prope insecureMode = Boolean.Parse(propertiesDictionary[SFSessionProperty.INSECUREMODE]), disableRetry = Boolean.Parse(propertiesDictionary[SFSessionProperty.DISABLERETRY]), forceRetryOn404 = Boolean.Parse(propertiesDictionary[SFSessionProperty.FORCERETRYON404]), + retryTimeout = int.Parse(propertiesDictionary[SFSessionProperty.RETRY_TIMEOUT]), maxHttpRetries = int.Parse(propertiesDictionary[SFSessionProperty.MAXHTTPRETRIES]), includeRetryReason = Boolean.Parse(propertiesDictionary[SFSessionProperty.INCLUDERETRYREASON]), proxyProperties = proxyPropertiesExtractor.ExtractProperties(propertiesDictionary) diff --git a/Snowflake.Data/Core/Session/SFSessionProperty.cs b/Snowflake.Data/Core/Session/SFSessionProperty.cs index ddb7e6f14..28ef2fa7e 100755 --- a/Snowflake.Data/Core/Session/SFSessionProperty.cs +++ b/Snowflake.Data/Core/Session/SFSessionProperty.cs @@ -78,6 +78,8 @@ internal enum SFSessionProperty FORCEPARSEERROR, [SFSessionPropertyAttr(required = false, defaultValue = "120")] BROWSER_RESPONSE_TIMEOUT, + [SFSessionPropertyAttr(required = false, defaultValue = "300")] + RETRY_TIMEOUT, [SFSessionPropertyAttr(required = false, defaultValue = "7")] MAXHTTPRETRIES, [SFSessionPropertyAttr(required = false)] From 5b926b7579534e9f36f17868adbf620f33faf609 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf <115584722+sfc-gh-ext-simba-lf@users.noreply.github.com> Date: Thu, 2 Nov 2023 18:35:59 -0700 Subject: [PATCH 11/21] Update README.md with new parameter "RETRY_TIMEOUT" --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 5605b77dd..13f2ca41d 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,7 @@ The following table lists all valid connection properties: | USER | Depends | If AUTHENTICATOR is set to `externalbrowser` this is optional. For native SSO through Okta, set this to the login name for your identity provider (IdP). | | WAREHOUSE | No | | | CONNECTION_TIMEOUT | No | Total timeout in seconds when connecting to Snowflake. The default is 300 seconds | +| RETRY_TIMEOUT | No | Total timeout in seconds for supported endpoints of retry policy. The default is 300 seconds. The value can only be increased from the default value or set to 0 for infinite timeout | | MAXHTTPRETRIES | No | Maximum number of times to retry failed HTTP requests (default: 7). You can set `MAXHTTPRETRIES=0` to remove the retry limit, but doing so runs the risk of the .NET driver infinitely retrying failed HTTP calls. | | CLIENT_SESSION_KEEP_ALIVE | No | Whether to keep the current session active after a period of inactivity, or to force the user to login again. If the value is `true`, Snowflake keeps the session active indefinitely, even if there is no activity from the user. If the value is `false`, the user must log in again after four hours of inactivity. The default is `false`. Setting this value overrides the server session property for the current session. | | DISABLERETRY | No | Set this property to `true` to prevent the driver from reconnecting automatically when the connection fails or drops. The default value is `false`. | From 2866decec1d1b18b9c92cf6154fa73c97199f2d2 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 2 Nov 2023 19:23:03 -0700 Subject: [PATCH 12/21] SNOW-878067: Modify timeout test cases --- .../IntegrationTests/SFConnectionIT.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index dbbf1bc3c..ae8cd09b4 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -403,9 +403,9 @@ public void TestLoginTimeout() int delta = 100; // in case server time slower. // Should timeout after the minimum possible timeout with jitter - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000); - // Should timeout before the defined connection timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + delta); + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000 - delta); + // But never more than 1 sec (max backoff) after the defined timeout + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); } } @@ -1746,9 +1746,9 @@ public void TestAsyncLoginTimeout() int delta = 100; // in case server time slower. // Should timeout after the minimum possible timeout with jitter - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000); - // Should timeout before the defined connection timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 + delta); + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000 - delta); + // But never more than 1 sec (max backoff) after the defined timeout + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); Assert.AreEqual(ConnectionState.Closed, conn.State); Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); From d0a445e6282be626b465136479b56be04743645a Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 2 Nov 2023 19:30:35 -0700 Subject: [PATCH 13/21] SNOW-878067: Modify timeout test cases --- Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index ae8cd09b4..540d6e064 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -402,10 +402,11 @@ public void TestLoginTimeout() stopwatch.Stop(); int delta = 100; // in case server time slower. + // Should timeout before the defined timeout + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); // Should timeout after the minimum possible timeout with jitter Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000 - delta); - // But never more than 1 sec (max backoff) after the defined timeout - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); + Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); } } From ce6b2b6e45799c079a8211655869f0230de193a4 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 2 Nov 2023 19:49:55 -0700 Subject: [PATCH 14/21] SNOW-878067: Modify timeout test cases --- 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 540d6e064..067572ef4 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -402,7 +402,7 @@ public void TestLoginTimeout() stopwatch.Stop(); int delta = 100; // in case server time slower. - // Should timeout before the defined timeout + // Should timeout before the defined timeout plus 1 (max backoff after timeout) Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); // Should timeout after the minimum possible timeout with jitter Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000 - delta); @@ -1784,7 +1784,7 @@ public void TestAsyncDefaultLoginTimeout() // Should timeout after the default timeout (300 sec) Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, conn.ConnectionTimeout * 1000 - delta); // But never more because there's no connection timeout remaining - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, conn.ConnectionTimeout * 1000 + delta); + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (conn.ConnectionTimeout + 1) * 1000); Assert.AreEqual(ConnectionState.Closed, conn.State); Assert.AreEqual(SFSessionHttpClientProperties.s_retryTimeoutDefault, conn.ConnectionTimeout); From 98f0abe4587862f336a30aff4977b1b7d6aabf28 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 2 Nov 2023 20:04:17 -0700 Subject: [PATCH 15/21] SNOW-878067: Modify timeout test cases --- .../IntegrationTests/SFConnectionIT.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 067572ef4..8ed7481d0 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -400,7 +400,7 @@ public void TestLoginTimeout() ((SnowflakeDbException)e.InnerException).ErrorCode); } stopwatch.Stop(); - int delta = 100; // in case server time slower. + int delta = 10; // in case server time slower. // Should timeout before the defined timeout plus 1 (max backoff after timeout) Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); @@ -471,12 +471,12 @@ public void TestDefaultLoginTimeout() ((SnowflakeDbException)e.InnerException).ErrorCode); stopwatch.Stop(); - int delta = 100; // in case server time slower. + int delta = 10; // in case server time slower. // Should timeout after the default timeout (300 sec) Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, conn.ConnectionTimeout * 1000 - delta); // But never more because there's no connection timeout remaining - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, conn.ConnectionTimeout * 1000 + delta); + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (conn.ConnectionTimeout + 1) * 1000); } } } @@ -1744,7 +1744,7 @@ public void TestAsyncLoginTimeout() } stopwatch.Stop(); - int delta = 100; // in case server time slower. + int delta = 10; // in case server time slower. // Should timeout after the minimum possible timeout with jitter Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000 - delta); @@ -1779,7 +1779,7 @@ public void TestAsyncDefaultLoginTimeout() ((SnowflakeDbException)e.InnerException).ErrorCode); } stopwatch.Stop(); - int delta = 100; // in case server time slower. + int delta = 10; // in case server time slower. // Should timeout after the default timeout (300 sec) Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, conn.ConnectionTimeout * 1000 - delta); From b25afecd385145f33a712366c32bb3b5c515807a Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 2 Nov 2023 20:21:07 -0700 Subject: [PATCH 16/21] SNOW-878067: Update comment --- 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 8ed7481d0..9816f570c 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -402,7 +402,7 @@ public void TestLoginTimeout() stopwatch.Stop(); int delta = 10; // in case server time slower. - // Should timeout before the defined timeout plus 1 (max backoff after timeout) + // Should timeout before the defined timeout plus 1 (buffer time) Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); // Should timeout after the minimum possible timeout with jitter Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000 - delta); @@ -1748,7 +1748,7 @@ public void TestAsyncLoginTimeout() // Should timeout after the minimum possible timeout with jitter Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000 - delta); - // But never more than 1 sec (max backoff) after the defined timeout + // 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); From 13fa45f0e69edddb58acf906e5fcde65ba5f4d0a Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Thu, 2 Nov 2023 20:23:45 -0700 Subject: [PATCH 17/21] SNOW-878067: Modify timeout test cases --- Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 9816f570c..4f192094b 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -1689,9 +1689,8 @@ public void TestCancelLoginBeforeTimeout() CancellationTokenSource connectionCancelToken = new CancellationTokenSource(); Task connectTask = conn.OpenAsync(connectionCancelToken.Token); - // Sleep for the default timeout minus 10 seconds (just before the actual timeout is reached) - // to make sure there are no false positive - Thread.Sleep((SFSessionHttpClientProperties.s_retryTimeoutDefault - 10) * 1000); + // Sleep for more than the default timeout to make sure there are no false positive) + Thread.Sleep((SFSessionHttpClientProperties.s_retryTimeoutDefault + 10) * 1000); Assert.AreEqual(ConnectionState.Connecting, conn.State); From 5e9931e8b12f165fb1e49b71dc2519a109e801d0 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Fri, 3 Nov 2023 11:15:15 -0700 Subject: [PATCH 18/21] SNOW-878067: Refactor retry logic --- Snowflake.Data/Core/HttpUtil.cs | 40 ++++++++++++++++----------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index ef5d98db0..870dcb25e 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -459,23 +459,26 @@ protected override async Task SendAsync(HttpRequestMessage requestMessage.RequestUri = updater.Update(errorReason); - if (retryCount > 1) - { - var jitter = GetJitter(backOffInSec); - // Set backoff time - if (isLoginRequest) - { - // Choose between previous sleep time and new base sleep time for login requests - backOffInSec = (int)ChooseRandom( - backOffInSec + jitter, - Math.Pow(s_exponentialFactor, retryCount) + jitter); - } - else if (backOffInSec < MAX_BACKOFF) - { - // Multiply sleep by 2 for non-login requests - backOffInSec *= 2; - } + logger.Debug($"Sleep {backOffInSec} seconds and then retry the request, retryCount: {retryCount}"); + + await Task.Delay(TimeSpan.FromSeconds(backOffInSec), cancellationToken).ConfigureAwait(false); + totalRetryTime += backOffInSec; + + var jitter = GetJitter(backOffInSec); + + // Set backoff time + if (isLoginRequest) + { + // Choose between previous sleep time and new base sleep time for login requests + backOffInSec = (int)ChooseRandom( + backOffInSec + jitter, + Math.Pow(s_exponentialFactor, retryCount) + jitter); + } + else if (backOffInSec < MAX_BACKOFF) + { + // Multiply sleep by 2 for non-login requests + backOffInSec *= 2; } if ((restTimeout.TotalSeconds > 0) && (totalRetryTime + backOffInSec > restTimeout.TotalSeconds)) @@ -485,11 +488,6 @@ protected override async Task SendAsync(HttpRequestMessage // then use the remaining connection timeout backOffInSec = Math.Min(backOffInSec, (int)restTimeout.TotalSeconds - totalRetryTime); } - - logger.Debug($"Sleep {backOffInSec} seconds and then retry the request, retryCount: {retryCount}"); - - await Task.Delay(TimeSpan.FromSeconds(backOffInSec), cancellationToken).ConfigureAwait(false); - totalRetryTime += backOffInSec; } } } From 4cc94f70703a5e9a6d47c87d004a2e4eb7894385 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Fri, 3 Nov 2023 11:36:34 -0700 Subject: [PATCH 19/21] SNOW-878067: Set maxHttpRetries to 0 to reach the defined timeout --- .../IntegrationTests/SFConnectionIT.cs | 12 ++++++------ Snowflake.Data/Core/HttpUtil.cs | 1 - 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 4f192094b..490ed1e5b 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -380,7 +380,7 @@ public void TestLoginTimeout() using (IDbConnection conn = new MockSnowflakeDbConnection()) { int timeoutSec = 5; - string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0}", + string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0};maxHttpRetries=0", timeoutSec); conn.ConnectionString = loginTimeOut5sec; @@ -404,8 +404,8 @@ public void TestLoginTimeout() // Should timeout before the defined timeout plus 1 (buffer time) Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); - // Should timeout after the minimum possible timeout with jitter - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000 - delta); + // Should timeout after the defined timeout since retry count is infinite + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 - delta); Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); } @@ -1724,7 +1724,7 @@ public void TestAsyncLoginTimeout() using (var conn = new MockSnowflakeDbConnection()) { int timeoutSec = 5; - string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0}", + string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0};maxHttpRetries=0", timeoutSec); conn.ConnectionString = loginTimeOut5sec; @@ -1745,8 +1745,8 @@ public void TestAsyncLoginTimeout() stopwatch.Stop(); int delta = 10; // in case server time slower. - // Should timeout after the minimum possible timeout with jitter - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, 1 * 1000 - delta); + // 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); diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 870dcb25e..834928afc 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -459,7 +459,6 @@ protected override async Task SendAsync(HttpRequestMessage requestMessage.RequestUri = updater.Update(errorReason); - logger.Debug($"Sleep {backOffInSec} seconds and then retry the request, retryCount: {retryCount}"); await Task.Delay(TimeSpan.FromSeconds(backOffInSec), cancellationToken).ConfigureAwait(false); From ef12cef4e1652daa0b1ef7845225934634a053db Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Fri, 3 Nov 2023 13:05:03 -0700 Subject: [PATCH 20/21] SNOW-878067: Remove OS Platform restriction on timeout tests --- .../IntegrationTests/SFConnectionIT.cs | 108 +++++++++--------- 1 file changed, 51 insertions(+), 57 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 490ed1e5b..7b661d8f6 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -375,40 +375,37 @@ public void TestConnectViaSecureString() [Test] public void TestLoginTimeout() { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + using (IDbConnection conn = new MockSnowflakeDbConnection()) { - using (IDbConnection conn = new MockSnowflakeDbConnection()) - { - int timeoutSec = 5; - string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0};maxHttpRetries=0", - timeoutSec); + int timeoutSec = 5; + string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0};maxHttpRetries=0", + timeoutSec); - conn.ConnectionString = loginTimeOut5sec; + conn.ConnectionString = loginTimeOut5sec; - Assert.AreEqual(conn.State, ConnectionState.Closed); - Stopwatch stopwatch = Stopwatch.StartNew(); - try - { - conn.Open(); - Assert.Fail(); - } - catch (AggregateException e) - { - // Jitter can cause the request to reach max number of retries before reaching the timeout - Assert.IsTrue(e.InnerException is TaskCanceledException || - SFError.REQUEST_TIMEOUT.GetAttribute().errorCode == - ((SnowflakeDbException)e.InnerException).ErrorCode); - } - stopwatch.Stop(); - int delta = 10; // in case server time slower. + Assert.AreEqual(conn.State, ConnectionState.Closed); + Stopwatch stopwatch = Stopwatch.StartNew(); + try + { + conn.Open(); + Assert.Fail(); + } + catch (AggregateException e) + { + // Jitter can cause the request to reach max number of retries before reaching the timeout + Assert.IsTrue(e.InnerException is TaskCanceledException || + SFError.REQUEST_TIMEOUT.GetAttribute().errorCode == + ((SnowflakeDbException)e.InnerException).ErrorCode); + } + stopwatch.Stop(); + int delta = 10; // in case server time slower. - // Should timeout before the defined timeout plus 1 (buffer time) - Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); - // Should timeout after the defined timeout since retry count is infinite - Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 - delta); + // Should timeout before the defined timeout plus 1 (buffer time) + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (timeoutSec + 1) * 1000); + // Should timeout after the defined timeout since retry count is infinite + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, timeoutSec * 1000 - delta); - Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); - } + Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); } } @@ -1719,40 +1716,37 @@ public void TestCancelLoginBeforeTimeout() [Test] public void TestAsyncLoginTimeout() { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + using (var conn = new MockSnowflakeDbConnection()) { - using (var conn = new MockSnowflakeDbConnection()) - { - int timeoutSec = 5; - string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0};maxHttpRetries=0", - timeoutSec); - conn.ConnectionString = loginTimeOut5sec; + int timeoutSec = 5; + string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0};maxHttpRetries=0", + timeoutSec); + conn.ConnectionString = loginTimeOut5sec; - Assert.AreEqual(conn.State, ConnectionState.Closed); + Assert.AreEqual(conn.State, ConnectionState.Closed); - Stopwatch stopwatch = Stopwatch.StartNew(); - try - { - Task connectTask = conn.OpenAsync(CancellationToken.None); - connectTask.Wait(); - } - catch (AggregateException e) - { - Assert.AreEqual(SFError.INTERNAL_ERROR.GetAttribute().errorCode, - ((SnowflakeDbException)e.InnerException).ErrorCode); + Stopwatch stopwatch = Stopwatch.StartNew(); + try + { + Task connectTask = conn.OpenAsync(CancellationToken.None); + connectTask.Wait(); + } + catch (AggregateException e) + { + Assert.AreEqual(SFError.INTERNAL_ERROR.GetAttribute().errorCode, + ((SnowflakeDbException)e.InnerException).ErrorCode); - } - stopwatch.Stop(); - int delta = 10; // in case server time slower. + } + stopwatch.Stop(); + int delta = 10; // in case server time slower. - // 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); + // 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); - Assert.AreEqual(ConnectionState.Closed, conn.State); - Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); - } + Assert.AreEqual(ConnectionState.Closed, conn.State); + Assert.AreEqual(timeoutSec, conn.ConnectionTimeout); } } From 4f511f46a28c9bfe9cfd1fc070463907d9f5f954 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf Date: Fri, 3 Nov 2023 17:01:30 -0700 Subject: [PATCH 21/21] SNOW-878067: Refactor setting which timeout to use --- .../IntegrationTests/SFConnectionIT.cs | 76 +++++++++++++++++++ .../UnitTests/HttpUtilTest.cs | 2 - .../Session/SFHttpClientPropertiesTest.cs | 3 +- Snowflake.Data/Core/HttpUtil.cs | 15 +--- .../Session/SFSessionHttpClientProperties.cs | 7 +- 5 files changed, 86 insertions(+), 17 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 7b661d8f6..eea852af2 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -442,6 +442,44 @@ public void TestLoginWithMaxRetryReached() } } + [Test] + public void TestLoginTimeoutWithRetryTimeoutLesserThanConnectionTimeout() + { + using (IDbConnection conn = new MockSnowflakeDbConnection()) + { + int connectionTimeout = 600; + int retryTimeout = 350; + string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0};retry_timeout={1};maxHttpRetries=0", + connectionTimeout, retryTimeout); + + conn.ConnectionString = loginTimeOut5sec; + + Assert.AreEqual(conn.State, ConnectionState.Closed); + Stopwatch stopwatch = Stopwatch.StartNew(); + try + { + conn.Open(); + Assert.Fail(); + } + catch (AggregateException e) + { + // Jitter can cause the request to reach max number of retries before reaching the timeout + Assert.IsTrue(e.InnerException is TaskCanceledException || + SFError.REQUEST_TIMEOUT.GetAttribute().errorCode == + ((SnowflakeDbException)e.InnerException).ErrorCode); + } + stopwatch.Stop(); + int delta = 10; // in case server time slower. + + // Should timeout before the defined timeout plus 1 (buffer time) + Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, (retryTimeout + 1) * 1000); + // Should timeout after the defined timeout since retry count is infinite + Assert.GreaterOrEqual(stopwatch.ElapsedMilliseconds, retryTimeout * 1000 - delta); + + Assert.AreEqual(retryTimeout, conn.ConnectionTimeout); + } + } + [Test] [Ignore("Disable unstable test cases for now")] public void TestDefaultLoginTimeout() @@ -1750,6 +1788,44 @@ public void TestAsyncLoginTimeout() } } + [Test] + public void TestAsyncLoginTimeoutWithRetryTimeoutLesserThanConnectionTimeout() + { + using (var conn = new MockSnowflakeDbConnection()) + { + int connectionTimeout = 600; + int retryTimeout = 350; + string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0};retry_timeout={1};maxHttpRetries=0", + connectionTimeout, retryTimeout); + conn.ConnectionString = loginTimeOut5sec; + + Assert.AreEqual(conn.State, ConnectionState.Closed); + + Stopwatch stopwatch = Stopwatch.StartNew(); + try + { + Task connectTask = conn.OpenAsync(CancellationToken.None); + connectTask.Wait(); + } + catch (AggregateException e) + { + Assert.AreEqual(SFError.INTERNAL_ERROR.GetAttribute().errorCode, + ((SnowflakeDbException)e.InnerException).ErrorCode); + + } + stopwatch.Stop(); + int delta = 10; // in case server time slower. + + // 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); + + Assert.AreEqual(ConnectionState.Closed, conn.State); + Assert.AreEqual(retryTimeout, conn.ConnectionTimeout); + } + } + [Test] public void TestAsyncDefaultLoginTimeout() { diff --git a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs index 4ac01d134..bf8063a7f 100644 --- a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs +++ b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs @@ -96,7 +96,6 @@ public void ShouldCreateHttpClientHandlerWithProxy() "localhost", false, false, - 300, 7 ); @@ -121,7 +120,6 @@ public void ShouldCreateHttpClientHandlerWithoutProxy() null, false, false, - 300, 0 ); diff --git a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs index 8337a3b66..ea6deb5ca 100644 --- a/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs +++ b/Snowflake.Data.Tests/UnitTests/Session/SFHttpClientPropertiesTest.cs @@ -87,7 +87,6 @@ public void ShouldBuildHttpClientConfig() Assert.AreEqual(properties.proxyProperties.nonProxyHosts, config.NoProxyList); Assert.AreEqual(properties.disableRetry, config.DisableRetry); Assert.AreEqual(properties.forceRetryOn404, config.ForceRetryOn404); - Assert.AreEqual(properties.retryTimeout, config.RetryTimeout); Assert.AreEqual(properties.maxHttpRetries, config.MaxHttpRetries); } @@ -264,7 +263,7 @@ public static IEnumerable PropertiesProvider() { validateDefaultParameters = true, clientSessionKeepAlive = false, - timeoutInSec = SFSessionHttpClientProperties.s_retryTimeoutDefault, + timeoutInSec = 0, insecureMode = false, disableRetry = false, forceRetryOn404 = false, diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 834928afc..b48d5a3a9 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -28,7 +28,6 @@ public HttpClientConfig( string noProxyList, bool disableRetry, bool forceRetryOn404, - int retryTimeout, int maxHttpRetries, bool includeRetryReason = true) { @@ -40,7 +39,6 @@ public HttpClientConfig( NoProxyList = noProxyList; DisableRetry = disableRetry; ForceRetryOn404 = forceRetryOn404; - RetryTimeout = retryTimeout; MaxHttpRetries = maxHttpRetries; IncludeRetryReason = includeRetryReason; @@ -54,7 +52,6 @@ public HttpClientConfig( noProxyList, disableRetry.ToString(), forceRetryOn404.ToString(), - retryTimeout.ToString(), maxHttpRetries.ToString(), includeRetryReason.ToString()}); } @@ -67,7 +64,6 @@ public HttpClientConfig( public readonly string NoProxyList; public readonly bool DisableRetry; public readonly bool ForceRetryOn404; - public readonly int RetryTimeout; public readonly int MaxHttpRetries; public readonly bool IncludeRetryReason; @@ -120,7 +116,7 @@ private HttpClient RegisterNewHttpClientIfNecessary(HttpClientConfig config) logger.Debug("Http client not registered. Adding."); var httpClient = new HttpClient( - new RetryHandler(SetupCustomHttpHandler(config), config.DisableRetry, config.ForceRetryOn404, config.RetryTimeout, config.MaxHttpRetries, config.IncludeRetryReason)) + new RetryHandler(SetupCustomHttpHandler(config), config.DisableRetry, config.ForceRetryOn404, config.MaxHttpRetries, config.IncludeRetryReason)) { Timeout = Timeout.InfiniteTimeSpan }; @@ -331,15 +327,13 @@ private class RetryHandler : DelegatingHandler private bool disableRetry; private bool forceRetryOn404; - private int retryTimeout; private int maxRetryCount; private bool includeRetryReason; - internal RetryHandler(HttpMessageHandler innerHandler, bool disableRetry, bool forceRetryOn404, int retryTimeout, int maxRetryCount, bool includeRetryReason) : base(innerHandler) + internal RetryHandler(HttpMessageHandler innerHandler, bool disableRetry, bool forceRetryOn404, int maxRetryCount, bool includeRetryReason) : base(innerHandler) { this.disableRetry = disableRetry; this.forceRetryOn404 = forceRetryOn404; - this.retryTimeout = retryTimeout; this.maxRetryCount = maxRetryCount; this.includeRetryReason = includeRetryReason; } @@ -358,10 +352,7 @@ protected override async Task SendAsync(HttpRequestMessage p.ConnectionLimit = 20; // Default value is 2, we need more connections for performing multiple parallel queries TimeSpan httpTimeout = (TimeSpan)requestMessage.Properties[SFRestRequest.HTTP_REQUEST_TIMEOUT_KEY]; - // Use the lower timeout value between rest timeout and max retry timeout - TimeSpan restTimeout = ((TimeSpan)requestMessage.Properties[SFRestRequest.REST_REQUEST_TIMEOUT_KEY]).TotalSeconds < retryTimeout ? - (TimeSpan)requestMessage.Properties[SFRestRequest.REST_REQUEST_TIMEOUT_KEY] : - TimeSpan.FromSeconds(retryTimeout); + TimeSpan restTimeout = (TimeSpan)requestMessage.Properties[SFRestRequest.REST_REQUEST_TIMEOUT_KEY]; if (logger.IsDebugEnabled()) { diff --git a/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs b/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs index 9a404a81f..b7466a362 100644 --- a/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs +++ b/Snowflake.Data/Core/Session/SFSessionHttpClientProperties.cs @@ -48,6 +48,12 @@ internal void CheckPropertiesAreValid() logger.Warn($"Max retry timeout provided is 0. Timeout will be infinite"); } + // Use the shorter timeout between CONNECTION_TIMEOUT and RETRY_TIMEOUT + if (retryTimeout < timeoutInSec) + { + timeoutInSec = retryTimeout; + } + if (maxHttpRetries > 0 && maxHttpRetries < s_maxHttpRetriesDefault) { logger.Warn($"Max retry count provided is less than the allowed minimum value of" + @@ -77,7 +83,6 @@ internal HttpClientConfig BuildHttpClientConfig() proxyProperties.nonProxyHosts, disableRetry, forceRetryOn404, - retryTimeout, maxHttpRetries, includeRetryReason); }