From ec0c0fa1828757f1327f0034124b1cd532c1db93 Mon Sep 17 00:00:00 2001 From: Juan Martinez Ramirez Date: Mon, 29 Jul 2024 12:58:05 -0600 Subject: [PATCH 1/7] Test change to throw exception when openasync http request fails. --- Snowflake.Data/Core/HttpUtil.cs | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 531e76fd7..96d502f40 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -17,6 +17,8 @@ namespace Snowflake.Data.Core { + using Client; + public class HttpClientConfig { public HttpClientConfig( @@ -87,7 +89,7 @@ public sealed class HttpUtil private HttpUtil() { - // This value is used by AWS SDK and can cause deadlock, + // This value is used by AWS SDK and can cause deadlock, // so we need to increase the default value of 2 // See: https://github.com/aws/aws-sdk-net/issues/152 ServicePointManager.DefaultConnectionLimit = 50; @@ -181,15 +183,15 @@ internal HttpMessageHandler SetupCustomHttpHandler(HttpClientConfig config) { // Get the original entry entry = bypassList[i].Trim(); - // . -> [.] because . means any char + // . -> [.] because . means any char entry = entry.Replace(".", "[.]"); // * -> .* because * is a quantifier and need a char or group to apply to entry = entry.Replace("*", ".*"); - + entry = entry.StartsWith("^") ? entry : $"^{entry}"; - + entry = entry.EndsWith("$") ? entry : $"{entry}$"; - + // Replace with the valid entry syntax bypassList[i] = entry; @@ -373,7 +375,7 @@ protected override async Task SendAsync(HttpRequestMessage while (true) { - + Exception toThrow = null; try { childCts = null; @@ -384,7 +386,7 @@ protected override async Task SendAsync(HttpRequestMessage if (httpTimeout.Ticks == 0) childCts.Cancel(); else - childCts.CancelAfter(httpTimeout); + childCts.CancelAfter(httpTimeout); } response = await base.SendAsync(requestMessage, childCts == null ? cancellationToken : childCts.Token).ConfigureAwait(false); @@ -406,6 +408,7 @@ protected override async Task SendAsync(HttpRequestMessage //TODO: Should probably check to see if the error is recoverable or transient. logger.Warn("Error occurred during request, retrying...", e); } + toThrow = e; } if (childCts != null) @@ -454,7 +457,16 @@ protected override async Task SendAsync(HttpRequestMessage { return response; } - throw new OperationCanceledException($"http request failed and max retry {maxRetryCount} reached"); + + + throw new SnowflakeDbException(toThrow, SFError.INTERNAL_ERROR, + $"Http request failed and max retry {maxRetryCount} reached"); + + } + + if (childCts.IsCancellationRequested) + { + throw new OperationCanceledException($"cancelado!!!"); } // Disposing of the response if not null now that we don't need it anymore From a0afc1fbbb4a738a458654ffef098369f4cf1d6a Mon Sep 17 00:00:00 2001 From: Juan Martinez Ramirez Date: Mon, 29 Jul 2024 15:55:59 -0600 Subject: [PATCH 2/7] Added mechanism to handle task cancelation for timeouts and errors when trying to execute a HTTP request --- .../IntegrationTests/SFConnectionIT.cs | 49 ++++++++++++++++++- .../Client/SnowflakeDbConnection.cs | 7 +-- Snowflake.Data/Core/HttpUtil.cs | 10 ++-- 3 files changed, 53 insertions(+), 13 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 7c17f9bbd..c9d30576c 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -164,7 +164,7 @@ public void TestConnectionIsNotMarkedAsOpenWhenWasNotCorrectlyOpenedBefore(bool } [Test] - public void TestConnectionIsNotMarkedAsOpenWhenWasNotCorrectlyOpenedWithUsingClause() + public void TestConnectionIsNotMarkedAsOpenWhenWfasNotCorrectlyOpenedWithUsingClause() { for (int i = 0; i < 2; ++i) { @@ -2271,6 +2271,53 @@ public void TestUseMultiplePoolsConnectionPoolByDefault() // assert Assert.AreEqual(ConnectionPoolType.MultipleConnectionPool, poolVersion); } + + [Test] + public async Task TestShouldThrowExceptionWhenTaskCanceledWhenOpenAsync() + { + // act + var connectionString = "account=myaccount.servername; " + + " user=admin; password=password; role=ACCOUNTADMIN;" + + " DB=TEST_DB; warehouse=COMPUTE_WH;" + + " useproxy=true;proxyhost=no.such.pro.xy;proxyport=8080"; + + using (var connection = new SnowflakeDbConnection()) + { + try{ + connection.ConnectionString = connectionString; + Task t = connection.OpenAsync(); + t.Wait(); + } + catch (Exception e) + { + Assert.IsInstanceOf(e.InnerException); + } + } + } + + [Test] + public async Task TestConnectionAsyncTimeoutWithMaxRetryReached() + { + var mockRestRequester = new MockRetryUntilRestTimeoutRestRequester() + { + _forceTimeoutForNonLoginRequestsOnly = false + }; + + using (DbConnection connection = new MockSnowflakeDbConnection(mockRestRequester)) + { + string maxRetryConnStr = ConnectionString + "maxHttpRetries=8;poolingEnabled=true"; + + connection.ConnectionString = maxRetryConnStr; + try{ + Task t = connection.OpenAsync(); + t.Wait(); + } + catch (Exception e) + { + Assert.IsInstanceOf(e.InnerException); + } + } + } } } diff --git a/Snowflake.Data/Client/SnowflakeDbConnection.cs b/Snowflake.Data/Client/SnowflakeDbConnection.cs index fc0ba199d..acaa88ba0 100755 --- a/Snowflake.Data/Client/SnowflakeDbConnection.cs +++ b/Snowflake.Data/Client/SnowflakeDbConnection.cs @@ -318,11 +318,6 @@ public override Task OpenAsync(CancellationToken cancellationToken) SFError.INTERNAL_ERROR, "Unable to connect"); } - else if (previousTask.IsCanceled) - { - _connectionState = ConnectionState.Closed; - logger.Debug("Connection canceled"); - } else { // Only continue if the session was opened successfully @@ -331,7 +326,7 @@ public override Task OpenAsync(CancellationToken cancellationToken) OnSessionEstablished(); } }, - cancellationToken); + TaskContinuationOptions.NotOnCanceled); } public Mutex GetArrayBindingMutex() diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 96d502f40..74a1e8751 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -458,17 +458,15 @@ protected override async Task SendAsync(HttpRequestMessage return response; } - + if (toThrow is TaskCanceledException) + { + throw toThrow; + } throw new SnowflakeDbException(toThrow, SFError.INTERNAL_ERROR, $"Http request failed and max retry {maxRetryCount} reached"); } - if (childCts.IsCancellationRequested) - { - throw new OperationCanceledException($"cancelado!!!"); - } - // Disposing of the response if not null now that we don't need it anymore response?.Dispose(); From 417672a77052b815c5af0a9823ad806da41dbbd3 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Fri, 2 Aug 2024 15:08:29 +0000 Subject: [PATCH 3/7] some changes --- .../IntegrationTests/SFConnectionIT.cs | 56 ++++++++----------- .../Client/SnowflakeDbConnection.cs | 9 ++- Snowflake.Data/Core/HttpUtil.cs | 18 +++--- 3 files changed, 37 insertions(+), 46 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index c9d30576c..ea48aab1f 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -164,7 +164,7 @@ public void TestConnectionIsNotMarkedAsOpenWhenWasNotCorrectlyOpenedBefore(bool } [Test] - public void TestConnectionIsNotMarkedAsOpenWhenWfasNotCorrectlyOpenedWithUsingClause() + public void TestConnectionIsNotMarkedAsOpenWhenWasNotCorrectlyOpenedWithUsingClause() { for (int i = 0; i < 2; ++i) { @@ -2273,49 +2273,37 @@ public void TestUseMultiplePoolsConnectionPoolByDefault() } [Test] - public async Task TestShouldThrowExceptionWhenTaskCanceledWhenOpenAsync() + public void TestOpenAsyncThrowExceptionWhenConnectToUnreachableHost() { - // act - var connectionString = "account=myaccount.servername; " + - " user=admin; password=password; role=ACCOUNTADMIN;" + - " DB=TEST_DB; warehouse=COMPUTE_WH;" + - " useproxy=true;proxyhost=no.such.pro.xy;proxyport=8080"; - - using (var connection = new SnowflakeDbConnection()) + // arrange + var connectionString = "account=testAccount;user=testUser;password=testPassword;useProxy=true;proxyHost=no.such.pro.xy;proxyPort=8080;" + + "connection_timeout=5;"; + using (var connection = new SnowflakeDbConnection(connectionString)) { - try{ - connection.ConnectionString = connectionString; - Task t = connection.OpenAsync(); - t.Wait(); - } - catch (Exception e) - { - Assert.IsInstanceOf(e.InnerException); - } + // act + var thrown = Assert.Throws(() => connection.OpenAsync().Wait()); + + // assert + SnowflakeDbExceptionAssert.HasErrorCode(thrown.InnerException, SFError.INTERNAL_ERROR); + Assert.AreEqual(ConnectionState.Closed, connection.State); } } [Test] - public async Task TestConnectionAsyncTimeoutWithMaxRetryReached() + public void TestOpenAsyncThrowExceptionWhenOperationIsCancelled() { - var mockRestRequester = new MockRetryUntilRestTimeoutRestRequester() + // arrange + var connectionString = "account=testAccount;user=testUser;password=testPassword;useProxy=true;proxyHost=no.such.pro.xy;proxyPort=8080;"; + using (var connection = new SnowflakeDbConnection(connectionString)) { - _forceTimeoutForNonLoginRequestsOnly = false - }; + var shortCancellation = new CancellationTokenSource(TimeSpan.FromSeconds(5)); - using (DbConnection connection = new MockSnowflakeDbConnection(mockRestRequester)) - { - string maxRetryConnStr = ConnectionString + "maxHttpRetries=8;poolingEnabled=true"; + // act + var thrown = Assert.Throws(() => connection.OpenAsync(shortCancellation.Token).Wait()); - connection.ConnectionString = maxRetryConnStr; - try{ - Task t = connection.OpenAsync(); - t.Wait(); - } - catch (Exception e) - { - Assert.IsInstanceOf(e.InnerException); - } + // assert + Assert.IsInstanceOf(thrown.InnerException); + Assert.AreEqual(ConnectionState.Closed, connection.State); } } } diff --git a/Snowflake.Data/Client/SnowflakeDbConnection.cs b/Snowflake.Data/Client/SnowflakeDbConnection.cs index acaa88ba0..680e761b7 100755 --- a/Snowflake.Data/Client/SnowflakeDbConnection.cs +++ b/Snowflake.Data/Client/SnowflakeDbConnection.cs @@ -318,6 +318,12 @@ public override Task OpenAsync(CancellationToken cancellationToken) SFError.INTERNAL_ERROR, "Unable to connect"); } + else if (previousTask.IsCanceled) + { + _connectionState = ConnectionState.Closed; + logger.Debug("Connection canceled"); + throw new TaskCanceledException("Connecting was cancelled"); + } else { // Only continue if the session was opened successfully @@ -325,8 +331,7 @@ public override Task OpenAsync(CancellationToken cancellationToken) logger.Debug($"Connection open with pooled session: {SfSession.sessionId}"); OnSessionEstablished(); } - }, - TaskContinuationOptions.NotOnCanceled); + }, TaskContinuationOptions.None); } public Mutex GetArrayBindingMutex() diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 74a1e8751..52f5eecf1 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -13,12 +13,11 @@ using System.Web; using System.Security.Authentication; using System.Linq; +using Snowflake.Data.Client; using Snowflake.Data.Core.Authenticator; namespace Snowflake.Data.Core { - using Client; - public class HttpClientConfig { public HttpClientConfig( @@ -375,7 +374,7 @@ protected override async Task SendAsync(HttpRequestMessage while (true) { - Exception toThrow = null; + TaskCanceledException cancelException = null; try { childCts = null; @@ -408,7 +407,8 @@ protected override async Task SendAsync(HttpRequestMessage //TODO: Should probably check to see if the error is recoverable or transient. logger.Warn("Error occurred during request, retrying...", e); } - toThrow = e; + if (e is TaskCanceledException) + cancelException = (TaskCanceledException) e; } if (childCts != null) @@ -458,13 +458,11 @@ protected override async Task SendAsync(HttpRequestMessage return response; } - if (toThrow is TaskCanceledException) - { - throw toThrow; - } - throw new SnowflakeDbException(toThrow, SFError.INTERNAL_ERROR, - $"Http request failed and max retry {maxRetryCount} reached"); + if (cancelException != null) + throw cancelException; + throw new SnowflakeDbException(cancelException, SFError.INTERNAL_ERROR, + $"Http request failed and max retry {maxRetryCount} reached"); } // Disposing of the response if not null now that we don't need it anymore From 28ac6f7f9569529a4136ddf409ecdcd3e70a83f5 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Fri, 2 Aug 2024 15:57:11 +0000 Subject: [PATCH 4/7] more changes --- Snowflake.Data/Client/SnowflakeDbConnection.cs | 2 +- Snowflake.Data/Core/HttpUtil.cs | 11 ++--------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/Snowflake.Data/Client/SnowflakeDbConnection.cs b/Snowflake.Data/Client/SnowflakeDbConnection.cs index 680e761b7..edc4b1e4e 100755 --- a/Snowflake.Data/Client/SnowflakeDbConnection.cs +++ b/Snowflake.Data/Client/SnowflakeDbConnection.cs @@ -331,7 +331,7 @@ public override Task OpenAsync(CancellationToken cancellationToken) logger.Debug($"Connection open with pooled session: {SfSession.sessionId}"); OnSessionEstablished(); } - }, TaskContinuationOptions.None); + }, TaskContinuationOptions.None); // this continuation should be executed always (even if the whole operation was canceled) because it sets the proper state of the connection } public Mutex GetArrayBindingMutex() diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 52f5eecf1..5e401783f 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -374,7 +374,6 @@ protected override async Task SendAsync(HttpRequestMessage while (true) { - TaskCanceledException cancelException = null; try { childCts = null; @@ -407,8 +406,6 @@ protected override async Task SendAsync(HttpRequestMessage //TODO: Should probably check to see if the error is recoverable or transient. logger.Warn("Error occurred during request, retrying...", e); } - if (e is TaskCanceledException) - cancelException = (TaskCanceledException) e; } if (childCts != null) @@ -457,12 +454,8 @@ protected override async Task SendAsync(HttpRequestMessage { return response; } - - if (cancelException != null) - throw cancelException; - - throw new SnowflakeDbException(cancelException, SFError.INTERNAL_ERROR, - $"Http request failed and max retry {maxRetryCount} reached"); + throw new SnowflakeDbException(new OperationCanceledException($"http request failed and max retry {maxRetryCount} reached"), + SFError.INTERNAL_ERROR, "Unable to connect"); } // Disposing of the response if not null now that we don't need it anymore From e18d8ebc2aba25acd5607d1c856cd45c86ae569b Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Fri, 2 Aug 2024 17:02:27 +0000 Subject: [PATCH 5/7] fix failing tests --- .../IntegrationTests/SFConnectionIT.cs | 4 +++- Snowflake.Data/Core/HttpUtil.cs | 17 ++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index ea48aab1f..019d4311e 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -2284,7 +2284,9 @@ public void TestOpenAsyncThrowExceptionWhenConnectToUnreachableHost() var thrown = Assert.Throws(() => connection.OpenAsync().Wait()); // assert - SnowflakeDbExceptionAssert.HasErrorCode(thrown.InnerException, SFError.INTERNAL_ERROR); + Assert.IsTrue(thrown.InnerException is TaskCanceledException || thrown.InnerException is SnowflakeDbException); + if (thrown.InnerException is SnowflakeDbException) + SnowflakeDbExceptionAssert.HasErrorCode(thrown.InnerException, SFError.INTERNAL_ERROR); Assert.AreEqual(ConnectionState.Closed, connection.State); } } diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 5e401783f..531e76fd7 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -13,7 +13,6 @@ using System.Web; using System.Security.Authentication; using System.Linq; -using Snowflake.Data.Client; using Snowflake.Data.Core.Authenticator; namespace Snowflake.Data.Core @@ -88,7 +87,7 @@ public sealed class HttpUtil private HttpUtil() { - // This value is used by AWS SDK and can cause deadlock, + // This value is used by AWS SDK and can cause deadlock, // so we need to increase the default value of 2 // See: https://github.com/aws/aws-sdk-net/issues/152 ServicePointManager.DefaultConnectionLimit = 50; @@ -182,15 +181,15 @@ internal HttpMessageHandler SetupCustomHttpHandler(HttpClientConfig config) { // Get the original entry entry = bypassList[i].Trim(); - // . -> [.] because . means any char + // . -> [.] because . means any char entry = entry.Replace(".", "[.]"); // * -> .* because * is a quantifier and need a char or group to apply to entry = entry.Replace("*", ".*"); - + entry = entry.StartsWith("^") ? entry : $"^{entry}"; - + entry = entry.EndsWith("$") ? entry : $"{entry}$"; - + // Replace with the valid entry syntax bypassList[i] = entry; @@ -374,6 +373,7 @@ protected override async Task SendAsync(HttpRequestMessage while (true) { + try { childCts = null; @@ -384,7 +384,7 @@ protected override async Task SendAsync(HttpRequestMessage if (httpTimeout.Ticks == 0) childCts.Cancel(); else - childCts.CancelAfter(httpTimeout); + childCts.CancelAfter(httpTimeout); } response = await base.SendAsync(requestMessage, childCts == null ? cancellationToken : childCts.Token).ConfigureAwait(false); @@ -454,8 +454,7 @@ protected override async Task SendAsync(HttpRequestMessage { return response; } - throw new SnowflakeDbException(new OperationCanceledException($"http request failed and max retry {maxRetryCount} reached"), - SFError.INTERNAL_ERROR, "Unable to connect"); + 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 From 0d577761dab05c97bcfbc95acccfa0967de5a871 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Fri, 2 Aug 2024 18:15:15 +0000 Subject: [PATCH 6/7] add test failing on the previous version --- .../IntegrationTests/SFConnectionIT.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 019d4311e..8ab8f9e3b 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -2273,11 +2273,13 @@ public void TestUseMultiplePoolsConnectionPoolByDefault() } [Test] - public void TestOpenAsyncThrowExceptionWhenConnectToUnreachableHost() + [TestCase("connection_timeout=5;")] + [TestCase("")] + public void TestOpenAsyncThrowExceptionWhenConnectToUnreachableHost(string extraParameters) { // arrange - var connectionString = "account=testAccount;user=testUser;password=testPassword;useProxy=true;proxyHost=no.such.pro.xy;proxyPort=8080;" - + "connection_timeout=5;"; + var connectionString = "account=testAccount;user=testUser;password=testPassword;useProxy=true;proxyHost=no.such.pro.xy;proxyPort=8080;" + + extraParameters; using (var connection = new SnowflakeDbConnection(connectionString)) { // act @@ -2298,7 +2300,7 @@ public void TestOpenAsyncThrowExceptionWhenOperationIsCancelled() var connectionString = "account=testAccount;user=testUser;password=testPassword;useProxy=true;proxyHost=no.such.pro.xy;proxyPort=8080;"; using (var connection = new SnowflakeDbConnection(connectionString)) { - var shortCancellation = new CancellationTokenSource(TimeSpan.FromSeconds(5)); + var shortCancellation = new CancellationTokenSource(TimeSpan.FromMilliseconds(10)); // act var thrown = Assert.Throws(() => connection.OpenAsync(shortCancellation.Token).Wait()); From e59c40e470bb0c21b64e395b3103d9168a1f35e6 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Fri, 2 Aug 2024 18:25:56 +0000 Subject: [PATCH 7/7] change test params --- Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs index 8ab8f9e3b..5f08e4051 100644 --- a/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs +++ b/Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs @@ -2300,7 +2300,7 @@ public void TestOpenAsyncThrowExceptionWhenOperationIsCancelled() var connectionString = "account=testAccount;user=testUser;password=testPassword;useProxy=true;proxyHost=no.such.pro.xy;proxyPort=8080;"; using (var connection = new SnowflakeDbConnection(connectionString)) { - var shortCancellation = new CancellationTokenSource(TimeSpan.FromMilliseconds(10)); + var shortCancellation = new CancellationTokenSource(TimeSpan.FromSeconds(5)); // act var thrown = Assert.Throws(() => connection.OpenAsync(shortCancellation.Token).Wait());