Skip to content

Commit

Permalink
SNOW-1502317: Stop retry on nonrecoverable exception
Browse files Browse the repository at this point in the history
  • Loading branch information
sfc-gh-ext-simba-lf committed Aug 29, 2024
1 parent 60c2f2b commit 1f51027
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 1 deletion.
34 changes: 34 additions & 0 deletions Snowflake.Data.Tests/IntegrationTests/SFConnectionIT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace Snowflake.Data.Tests.IntegrationTests
using Snowflake.Data.Tests.Mock;
using System.Runtime.InteropServices;
using System.Net.Http;
using System.Security.Authentication;

[TestFixture]
class SFConnectionIT : SFBaseTest
Expand Down Expand Up @@ -581,6 +582,39 @@ public void TestEnableLoginRetryOn404()
}
}


[Test]
public void TestAuthenticationExceptionThrowsExceptionAndNotRetried()
{
var mockRestRequester = new MockInfiniteTimeout();

using (var conn = new MockSnowflakeDbConnection(mockRestRequester))
{
string invalidConnectionString = "host=google.com/404;"
+ "connection_timeout=0;account=testFailFast;user=testFailFast;password=testFailFast;disableretry=true;forceretryon404=true";
conn.ConnectionString = invalidConnectionString;

Assert.AreEqual(conn.State, ConnectionState.Closed);
try
{
conn.Open();
Assert.Fail();
}
catch (AggregateException e)
{
Assert.IsInstanceOf<HttpRequestException>(e.InnerException);
Assert.IsInstanceOf<AuthenticationException>(e.InnerException.InnerException);
Assert.IsTrue(e.InnerException.InnerException.Message.Contains("The remote certificate is invalid because of errors in the certificate chain: RevocationStatusUnknown"));
}
catch (Exception unexpected)
{
Assert.Fail($"Unexpected {unexpected.GetType()} exception occurred");
}

Assert.AreEqual(ConnectionState.Closed, conn.State);
}
}

[Test]
public void TestValidateDefaultParameters()
{
Expand Down
36 changes: 36 additions & 0 deletions Snowflake.Data.Tests/Mock/MockInfiniteTimeout.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright (c) 2024 Snowflake Computing Inc. All rights reserved.
*/

using Snowflake.Data.Core;
using System;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;

namespace Snowflake.Data.Tests.Mock
{

class MockInfiniteTimeout : RestRequester, IMockRestRequester
{
public MockInfiniteTimeout() : base(null)
{
// Does nothing
}

public void setHttpClient(HttpClient httpClient)
{
base._HttpClient = httpClient;
}

protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage message,
TimeSpan restTimeout,
CancellationToken externalCancellationToken,
string sid = "")
{
message.Properties[BaseRestRequest.HTTP_REQUEST_TIMEOUT_KEY] = Timeout.InfiniteTimeSpan;
return await (base.SendAsync(message, restTimeout, externalCancellationToken).ConfigureAwait(false));
}
}
}

24 changes: 23 additions & 1 deletion Snowflake.Data/Core/HttpUtil.cs
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
bool isOktaSSORequest = IsOktaSSORequest(requestMessage.RequestUri.Host, absolutePath);
int backOffInSec = s_baseBackOffTime;
int totalRetryTime = 0;
Exception lastException = null;

ServicePoint p = ServicePointManager.FindServicePoint(requestMessage.RequestUri);
p.Expect100Continue = false; // Saves about 100 ms per request
Expand Down Expand Up @@ -391,6 +392,7 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
}
catch (Exception e)
{
lastException = e;
if (cancellationToken.IsCancellationRequested)
{
logger.Debug("SF rest request timeout or explicit cancel called.");
Expand All @@ -401,10 +403,23 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
logger.Warn("Http request timeout. Retry the request");
totalRetryTime += (int)httpTimeout.TotalSeconds;
}
else if (e is HttpRequestException && e.InnerException is AuthenticationException)
{
logger.Error("Non-retryable error encountered: ", e);
if (e.InnerException != null)
{
logger.Error("Details on inner exception: ", e.InnerException);
}
throw;
}
else
{
//TODO: Should probably check to see if the error is recoverable or transient.
logger.Warn("Error occurred during request, retrying...", e);
if (e.InnerException != null)
{
logger.Warn("Details on inner exception: ", e.InnerException);
}
}
}

Expand Down Expand Up @@ -454,7 +469,14 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
{
return response;
}
throw new OperationCanceledException($"http request failed and max retry {maxRetryCount} reached");
var errorMessage = $"http request failed and max retry {maxRetryCount} reached.\n";
errorMessage += $"Last exception encountered: {lastException.Message}\n";
if (lastException.InnerException != null)
{
errorMessage += $"Details on inner exception: {lastException.InnerException.Message}";
}
logger.Error(errorMessage);
throw new OperationCanceledException(errorMessage);
}

// Disposing of the response if not null now that we don't need it anymore
Expand Down

0 comments on commit 1f51027

Please sign in to comment.