Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SNOW-1276398 Fix not thrown exception when OpenAsync http request fails #999

Merged
merged 7 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 48 additions & 1 deletion Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public void TestConnectionIsNotMarkedAsOpenWhenWasNotCorrectlyOpenedBefore(bool
}

[Test]
public void TestConnectionIsNotMarkedAsOpenWhenWasNotCorrectlyOpenedWithUsingClause()
public void TestConnectionIsNotMarkedAsOpenWhenWfasNotCorrectlyOpenedWithUsingClause()
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved
{
for (int i = 0; i < 2; ++i)
{
Expand Down Expand Up @@ -2271,6 +2271,53 @@ public void TestUseMultiplePoolsConnectionPoolByDefault()
// assert
Assert.AreEqual(ConnectionPoolType.MultipleConnectionPool, poolVersion);
}

[Test]
public async Task TestShouldThrowExceptionWhenTaskCanceledWhenOpenAsync()
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved
{
// act
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved
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";
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved

using (var connection = new SnowflakeDbConnection())
{
try{
connection.ConnectionString = connectionString;
Task t = connection.OpenAsync();
t.Wait();
}
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved
catch (Exception e)
{
Assert.IsInstanceOf<SnowflakeDbException>(e.InnerException);
}
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved
}
}

[Test]
public async Task TestConnectionAsyncTimeoutWithMaxRetryReached()
{
var mockRestRequester = new MockRetryUntilRestTimeoutRestRequester()
{
_forceTimeoutForNonLoginRequestsOnly = false
};

using (DbConnection connection = new MockSnowflakeDbConnection(mockRestRequester))
{
string maxRetryConnStr = ConnectionString + "maxHttpRetries=8;poolingEnabled=true";
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved

connection.ConnectionString = maxRetryConnStr;
try{
Task t = connection.OpenAsync();
t.Wait();
}
catch (Exception e)
{
Assert.IsInstanceOf<TaskCanceledException>(e.InnerException);
}
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}

Expand Down
7 changes: 1 addition & 6 deletions Snowflake.Data/Client/SnowflakeDbConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -318,11 +318,6 @@ public override Task OpenAsync(CancellationToken cancellationToken)
SFError.INTERNAL_ERROR,
"Unable to connect");
}
else if (previousTask.IsCanceled)
{
_connectionState = ConnectionState.Closed;
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved
logger.Debug("Connection canceled");
}
else
{
// Only continue if the session was opened successfully
Expand All @@ -331,7 +326,7 @@ public override Task OpenAsync(CancellationToken cancellationToken)
OnSessionEstablished();
}
},
cancellationToken);
TaskContinuationOptions.NotOnCanceled);
}

public Mutex GetArrayBindingMutex()
Expand Down
26 changes: 18 additions & 8 deletions Snowflake.Data/Core/HttpUtil.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

namespace Snowflake.Data.Core
{
using Client;
sfc-gh-knozderko marked this conversation as resolved.
Show resolved Hide resolved

public class HttpClientConfig
{
public HttpClientConfig(
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -373,7 +375,7 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage

while (true)
{

Exception toThrow = null;
try
{
childCts = null;
Expand All @@ -384,7 +386,7 @@ protected override async Task<HttpResponseMessage> 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);
Expand All @@ -406,6 +408,7 @@ protected override async Task<HttpResponseMessage> 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)
Expand Down Expand Up @@ -454,7 +457,14 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
{
return response;
}
throw new OperationCanceledException($"http request failed and max retry {maxRetryCount} reached");

if (toThrow is TaskCanceledException)
{
throw toThrow;
}
throw new SnowflakeDbException(toThrow, 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
Expand Down
Loading