Skip to content

Commit

Permalink
SNOW-1502317: Stop retry on nonrecoverable exception (#1013)
Browse files Browse the repository at this point in the history
  • Loading branch information
@sfc-gh-ext-simba-lf
sfc-gh-ext-simba-lf authored Sep 18, 2024
1 parent caace83 commit aef4d3a
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 8 deletions.
42 changes: 42 additions & 0 deletions Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,55 @@ namespace Snowflake.Data.Tests.UnitTests
using NUnit.Framework;
using Snowflake.Data.Core;
using RichardSzalay.MockHttp;
using System.Threading;
using System.Threading.Tasks;
using System.Net;
using System;
using System.Security.Authentication;
using Moq;
using Moq.Protected;

[TestFixture]
class HttpUtilTest
{
[Test]
public async Task TestNonRetryableHttpExceptionThrowsError()
{
var request = new HttpRequestMessage(HttpMethod.Post, new Uri("https://authenticationexceptiontest.com/"));
// Disable warning as this is the way to be compliant with netstandard2.0
// API reference: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httprequestmessage?view=netstandard-2.0
#pragma warning disable CS0618 // Type or member is obsolete
request.Properties[BaseRestRequest.HTTP_REQUEST_TIMEOUT_KEY] = Timeout.InfiniteTimeSpan;
request.Properties[BaseRestRequest.REST_REQUEST_TIMEOUT_KEY] = Timeout.InfiniteTimeSpan;
#pragma warning restore CS0618 // Type or member is obsolete

var handler = new Mock<DelegatingHandler>();
handler.Protected()
.Setup<Task<HttpResponseMessage>>(
"SendAsync",
ItExpr.Is<HttpRequestMessage>(req => req.RequestUri.ToString().Contains("https://authenticationexceptiontest.com/")),
ItExpr.IsAny<CancellationToken>())
.ThrowsAsync(new HttpRequestException("", new AuthenticationException()));

var httpClient = HttpUtil.Instance.GetHttpClient(
new HttpClientConfig(false, "fakeHost", "fakePort", "user", "password", "fakeProxyList", false, false, 7),
handler.Object);

try
{
await httpClient.SendAsync(request, CancellationToken.None).ConfigureAwait(false);
Assert.Fail();
}
catch (HttpRequestException e)
{
Assert.IsInstanceOf<AuthenticationException>(e.InnerException);
}
catch (Exception unexpected)
{
Assert.Fail($"Unexpected {unexpected.GetType()} exception occurred");
}
}

[Test]
// Parameters: status code, force retry on 404, expected retryable value
[TestCase(HttpStatusCode.OK, false, false)]
Expand Down
44 changes: 36 additions & 8 deletions Snowflake.Data/Core/HttpUtil.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,24 +100,24 @@ private HttpUtil()

private Dictionary<string, HttpClient> _HttpClients = new Dictionary<string, HttpClient>();

internal HttpClient GetHttpClient(HttpClientConfig config)
internal HttpClient GetHttpClient(HttpClientConfig config, DelegatingHandler customHandler = null)
{
lock (httpClientProviderLock)
{
return RegisterNewHttpClientIfNecessary(config);
return RegisterNewHttpClientIfNecessary(config, customHandler);
}
}


private HttpClient RegisterNewHttpClientIfNecessary(HttpClientConfig config)
private HttpClient RegisterNewHttpClientIfNecessary(HttpClientConfig config, DelegatingHandler customHandler = null)
{
string name = config.ConfKey;
if (!_HttpClients.ContainsKey(name))
{
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, customHandler), config.DisableRetry, config.ForceRetryOn404, config.MaxHttpRetries, config.IncludeRetryReason))
{
Timeout = Timeout.InfiniteTimeSpan
};
Expand All @@ -129,8 +129,13 @@ private HttpClient RegisterNewHttpClientIfNecessary(HttpClientConfig config)
return _HttpClients[name];
}

internal HttpMessageHandler SetupCustomHttpHandler(HttpClientConfig config)
internal HttpMessageHandler SetupCustomHttpHandler(HttpClientConfig config, DelegatingHandler customHandler = null)
{
if (customHandler != null)
{
return customHandler;
}

HttpMessageHandler httpHandler;
try
{
Expand Down Expand Up @@ -352,6 +357,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 @@ -392,6 +398,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 @@ -404,8 +411,18 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
}
else
{
//TODO: Should probably check to see if the error is recoverable or transient.
logger.Warn("Error occurred during request, retrying...", e);
Exception innermostException = GetInnerMostException(e);

if (innermostException is AuthenticationException)
{
logger.Error("Non-retryable error encountered: ", e);
throw;
}
else
{
//TODO: Should probably check to see if the error is recoverable or transient.
logger.Warn("Error occurred during request, retrying...", e);
}
}
}

Expand Down Expand Up @@ -455,7 +472,10 @@ 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}";
logger.Error(errorMessage);
throw new OperationCanceledException(errorMessage);
}

// Disposing of the response if not null now that we don't need it anymore
Expand Down Expand Up @@ -495,6 +515,14 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
}
}

static private Exception GetInnerMostException(Exception exception)
{
var innermostException = exception;
while (innermostException.InnerException != null && innermostException != innermostException.InnerException)
innermostException = innermostException.InnerException;
return innermostException;
}

/// <summary>
/// Check whether or not the error is retryable or not.
/// </summary>
Expand Down

0 comments on commit aef4d3a

Please sign in to comment.