Skip to content
This repository has been archived by the owner on Jul 9, 2024. It is now read-only.

Add more information on failures #223

Merged
merged 8 commits into from
Mar 25, 2024
Merged

Add more information on failures #223

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a0bd3aa
Add more information on failures
Mar 24, 2024
c764430
Shorten some tests time.
Mar 24, 2024
d815e33
Remove default error message text.
Mar 24, 2024
dba777e
Update src/Middleware/RetryHandler.cs
DCourtel Mar 25, 2024
242df12
Update src/Middleware/RetryHandler.cs
DCourtel Mar 25, 2024
906a4fd
Update src/Middleware/RetryHandler.cs
DCourtel Mar 25, 2024
34f91da
Update src/Middleware/RetryHandler.cs
DCourtel Mar 25, 2024
376eb06
Resolve comments
Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/RetryHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ public void RetryHandlerConstructor()
Assert.IsType<RetryHandler>(retry);
}


[Fact]
public void RetryHandlerHttpMessageHandlerConstructor()
{
Expand Down Expand Up @@ -89,7 +88,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]
Expand All @@ -116,7 +114,6 @@ public async Task ShouldRetryWithAddRetryAttemptHeader(HttpStatusCode statusCode
Assert.Equal(values.First(), 1.ToString());
}


[Theory]
[InlineData(HttpStatusCode.GatewayTimeout)] // 504
[InlineData(HttpStatusCode.ServiceUnavailable)] // 503
Expand All @@ -140,7 +137,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]
Expand Down Expand Up @@ -196,8 +192,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
Expand All @@ -208,16 +203,23 @@ 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.False(httpRequestMessage.Headers.TryGetValues(RetryAttempt, out _), "Don't set Retry-Attempt Header");
}
}
Expand Down Expand Up @@ -246,17 +248,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
Expand All @@ -269,7 +270,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);
}
Expand Down Expand Up @@ -351,7 +352,6 @@ public async Task ShouldRetryBasedOnRetryAfterHeaderWithHttpDate(HttpStatusCode
Assert.NotSame(response.RequestMessage, httpRequestMessage);
}


[Theory]
[InlineData(1, HttpStatusCode.BadGateway, true)]
[InlineData(2, HttpStatusCode.BadGateway, true)]
Expand Down Expand Up @@ -411,14 +411,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)
Expand Down
62 changes: 38 additions & 24 deletions src/Middleware/RetryHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,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)
{
Expand All @@ -69,7 +70,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
Expand All @@ -93,27 +93,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 GetInnerException(response, cancellationToken));
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);
Expand Down Expand Up @@ -155,20 +148,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 GetInnerException(response, cancellationToken));

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.", exceptions);
DCourtel marked this conversation as resolved.
Show resolved Hide resolved
}

/// <summary>
Expand Down Expand Up @@ -248,5 +230,37 @@ private static bool ShouldRetry(HttpStatusCode statusCode)
_ => false
};
}

private static async Task<Exception> GetInnerException(HttpResponseMessage response, CancellationToken cancellationToken)
DCourtel marked this conversation as resolved.
Show resolved Hide resolved
{
var httpStatusCode = response.StatusCode;
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)
{
#if NET5_0_OR_GREATER
var responseContent = await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false);
#else
var responseContent = await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false);
DCourtel marked this conversation as resolved.
Show resolved Hide resolved
#endif
errorMessage = GetStringFromContent(responseContent);
}

return new Exception($"HTTP request failed with status code: {httpStatusCode}.{errorMessage}");
DCourtel marked this conversation as resolved.
Show resolved Hide resolved
}

private static string GetStringFromContent(byte[] content)
{
try
{
return System.Text.Encoding.UTF8.GetString(content);
}
catch(Exception)
{
return "The Graph retry handler was unable to cast the response into a UTF8 string.";
}
}
}
}