diff --git a/CHANGELOG.md b/CHANGELOG.md index 75a1d1d24..b7da06c6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Added `CRLDOWNLOADMAXSIZE` connection parameter to limit the maximum size of CRL files downloaded during certificate revocation checks. - AWS WIF will now also check the application config and AWS profile credential store when determining the current AWS region - Allow users to configure the maximum amount of connections via `SERVICE_POINT_CONNECTION_LIMIT` property. + - Add retry for HTTP 307/308 status codes - v5.0.0 - Disabled CRL checks by default. - Added support for alternative, memory efficient and thread safe CRL (Certificate Revocation List) checks. diff --git a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs index 92ec7d8df..090f40a1c 100644 --- a/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs +++ b/Snowflake.Data.Tests/UnitTests/HttpUtilTest.cs @@ -64,6 +64,8 @@ public async Task TestNonRetryableHttpExceptionThrowsError() [TestCase((HttpStatusCode)429, false, true)] // HttpStatusCode.TooManyRequests is not available on .NET Framework [TestCase(HttpStatusCode.InternalServerError, false, true)] [TestCase(HttpStatusCode.ServiceUnavailable, false, true)] + [TestCase(HttpStatusCode.TemporaryRedirect, false, true)] + [TestCase((HttpStatusCode)308, false, true)] // HttpStatusCode.PermanentRedirect is not available on .NET Framework public async Task TestIsRetryableHTTPCode(HttpStatusCode statusCode, bool forceRetryOn404, bool expectedIsRetryable) { var mockHttp = new MockHttpMessageHandler(); diff --git a/Snowflake.Data.Tests/UnitTests/HttpUtilWiremockRunnerTest.cs b/Snowflake.Data.Tests/UnitTests/HttpUtilWiremockRunnerTest.cs new file mode 100644 index 000000000..ac1d13a98 --- /dev/null +++ b/Snowflake.Data.Tests/UnitTests/HttpUtilWiremockRunnerTest.cs @@ -0,0 +1,112 @@ +using System.Net.Http; +using System.Threading.Tasks; +using Newtonsoft.Json; +using NUnit.Framework; +using Snowflake.Data.Core; +using Snowflake.Data.Tests.Util; +using System; +using System.Threading; + +namespace Snowflake.Data.Tests.UnitTests +{ + [TestFixture, NonParallelizable] + public class HttpUtilWiremockRunnerTest + { + private WiremockRunner _runner; + + [OneTimeSetUp] + public void BeforeAll() + { + _runner = WiremockRunner.NewWiremock(); + } + + [SetUp] + public void BeforeEach() + { + _runner.ResetMapping(); + } + + [OneTimeTearDown] + public void AfterAll() + { + _runner.Stop(); + } + + [Test] + public async Task TestHttp307Retry() + { + // arrange + _runner.AddMappings("wiremock/HttpUtil/http_307_retry.json"); + var expectedQueryId = "http_307_retry_queryId"; + var httpClient = CreateHttpClientForRetry(); + var request = CreateQueryRequest(); + + //act + var response = await httpClient.SendAsync(request); + + // assert + await AssertResponseId(response, expectedQueryId); + } + + [Test] + public async Task TestHttp308Retry() + { + // arrange + _runner.AddMappings("wiremock/HttpUtil/http_308_retry.json"); + var expectedQueryId = "http_308_retry_queryId"; + var httpClient = CreateHttpClientForRetry(); + var request = CreateQueryRequest(); + + //act + var response = await httpClient.SendAsync(request); + + // assert + await AssertResponseId(response, expectedQueryId); + } + + private HttpClient CreateHttpClientForRetry() + { + var config = new HttpClientConfig(null, null, null, null, null, false, false, 7, 20); + var handler = new CustomDelegatingHandler(new HttpClientHandler + { + ClientCertificateOptions = ClientCertificateOption.Manual, + ServerCertificateCustomValidationCallback = (_, _, _, _) => true, + AllowAutoRedirect = true, + }); + return HttpUtil.Instance.CreateNewHttpClient(config, handler); + } + + private HttpRequestMessage CreateQueryRequest() + { + var request = new HttpRequestMessage(HttpMethod.Post, _runner.WiremockBaseHttpsUrl + "/queries/v1/query-request?requestId=abc"); +#pragma warning disable CS0618 // Type or member is obsolete + request.Properties.Add(BaseRestRequest.HTTP_REQUEST_TIMEOUT_KEY, TimeSpan.FromSeconds(BaseRestRequest.s_defaultHttpSecondsTimeout)); + request.Properties.Add(BaseRestRequest.REST_REQUEST_TIMEOUT_KEY, TimeSpan.FromSeconds(BaseRestRequest.s_defaultRestRetrySecondsTimeout)); +#pragma warning restore CS0618 // Type or member is obsolete + return request; + } + + private async Task AssertResponseId(HttpResponseMessage response, string expectedQueryId) + { + var json = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject(json, JsonUtils.JsonSettings); + + // assert + Assert.True(response.IsSuccessStatusCode); + Assert.AreEqual(expectedQueryId, result.data.queryId); + } + } + + public class CustomDelegatingHandler : DelegatingHandler + { + public CustomDelegatingHandler(HttpMessageHandler httpMessageHandler) : base(httpMessageHandler) { } + + protected override async Task SendAsync( + HttpRequestMessage request, + CancellationToken cancellationToken) + { + var response = await base.SendAsync(request, cancellationToken); + return response; + } + } +} diff --git a/Snowflake.Data.Tests/wiremock/HttpUtil/http_307_retry.json b/Snowflake.Data.Tests/wiremock/HttpUtil/http_307_retry.json new file mode 100644 index 000000000..9e790b29e --- /dev/null +++ b/Snowflake.Data.Tests/wiremock/HttpUtil/http_307_retry.json @@ -0,0 +1,52 @@ +{ + "mappings": [ + { + "scenarioName": "HTTP 307 retry", + "requiredScenarioState": "Started", + "newScenarioState": "Query attempt with HTTP 307 response", + "request": { + "urlPathPattern": "/queries/v1/query-request.*", + "method": "POST" + }, + "response": { + "status": 307, + "headers": { + "Location": "/temp-redirect-1" + } + } + }, + { + "scenarioName": "HTTP 307 retry", + "requiredScenarioState": "Query attempt with HTTP 307 response", + "newScenarioState": "307 redirect followed and times out", + "request": { + "urlPathPattern": "/temp-redirect-1", + "method": "POST" + }, + "response": { + "fixedDelayMilliseconds": 20000 + } + }, + { + "scenarioName": "HTTP 307 retry", + "requiredScenarioState": "307 redirect followed and times out", + "newScenarioState": "Retry attempt successful", + "request": { + "urlPathPattern": "/queries/v1/query-request.*", + "method": "POST" + }, + "response": { + "status": 200, + "jsonBody": { + "data": { + "queryId": "http_307_retry_queryId" + }, + "code": null, + "message": null, + "success": true + } + } + } + ] +} + diff --git a/Snowflake.Data.Tests/wiremock/HttpUtil/http_308_retry.json b/Snowflake.Data.Tests/wiremock/HttpUtil/http_308_retry.json new file mode 100644 index 000000000..27c857dc2 --- /dev/null +++ b/Snowflake.Data.Tests/wiremock/HttpUtil/http_308_retry.json @@ -0,0 +1,51 @@ +{ + "mappings": [ + { + "scenarioName": "HTTP 308 retry", + "requiredScenarioState": "Started", + "newScenarioState": "Query attempt with HTTP 308 response", + "request": { + "urlPathPattern": "/queries/v1/query-request.*", + "method": "POST" + }, + "response": { + "status": 308, + "headers": { + "Location": "/temp-redirect-1" + } + } + }, + { + "scenarioName": "HTTP 308 retry", + "requiredScenarioState": "Query attempt with HTTP 308 response", + "newScenarioState": "308 redirect followed and times out", + "request": { + "urlPathPattern": "/temp-redirect-1", + "method": "POST" + }, + "response": { + "fixedDelayMilliseconds": 20000 + } + }, + { + "scenarioName": "HTTP 308 retry", + "requiredScenarioState": "308 redirect followed and times out", + "newScenarioState": "Retry attempt successful", + "request": { + "urlPathPattern": "/queries/v1/query-request.*", + "method": "POST" + }, + "response": { + "status": 200, + "jsonBody": { + "data": { + "queryId": "http_308_retry_queryId" + }, + "code": null, + "message": null, + "success": true + } + } + } + ] +} diff --git a/Snowflake.Data/Core/HttpUtil.cs b/Snowflake.Data/Core/HttpUtil.cs index 405d8b4ae..d0fc23495 100644 --- a/Snowflake.Data/Core/HttpUtil.cs +++ b/Snowflake.Data/Core/HttpUtil.cs @@ -292,7 +292,8 @@ private HttpClientHandler CreateHttpClientHandler(HttpClientConfig config) { AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate, UseCookies = false, // Disable cookies - UseProxy = false + UseProxy = false, + AllowAutoRedirect = true }; } } @@ -306,7 +307,8 @@ private HttpClientHandler CreateHttpClientHandlerWithDotnetCrlCheck(HttpClientCo SslProtocols = config.GetRequestedTlsProtocolsRange(), AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate, UseCookies = false, // Disable cookies - UseProxy = false + UseProxy = false, + AllowAutoRedirect = true }; } @@ -327,7 +329,8 @@ private HttpClientHandler CreateHttpClientHandlerWithCustomizedCrlCheck(HttpClie SslProtocols = config.GetRequestedTlsProtocolsRange(), AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate, UseCookies = false, // Disable cookies - UseProxy = false + UseProxy = false, + AllowAutoRedirect = true }; } @@ -427,8 +430,11 @@ internal UriUpdater(Uri uri, bool includeRetryReason = true) } } - internal Uri Update(int retryReason = 0) + internal Uri Update(int retryReason = 0, Uri requestUri = null, Uri location = null) { + if (IsRedirectHTTPCode(retryReason)) + return GetRedirectedUri(requestUri, location); + // Optimization to bypass parsing if there is no rules at all. if (rules.Count == 0) { @@ -450,6 +456,13 @@ internal Uri Update(int retryReason = 0) return uriBuilder.Uri; } + + private Uri GetRedirectedUri(Uri requestUri, Uri location) + { + if (requestUri.AbsolutePath != location.ToString()) + return new Uri(uriBuilder.Uri, location); + return uriBuilder.Uri; + } } private class RetryHandler : DelegatingHandler { @@ -620,10 +633,11 @@ protected override async Task SendAsync(HttpRequestMessage throw new OperationCanceledException(errorMessage); } + requestMessage.RequestUri = updater.Update(errorReason, requestMessage.RequestUri, response?.Headers?.Location); + // Disposing of the response if not null now that we don't need it anymore response?.Dispose(); - requestMessage.RequestUri = updater.Update(errorReason); logger.Debug($"Sleep {backOffInSec} seconds and then retry the request, retryCount: {retryCount}"); @@ -674,7 +688,17 @@ static public bool isRetryableHTTPCode(int statusCode, bool forceRetryOn404) // Request timeout (statusCode == 408) || // Too many requests - (statusCode == 429); + (statusCode == 429) || + IsRedirectHTTPCode(statusCode); + } + + static public bool IsRedirectHTTPCode(int statusCode) + { + return + // Temporary redirect + (statusCode == 307) || + // Permanent redirect + (statusCode == 308); } /// diff --git a/Snowflake.Data/Core/RestRequest.cs b/Snowflake.Data/Core/RestRequest.cs index d5325c96d..43d075f02 100644 --- a/Snowflake.Data/Core/RestRequest.cs +++ b/Snowflake.Data/Core/RestRequest.cs @@ -24,7 +24,10 @@ internal abstract class BaseRestRequest : IRestRequest internal static string REST_REQUEST_TIMEOUT_KEY = "TIMEOUT_PER_REST_REQUEST"; // The default Rest timeout. Set to 120 seconds. - public static int DEFAULT_REST_RETRY_SECONDS_TIMEOUT = 120; + public static readonly int s_defaultRestRetrySecondsTimeout = 120; + + // Default each http request timeout to 16 seconds + public static readonly int s_defaultHttpSecondsTimeout = 16; internal Uri Url { get; set; } @@ -110,10 +113,8 @@ internal class SFRestRequest : BaseRestRequest, IRestRequest internal SFRestRequest() : base() { - RestTimeout = TimeSpan.FromSeconds(DEFAULT_REST_RETRY_SECONDS_TIMEOUT); - - // default each http request timeout to 16 seconds - HttpTimeout = TimeSpan.FromSeconds(16); + RestTimeout = TimeSpan.FromSeconds(s_defaultRestRetrySecondsTimeout); + HttpTimeout = TimeSpan.FromSeconds(s_defaultHttpSecondsTimeout); } internal Object jsonBody { get; set; }