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] 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)]