From aef4d3af3f445b3fab411dc77bf8ef6486e2a6d0 Mon Sep 17 00:00:00 2001 From: sfc-gh-ext-simba-lf <115584722+sfc-gh-ext-simba-lf@users.noreply.github.com> Date: Wed, 18 Sep 2024 13:53:09 -0700 Subject: [PATCH] SNOW-1502317: Stop retry on nonrecoverable exception (#1013) --- .../UnitTests/HttpUtilTest.cs | 42 ++++++++++++++++++ Snowflake.Data/Core/HttpUtil.cs | 44 +++++++++++++++---- 2 files changed, 78 insertions(+), 8 deletions(-) diff --git a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs index 668db5f91..879fb26d2 100644 --- a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs +++ b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs @@ -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(); + handler.Protected() + .Setup>( + "SendAsync", + ItExpr.Is(req => req.RequestUri.ToString().Contains("https://authenticationexceptiontest.com/")), + ItExpr.IsAny()) + .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(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)] diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 558ce252f..72d18bcdd 100755 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -100,16 +100,16 @@ private HttpUtil() private Dictionary _HttpClients = new Dictionary(); - 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)) @@ -117,7 +117,7 @@ private HttpClient RegisterNewHttpClientIfNecessary(HttpClientConfig config) 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 }; @@ -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 { @@ -352,6 +357,7 @@ protected override async Task 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 @@ -392,6 +398,7 @@ protected override async Task SendAsync(HttpRequestMessage } catch (Exception e) { + lastException = e; if (cancellationToken.IsCancellationRequested) { logger.Debug("SF rest request timeout or explicit cancel called."); @@ -404,8 +411,18 @@ protected override async Task 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); + } } } @@ -455,7 +472,10 @@ protected override async Task 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 @@ -495,6 +515,14 @@ protected override async Task SendAsync(HttpRequestMessage } } + static private Exception GetInnerMostException(Exception exception) + { + var innermostException = exception; + while (innermostException.InnerException != null && innermostException != innermostException.InnerException) + innermostException = innermostException.InnerException; + return innermostException; + } + /// /// Check whether or not the error is retryable or not. ///