Retry once on certain WebException codes (#785)

Address NuGet/Engineering#3212
Only added retry for WebException codes that were clearly transient in logs

src/NuGet.Services.AzureSearch/DependencyInjectionExtensions.cs

@@ -10,6 +10,7 @@
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Microsoft.Rest;
+using Microsoft.Rest.TransientFaultHandling;
 using Microsoft.WindowsAzure.Storage;
 using NuGet.Protocol;
 using NuGet.Protocol.Catalog;
@@ -231,13 +232,26 @@ public static IServiceCollection AddAzureSearch(
             services.AddFeatureFlags();
             services.AddTransient<IFeatureFlagService, FeatureFlagService>();
 
+            services.AddTransient<ISearchServiceClientWrapper>(p => new SearchServiceClientWrapper(
+                p.GetRequiredService<ISearchServiceClient>(),
+                GetSearchDelegatingHandlers(p.GetRequiredService<ILoggerFactory>()),
+                GetSearchRetryPolicy(),
+                p.GetRequiredService<ILogger<DocumentsOperationsWrapper>>()));
+
             services
                 .AddTransient<ISearchServiceClient>(p =>
                 {
                     var options = p.GetRequiredService<IOptionsSnapshot<AzureSearchConfiguration>>();
-                    return new SearchServiceClient(
+
+                    var client = new SearchServiceClient(
                         options.Value.SearchServiceName,
-                        new SearchCredentials(options.Value.SearchServiceApiKey));
+                        new SearchCredentials(options.Value.SearchServiceApiKey),
+                        new WebRequestHandler(),
+                        GetSearchDelegatingHandlers(p.GetRequiredService<ILoggerFactory>()));
+
+                    client.SetRetryPolicy(GetSearchRetryPolicy());
+
+                    return client;
                 });
 
             services.AddSingleton<IAuxiliaryDataCache, AuxiliaryDataCache>();
@@ -276,12 +290,36 @@ public static IServiceCollection AddAzureSearch(
             services.AddTransient<ISearchIndexActionBuilder, SearchIndexActionBuilder>();
             services.AddTransient<ISearchParametersBuilder, SearchParametersBuilder>();
             services.AddTransient<ISearchResponseBuilder, SearchResponseBuilder>();
-            services.AddTransient<ISearchServiceClientWrapper, SearchServiceClientWrapper>();
             services.AddTransient<ISearchTextBuilder, SearchTextBuilder>();
             services.AddTransient<IServiceClientTracingInterceptor, ServiceClientTracingLogger>();
             services.AddTransient<ISystemTime, SystemTime>();
 
             return services;
         }
+
+        /// <summary>
+        /// Defaults originally taken from:
+        /// https://github.com/Azure/azure-sdk-for-net/blob/96421089bc26198098f320ea50e0208e98376956/sdk/mgmtcommon/ClientRuntime/ClientRuntime/RetryDelegatingHandler.cs#L19-L22
+        /// 
+        /// Note that this policy only applied to the <see cref="RetryDelegatingHandler"/> automatically initialized by
+        /// the Azure Search SDK. This policy does not apply to <see cref="WebExceptionRetryDelegatingHandler"/>.
+        /// </summary>
+        private static RetryPolicy GetSearchRetryPolicy()
+        {
+            return new RetryPolicy(
+                new HttpStatusCodeErrorDetectionStrategy(),
+                retryCount: 3,
+                minBackoff: TimeSpan.FromSeconds(1),
+                maxBackoff: TimeSpan.FromSeconds(10),
+                deltaBackoff: TimeSpan.FromSeconds(10));
+        }
+
+        public static DelegatingHandler[] GetSearchDelegatingHandlers(ILoggerFactory loggerFactory)
+        {
+            return new[]
+            {
+                new WebExceptionRetryDelegatingHandler(loggerFactory.CreateLogger<WebExceptionRetryDelegatingHandler>()),
+            };
+        }
     }
 }

src/NuGet.Services.AzureSearch/NuGet.Services.AzureSearch.csproj

@@ -228,6 +228,7 @@
     <Compile Include="Properties\AssemblyInfo.cs" />
     <Compile Include="Properties\AssemblyInfo.*.cs" />
     <Compile Include="AzureSearchException.cs" />
+    <Compile Include="WebExceptionRetryDelegatingHandler.cs" />
     <Compile Include="Wrappers\DocumentsOperationsWrapper.cs" />
     <Compile Include="Wrappers\IDocumentsOperationsWrapper.cs" />
     <Compile Include="Wrappers\IIndexesOperationsWrapper.cs" />

src/NuGet.Services.AzureSearch/WebExceptionRetryDelegatingHandler.cs

@@ -0,0 +1,54 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using System.Collections.Generic;
+using System.Net;
+using System.Net.Http;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.Extensions.Logging;
+
+namespace NuGet.Services.AzureSearch
+{
+    public class WebExceptionRetryDelegatingHandler : DelegatingHandler
+    {
+        private static readonly HashSet<WebExceptionStatus> _transientWebExceptionStatuses = new HashSet<WebExceptionStatus>(new[]
+        {
+            WebExceptionStatus.ConnectFailure,   // Unable to connect to the remote server
+            WebExceptionStatus.ConnectionClosed, // The underlying connection was closed
+            WebExceptionStatus.KeepAliveFailure, // A connection that was expected to be kept alive was closed by the server
+            WebExceptionStatus.ReceiveFailure,   // An unexpected error occurred on a receive
+        });
+
+        private readonly ILogger<WebExceptionRetryDelegatingHandler> _logger;
+
+        public WebExceptionRetryDelegatingHandler(ILogger<WebExceptionRetryDelegatingHandler> logger)
+        {
+            _logger = logger ?? throw new ArgumentNullException(nameof(logger));
+        }
+
+        protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
+        {
+            try
+            {
+                return await base.SendAsync(request, cancellationToken);
+            }
+            catch (Exception ex) when (ex is HttpRequestException hre && hre.InnerException is WebException we)
+            {
+                if (_transientWebExceptionStatuses.Contains(we.Status))
+                {
+                    // Retry only a single time since some of these transient exceptions take a while (~20 seconds) to be
+                    // thrown and we don't want to make the user wait too long even to see a failure.
+                    _logger.LogWarning(ex, "Transient web exception encountered, status {Status}. Attempting a single retry.", we.Status);
+                    return await base.SendAsync(request, cancellationToken);
+                }
+                else
+                {
+                    _logger.LogError(ex, "Non-transient web exception encountered, status {Status}.", we.Status);
+                    throw;
+                }
+            }
+        }
+    }
+}

src/NuGet.Services.AzureSearch/Wrappers/IndexesOperationsWrapper.cs

@@ -2,29 +2,58 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Linq;
+using System.Net.Http;
 using System.Threading.Tasks;
 using Microsoft.Azure.Search;
 using Microsoft.Azure.Search.Models;
 using Microsoft.Extensions.Logging;
+using Microsoft.Rest.TransientFaultHandling;
 
 namespace NuGet.Services.AzureSearch.Wrappers
 {
     public class IndexesOperationsWrapper : IIndexesOperationsWrapper
     {
         private readonly IIndexesOperations _inner;
+        private readonly DelegatingHandler[] _handlers;
+        private readonly RetryPolicy _retryPolicy;
         private readonly ILogger<DocumentsOperationsWrapper> _documentsOperationsLogger;
 
         public IndexesOperationsWrapper(
             IIndexesOperations inner,
+            DelegatingHandler[] handlers,
+            RetryPolicy retryPolicy,
             ILogger<DocumentsOperationsWrapper> documentsOperationsLogger)
         {
             _inner = inner ?? throw new ArgumentNullException(nameof(inner));
+            _handlers = handlers ?? throw new ArgumentNullException(nameof(handlers));
+            _retryPolicy = retryPolicy;
             _documentsOperationsLogger = documentsOperationsLogger ?? throw new ArgumentNullException(nameof(documentsOperationsLogger));
         }
 
+        /// <summary>
+        /// This is implemented in lieu of <see cref="IndexesGetClientExtensions.GetClient(IIndexesOperations, string)"/>
+        /// because it allows the delegating handlers and retry policy to be specified. See:
+        /// https://github.com/Azure/azure-sdk-for-net/blob/96421089bc26198098f320ea50e0208e98376956/sdk/search/Microsoft.Azure.Search/src/IndexesGetClientExtensions.cs#L27-L41
+        /// </summary>
         public ISearchIndexClientWrapper GetClient(string indexName)
         {
-            return new SearchIndexClientWrapper(_inner.GetClient(indexName), _documentsOperationsLogger);
+            var searchIndexClient = new SearchIndexClient(
+                _inner.Client.SearchServiceName,
+                indexName,
+                _inner.Client.SearchCredentials,
+                _inner.Client.HttpMessageHandlers.OfType<HttpClientHandler>().SingleOrDefault(),
+                _handlers);
+
+            searchIndexClient.SearchDnsSuffix = _inner.Client.SearchDnsSuffix;
+            searchIndexClient.HttpClient.Timeout = _inner.Client.HttpClient.Timeout;
+
+            if (_retryPolicy != null)
+            {
+                searchIndexClient.SetRetryPolicy(_retryPolicy);
+            }
+
+            return new SearchIndexClientWrapper(searchIndexClient, _documentsOperationsLogger);
         }
 
         public async Task<bool> ExistsAsync(string indexName)

src/NuGet.Services.AzureSearch/Wrappers/SearchServiceClientWrapper.cs

@@ -2,8 +2,10 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Net.Http;
 using Microsoft.Azure.Search;
 using Microsoft.Extensions.Logging;
+using Microsoft.Rest.TransientFaultHandling;
 
 namespace NuGet.Services.AzureSearch.Wrappers
 {
@@ -13,10 +15,12 @@ public class SearchServiceClientWrapper : ISearchServiceClientWrapper
 
         public SearchServiceClientWrapper(
             ISearchServiceClient inner,
+            DelegatingHandler[] handlers,
+            RetryPolicy retryPolicy,
             ILogger<DocumentsOperationsWrapper> documentsOperationsLogger)
         {
             _inner = inner ?? throw new ArgumentNullException(nameof(inner));
-            Indexes = new IndexesOperationsWrapper(_inner.Indexes, documentsOperationsLogger);
+            Indexes = new IndexesOperationsWrapper(_inner.Indexes, handlers, retryPolicy, documentsOperationsLogger);
         }
 
         public IIndexesOperationsWrapper Indexes { get; }

tests/NuGet.Services.AzureSearch.Tests/DependencyInjectionExtensionsFacts.cs

@@ -0,0 +1,161 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Net;
+using System.Net.Http;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.Azure.Search;
+using Microsoft.Extensions.Logging;
+using Microsoft.Rest.TransientFaultHandling;
+using Moq;
+using Moq.Language.Flow;
+using NuGet.Services.AzureSearch.Wrappers;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace NuGet.Services.AzureSearch
+{
+    public class DependencyInjectionExtensionsFacts
+    {
+        public class TheGetRetryPolicyMethod
+        {
+            public TheGetRetryPolicyMethod(ITestOutputHelper output)
+            {
+                LoggerFactory = new LoggerFactory().AddXunit(output);
+                HttpClientHandler = new Mock<TestHttpClientHandler> { CallBase = true };
+
+                SearchServiceClient = new SearchServiceClient(
+                    "test-search-service",
+                    new SearchCredentials("api-key"),
+                    HttpClientHandler.Object,
+                    DependencyInjectionExtensions.GetSearchDelegatingHandlers(LoggerFactory));
+                SearchServiceClient.SetRetryPolicy(SingleRetry);
+
+                IndexesOperationsWrapper = new IndexesOperationsWrapper(
+                    SearchServiceClient.Indexes,
+                    DependencyInjectionExtensions.GetSearchDelegatingHandlers(LoggerFactory),
+                    SingleRetry,
+                    LoggerFactory.CreateLogger<DocumentsOperationsWrapper>());
+            }
+
+            [Theory]
+            [MemberData(nameof(NonTransientTestData))]
+            public async Task DoesNotRetryNonTransientErrorsForIndexOperations(Action<ISetup<TestHttpClientHandler, Task<HttpResponseMessage>>> setup)
+            {
+                setup(HttpClientHandler.Setup(x => x.OnSendAsync(It.IsAny<HttpRequestMessage>(), It.IsAny<CancellationToken>())));
+
+                await Assert.ThrowsAnyAsync<Exception>(() => SearchServiceClient.Indexes.ListAsync());
+
+                VerifyAttemptCount(1);
+            }
+
+            [Theory]
+            [MemberData(nameof(NonTransientTestData))]
+            public async Task DoesNotRetryNonTransientErrorsForDocumentOperations(Action<ISetup<TestHttpClientHandler, Task<HttpResponseMessage>>> setup)
+            {
+                setup(HttpClientHandler.Setup(x => x.OnSendAsync(It.IsAny<HttpRequestMessage>(), It.IsAny<CancellationToken>())));
+
+                await Assert.ThrowsAnyAsync<Exception>(() => IndexesOperationsWrapper.GetClient("test-index").Documents.CountAsync());
+
+                VerifyAttemptCount(1);
+            }
+
+            [Theory]
+            [MemberData(nameof(TransientTestData))]
+            public async Task RetriesTransientErrorsForIndexOperations(Action<ISetup<TestHttpClientHandler, Task<HttpResponseMessage>>> setup)
+            {
+                setup(HttpClientHandler.Setup(x => x.OnSendAsync(It.IsAny<HttpRequestMessage>(), It.IsAny<CancellationToken>())));
+
+                await Assert.ThrowsAnyAsync<Exception>(() => SearchServiceClient.Indexes.ListAsync());
+
+                VerifyAttemptCount(2);
+            }
+
+            [Theory]
+            [MemberData(nameof(TransientTestData))]
+            public async Task RetriesTransientErrorsForDocumentOperations(Action<ISetup<TestHttpClientHandler, Task<HttpResponseMessage>>> setup)
+            {
+                setup(HttpClientHandler.Setup(x => x.OnSendAsync(It.IsAny<HttpRequestMessage>(), It.IsAny<CancellationToken>())));
+
+                await Assert.ThrowsAnyAsync<Exception>(() => IndexesOperationsWrapper.GetClient("test-index").Documents.CountAsync());
+
+                VerifyAttemptCount(2);
+            }
+
+            private void VerifyAttemptCount(int count)
+            {
+                HttpClientHandler.Verify(
+                    x => x.OnSendAsync(It.IsAny<HttpRequestMessage>(), It.IsAny<CancellationToken>()),
+                    Times.Exactly(count));
+            }
+
+            public static IEnumerable<HttpStatusCode> TransientHttpStatusCodes => new[]
+            {
+                HttpStatusCode.RequestTimeout,
+                (HttpStatusCode)429,
+                HttpStatusCode.InternalServerError,
+                HttpStatusCode.BadGateway,
+                HttpStatusCode.InternalServerError,
+                HttpStatusCode.GatewayTimeout,
+            };
+
+            public static IEnumerable<WebExceptionStatus> TransientWebExceptionStatuses => new[]
+            {
+                WebExceptionStatus.ConnectFailure,
+                WebExceptionStatus.ConnectionClosed,
+                WebExceptionStatus.KeepAliveFailure,
+                WebExceptionStatus.ReceiveFailure,
+            };
+
+            public static IEnumerable<object[]> TransientTestData => GetTestData(TransientHttpStatusCodes, TransientWebExceptionStatuses);
+
+            public static IEnumerable<HttpStatusCode> NonTransientHttpStatusCodes => new[]
+            {
+                HttpStatusCode.BadRequest,
+                HttpStatusCode.Unauthorized,
+                HttpStatusCode.Forbidden,
+                HttpStatusCode.NotFound,
+                HttpStatusCode.Conflict,
+                HttpStatusCode.NotImplemented,
+            };
+
+            public static IEnumerable<WebExceptionStatus> NonTransientWebExceptionStatuses => new[]
+            {
+                WebExceptionStatus.TrustFailure,
+                WebExceptionStatus.NameResolutionFailure,
+            };
+
+            public static IEnumerable<object[]> NonTransientTestData => GetTestData(NonTransientHttpStatusCodes, NonTransientWebExceptionStatuses);
+
+            private static IEnumerable<object[]> GetTestData(IEnumerable<HttpStatusCode> statusCodes, IEnumerable<WebExceptionStatus> webExceptionStatuses)
+            {
+                var setups = new List<Action<ISetup<TestHttpClientHandler, Task<HttpResponseMessage>>>>();
+
+                foreach (var statusCode in statusCodes)
+                {
+                    setups.Add(s => s.ReturnsAsync(() => new HttpResponseMessage(statusCode) { Content = new StringContent(string.Empty) }));
+                }
+
+                foreach (var webExceptionStatus in webExceptionStatuses)
+                {
+                    setups.Add(s => s.ThrowsAsync(new HttpRequestException("Fail.", new WebException("Inner fail.", webExceptionStatus))));
+                }
+
+                return setups.Select(x => new object[] { x });
+            }
+
+            public RetryPolicy SingleRetry => new RetryPolicy(
+                new HttpStatusCodeErrorDetectionStrategy(),
+                new FixedIntervalRetryStrategy(retryCount: 1, retryInterval: TimeSpan.Zero));
+
+            public ILoggerFactory LoggerFactory { get; }
+            public Mock<TestHttpClientHandler> HttpClientHandler { get; }
+            public SearchServiceClient SearchServiceClient { get; }
+            public IndexesOperationsWrapper IndexesOperationsWrapper { get; }
+        }
+    }
+}

tests/NuGet.Services.AzureSearch.Tests/NuGet.Services.AzureSearch.Tests.csproj

@@ -68,6 +68,7 @@
     <Compile Include="IndexBuilderFacts.cs" />
     <Compile Include="Models\CommittedDocumentFacts.cs" />
     <Compile Include="AuxiliaryFiles\OwnerDataClientFacts.cs" />
+    <Compile Include="DependencyInjectionExtensionsFacts.cs" />
     <Compile Include="SearchIndexActionBuilderFacts.cs" />
     <Compile Include="Auxiliary2AzureSearch\UpdateOwnersCommandFacts.cs" />
     <Compile Include="Auxiliary2AzureSearch\DataSetComparerFacts.cs" />