SNOW-878067: Retry Strategy

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(SFSessionHttpClientProperties.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 + SFSessionHttpClientProperties.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<SFErrorAttr>().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<SFErrorAttr>().errorCode ==
                             ((SnowflakeDbException)e.InnerException).ErrorCode);
                     }
                     stopwatch.Stop();
-                    int detla = 10; //in case server time slower.
+                    int delta = 100; // 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 + delta);
+                    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<SFErrorAttr>().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, SFSessionHttpClientProperties.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 + SFSessionHttpClientProperties.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 + SFSessionHttpClientProperties.s_connectionTimeoutDefault;
                     string loginTimeOut5sec = String.Format(ConnectionString + "connection_timeout={0}",
                         timeoutSec);
                     conn.ConnectionString = loginTimeOut5sec;
@@ -1735,15 +1743,15 @@ public void TestAsyncLoginTimeout()
 
                     }
                     stopwatch.Stop();
-                    int detla = 10; //in case server time slower.
+                    int delta = 100; // 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 + delta);
 
                     Assert.AreEqual(ConnectionState.Closed, conn.State);
-                    Assert.AreEqual(5, conn.ConnectionTimeout);
+                    Assert.AreEqual(timeoutSec, conn.ConnectionTimeout);
                 }
             }
         }
@@ -1770,15 +1778,15 @@ 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 (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, SFSessionHttpClientProperties.s_connectionTimeoutDefault * 1000 - delta);
+                // But never more because there's no connection timeout remaining
+                Assert.LessOrEqual(stopwatch.ElapsedMilliseconds, SFSessionHttpClientProperties.s_connectionTimeoutDefault * 1000 + delta);
 
                 Assert.AreEqual(ConnectionState.Closed, conn.State);
-                Assert.AreEqual(120, conn.ConnectionTimeout);
+                Assert.AreEqual(SFSessionHttpClientProperties.s_connectionTimeoutDefault, conn.ConnectionTimeout);
             }
         }
 

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<object> t = command.ExecuteScalarAsync();
+                        t.Wait();
+                    }
+                    Assert.Fail();
+                }
+                catch (Exception e)
+                {
+                    Assert.IsInstanceOf<TaskCanceledException>(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<TaskCanceledException>(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()
         {

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<HttpResponseMessage> 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));
         }
     }

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()
         {

Snowflake.Data.Tests/UnitTests/SFSessionPropertyTest.cs

@@ -48,7 +48,7 @@ public static IEnumerable<TestCase> 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";