Merge pull request #223 from DCourtel/main

Add more information on failures
microsoft · Mar 25, 2024 · ace6591 · ace6591
2 parents 9d809e5 + 376eb06
commit ace6591
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 47 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [1.3.8] - 2024-03-25]
+
+## Changed
+
+- When too many retries are attempted, the RetryHandler will now throw an `AggregateException` (instead of an `InvalidOperationException`).
+  The `InnerExceptions` property of the `AggregateException` will contain a list of `ApiException` with the HTTP status code and an error message if available.
+
 ## [1.3.7] - 2024-02-26
 
 ### Changed

diff --git a/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/RetryHandlerTests.cs b/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/RetryHandlerTests.cs
@@ -6,6 +6,7 @@
 using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.Kiota.Abstractions;
 using Microsoft.Kiota.Http.HttpClientLibrary.Middleware;
 using Microsoft.Kiota.Http.HttpClientLibrary.Middleware.Options;
 using Microsoft.Kiota.Http.HttpClientLibrary.Tests.Mocks;
@@ -51,7 +52,6 @@ public void RetryHandlerConstructor()
             Assert.IsType<RetryHandler>(retry);
         }
 
-
         [Fact]
         public void RetryHandlerHttpMessageHandlerConstructor()
         {
@@ -89,7 +89,6 @@ public async Task OkStatusShouldPassThrough()
             Assert.NotNull(response.RequestMessage);
             Assert.Same(response.RequestMessage, httpRequestMessage);
             Assert.False(response.RequestMessage.Headers.Contains(RetryAttempt), "The request add header wrong.");
-
         }
 
         [Theory]
@@ -116,7 +115,6 @@ public async Task ShouldRetryWithAddRetryAttemptHeader(HttpStatusCode statusCode
             Assert.Equal(values.First(), 1.ToString());
         }
 
-
         [Theory]
         [InlineData(HttpStatusCode.GatewayTimeout)]  // 504
         [InlineData(HttpStatusCode.ServiceUnavailable)]  // 503
@@ -140,7 +138,6 @@ public async Task ShouldRetryWithBuffedContent(HttpStatusCode statusCode)
             Assert.NotNull(response.RequestMessage.Content);
             Assert.NotNull(response.RequestMessage.Content.Headers.ContentLength);
             Assert.Equal("Hello World", await response.RequestMessage.Content.ReadAsStringAsync());
-
         }
 
         [Theory]
@@ -196,8 +193,7 @@ public async Task ShouldNotRetryWithPutStreaming(HttpStatusCode statusCode)
             Assert.Equal(response.RequestMessage.Content.Headers.ContentLength, -1);
         }
 
-
-        [Theory(Skip = "Test takes a while to run")]
+        [Theory]
         [InlineData(HttpStatusCode.GatewayTimeout)]  // 504
         [InlineData(HttpStatusCode.ServiceUnavailable)]  // 503
         [InlineData((HttpStatusCode)429)] // 429
@@ -208,16 +204,25 @@ public async Task ExceedMaxRetryShouldReturn(HttpStatusCode statusCode)
             var retryResponse = new HttpResponseMessage(statusCode);
             var response2 = new HttpResponseMessage(statusCode);
             this._testHttpMessageHandler.SetHttpResponse(retryResponse, response2);
+            var retryHandler = new RetryHandler
+            {
+                InnerHandler = _testHttpMessageHandler,
+                RetryOption = new RetryHandlerOption { Delay = 1 }
+            };
+            var invoker = new HttpMessageInvoker(retryHandler);
             // Act
             try
             {
-                await _invoker.SendAsync(httpRequestMessage, new CancellationToken());
+                await invoker.SendAsync(httpRequestMessage, new CancellationToken());
             }
             catch(Exception exception)
             {
                 // Assert
-                Assert.IsType<InvalidOperationException>(exception);
-                Assert.Equal("Too many retries performed", exception.Message);
+                Assert.IsType<AggregateException>(exception);
+                var aggregateException = exception as AggregateException;
+                Assert.StartsWith("Too many retries performed.", aggregateException.Message);
+                Assert.All(aggregateException.InnerExceptions, innerexception => Assert.IsType<ApiException>(innerexception));
+                Assert.All(aggregateException.InnerExceptions, innerexception => Assert.True((innerexception as ApiException).ResponseStatusCode == (int)statusCode));
                 Assert.False(httpRequestMessage.Headers.TryGetValues(RetryAttempt, out _), "Don't set Retry-Attempt Header");
             }
         }
@@ -246,17 +251,16 @@ public async Task ShouldDelayBasedOnRetryAfterHeaderWithHttpDate(HttpStatusCode
             // Arrange
             var retryResponse = new HttpResponseMessage(statusCode);
             var futureTime = DateTime.Now + TimeSpan.FromSeconds(3);// 3 seconds from now
-            var futureTimeString = futureTime.ToString(CultureInfo.InvariantCulture.DateTimeFormat.RFC1123Pattern);
+            var futureTimeString = futureTime.ToString(CultureInfo.InvariantCulture.DateTimeFormat.RFC1123Pattern, CultureInfo.InvariantCulture);
             Assert.Contains("GMT", futureTimeString); // http date always end in GMT according to the spec
-            retryResponse.Headers.TryAddWithoutValidation(RetryAfter, futureTimeString);
+            Assert.True(retryResponse.Headers.TryAddWithoutValidation(RetryAfter, futureTimeString));
             // Act
             await DelayTestWithMessage(retryResponse, 1, "Init");
             // Assert
             Assert.Equal("Init Work 1", Message);
         }
 
-
-        [Theory(Skip = "Skipped as this takes 9 minutes to run for each scenario")] // Takes 9 minutes to run for each scenario
+        [Theory]
         [InlineData(HttpStatusCode.GatewayTimeout)]  // 504
         [InlineData(HttpStatusCode.ServiceUnavailable)]  // 503
         [InlineData((HttpStatusCode)429)] // 429
@@ -269,7 +273,7 @@ public async Task ShouldDelayBasedOnExponentialBackOff(HttpStatusCode statusCode
             for(int count = 0; count < 3; count++)
             {
                 // Act
-                await DelayTestWithMessage(retryResponse, count, "Init");
+                await DelayTestWithMessage(retryResponse, count, "Init", 1);
                 // Assert
                 Assert.Equal(Message, compareMessage + count);
             }
@@ -351,7 +355,6 @@ public async Task ShouldRetryBasedOnRetryAfterHeaderWithHttpDate(HttpStatusCode
             Assert.NotSame(response.RequestMessage, httpRequestMessage);
         }
 
-
         [Theory]
         [InlineData(1, HttpStatusCode.BadGateway, true)]
         [InlineData(2, HttpStatusCode.BadGateway, true)]
@@ -411,14 +414,16 @@ public async Task ShouldRetryBasedOnCustomShouldRetryDelegate(int expectedMaxRet
             catch(Exception exception)
             {
                 // Assert
-                Assert.IsType<InvalidOperationException>(exception);
+                Assert.IsType<AggregateException>(exception);
+                var aggregateException = exception as AggregateException;
                 Assert.True(isExceptionExpected);
-                Assert.Equal("Too many retries performed", exception.Message);
+                Assert.StartsWith("Too many retries performed.", aggregateException.Message);
+                Assert.Equal(1 + expectedMaxRetry, aggregateException.InnerExceptions.Count);
+                Assert.All(aggregateException.InnerExceptions, innerexception => Assert.Contains(expectedStatusCode.ToString(), innerexception.Message));
             }
 
             // Assert
             mockHttpMessageHandler.Protected().Verify<Task<HttpResponseMessage>>("SendAsync", Times.Exactly(1 + expectedMaxRetry), ItExpr.IsAny<HttpRequestMessage>(), It.IsAny<CancellationToken>());
-
         }
 
         private async Task DelayTestWithMessage(HttpResponseMessage response, int count, string message, int delay = RetryHandlerOption.MaxDelay)

diff --git a/Microsoft.Kiota.Http.HttpClientLibrary.sln b/Microsoft.Kiota.Http.HttpClientLibrary.sln
@@ -1,18 +1,19 @@
 
 Microsoft Visual Studio Solution File, Format Version 12.00
-# Visual Studio Version 16
-VisualStudioVersion = 16.0.30114.105
+# Visual Studio Version 17
+VisualStudioVersion = 17.9.34714.143
 MinimumVisualStudioVersion = 10.0.40219.1
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Kiota.Http.HttpClientLibrary", "src\Microsoft.Kiota.Http.HttpClientLibrary.csproj", "{769E84B2-FD14-4173-B83B-6792DFB0BA14}"
 EndProject
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution Items", "{178B3904-FBB4-4E5A-A569-B5082BABC124}"
 	ProjectSection(SolutionItems) = preProject
 		.editorconfig = .editorconfig
+		CHANGELOG.md = CHANGELOG.md
 	EndProjectSection
 EndProject
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.Kiota.Http.HttpClientLibrary.Tests", "Microsoft.Kiota.Http.HttpClientLibrary.Tests\Microsoft.Kiota.Http.HttpClientLibrary.Tests.csproj", "{CCD20728-D593-48F8-8627-6E7A57B31A43}"
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Kiota.Http.HttpClientLibrary.Tests", "Microsoft.Kiota.Http.HttpClientLibrary.Tests\Microsoft.Kiota.Http.HttpClientLibrary.Tests.csproj", "{CCD20728-D593-48F8-8627-6E7A57B31A43}"
 EndProject
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "KiotaGenerated", "Kiota.Generated\KiotaGenerated.csproj", "{6667C82F-EFF1-4D7C-828D-F981629C8256}"
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "KiotaGenerated", "Kiota.Generated\KiotaGenerated.csproj", "{6667C82F-EFF1-4D7C-828D-F981629C8256}"
 EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution

diff --git a/src/Microsoft.Kiota.Http.HttpClientLibrary.csproj b/src/Microsoft.Kiota.Http.HttpClientLibrary.csproj
@@ -14,7 +14,7 @@
     <PackageProjectUrl>https://aka.ms/kiota/docs</PackageProjectUrl>
     <EmbedUntrackedSources>true</EmbedUntrackedSources>
     <Deterministic>true</Deterministic>
-    <VersionPrefix>1.3.7</VersionPrefix>
+    <VersionPrefix>1.3.8</VersionPrefix>
     <VersionSuffix></VersionSuffix>
     <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
     <!-- Enable this line once we go live to prevent breaking changes -->

diff --git a/src/Middleware/RetryHandler.cs b/src/Middleware/RetryHandler.cs
@@ -5,12 +5,14 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.Linq;
 using System.Net;
 using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.Kiota.Abstractions;
 using Microsoft.Kiota.Http.HttpClientLibrary.Extensions;
 using Microsoft.Kiota.Http.HttpClientLibrary.Middleware.Options;
 
@@ -46,6 +48,7 @@ public RetryHandler(RetryHandlerOption? retryOption = null)
         /// </summary>
         /// <param name="request">The HTTP request<see cref="HttpRequestMessage"/>needs to be sent.</param>
         /// <param name="cancellationToken">The <see cref="CancellationToken"/> for the request.</param>
+        /// <exception cref="AggregateException">Thrown when too many retries are performed.</exception>
         /// <returns></returns>
         protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
         {
@@ -69,7 +72,6 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
 
             try
             {
-
                 var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
 
                 // Check whether retries are permitted and that the MaxRetry value is a non - negative, non - zero value
@@ -93,27 +95,20 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
         /// <param name="retryOption">The <see cref="RetryHandlerOption"/> for the retry.</param>
         /// <param name="cancellationToken">The <see cref="CancellationToken"/> for the retry.</param>
         /// <param name="activitySource">The <see cref="ActivitySource"/> for the retry.</param>
+        /// <exception cref="AggregateException">Thrown when too many retries are performed.</exception>"
         /// <returns></returns>
         private async Task<HttpResponseMessage> SendRetryAsync(HttpResponseMessage response, RetryHandlerOption retryOption, CancellationToken cancellationToken, ActivitySource? activitySource)
         {
             int retryCount = 0;
             TimeSpan cumulativeDelay = TimeSpan.Zero;
+            List<Exception> exceptions = new();
 
             while(retryCount < retryOption.MaxRetry)
             {
+                exceptions.Add(await GetInnerExceptionAsync(response));
                 using var retryActivity = activitySource?.StartActivity($"{nameof(RetryHandler)}_{nameof(SendAsync)} - attempt {retryCount}");
                 retryActivity?.SetTag("http.retry_count", retryCount);
                 retryActivity?.SetTag("http.status_code", response.StatusCode);
-                // Drain response content to free connections. Need to perform this
-                // before retry attempt and before the TooManyRetries ServiceException.
-                if(response.Content != null)
-                {
-#if NET5_0_OR_GREATER
-                    await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false);
-#else
-                    await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false);
-#endif
-                }
 
                 // Call Delay method to get delay time from response's Retry-After header or by exponential backoff
                 Task delay = RetryHandler.Delay(response, retryCount, retryOption.Delay, out double delayInSeconds, cancellationToken);
@@ -155,20 +150,9 @@ private async Task<HttpResponseMessage> SendRetryAsync(HttpResponseMessage respo
                 }
             }
 
-            // Drain response content to free connections. Need to perform this
-            // before retry attempt and before the TooManyRetries ServiceException.
-            if(response.Content != null)
-            {
-#if NET5_0_OR_GREATER
-                await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false);
-#else
-                await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false);
-#endif
-            }
+            exceptions.Add(await GetInnerExceptionAsync(response));
 
-            throw new InvalidOperationException(
-                "Too many retries performed",
-                new Exception($"More than {retryCount} retries encountered while sending the request."));
+            throw new AggregateException($"Too many retries performed. More than {retryCount} retries encountered while sending the request.", exceptions);
         }
 
         /// <summary>
@@ -248,5 +232,23 @@ private static bool ShouldRetry(HttpStatusCode statusCode)
                 _ => false
             };
         }
+
+        private static async Task<Exception> GetInnerExceptionAsync(HttpResponseMessage response)
+        {
+            string? errorMessage = null;
+
+            // Drain response content to free connections. Need to perform this
+            // before retry attempt and before the TooManyRetries ServiceException.
+            if(response.Content != null)
+            {
+                errorMessage = await response.Content.ReadAsStringAsync().ConfigureAwait(false);
+            }
+
+            return new ApiException($"HTTP request failed with status code: {response.StatusCode}.{errorMessage}")
+            {
+                ResponseStatusCode = (int)response.StatusCode,
+                ResponseHeaders = response.Headers.ToDictionary(header => header.Key, header => header.Value),
+            };
+        }
     }
 }