Skip to content

Commit

Permalink
fix: dispose async when possible
Browse files Browse the repository at this point in the history 
Signed-off-by: Vincent Biret <[email protected]>
  • Loading branch information
@baywet
baywet committed Aug 27, 2024
1 parent ebf15e4 commit 1b2094d
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 28 deletions.
79 changes: 54 additions & 25 deletions src/http/httpClient/HttpClientRequestAdapter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,21 @@ public string? BaseUrl
public async Task<IEnumerable<ModelType>?> SendCollectionAsync<ModelType>(RequestInformation requestInfo, ParsableFactory<ModelType> factory, Dictionary<string, ParsableFactory<IParsable>>? errorMapping = default, CancellationToken cancellationToken = default) where ModelType : IParsable
{
using var span = startTracingSpan(requestInfo, nameof(SendCollectionAsync));
var response = await GetHttpResponseMessage(requestInfo, cancellationToken, span).ConfigureAwait(false);
var response = await GetHttpResponseMessageAsync(requestInfo, cancellationToken, span).ConfigureAwait(false);
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_0_OR_GREATER
if (requestInfo.Content is not null)
await requestInfo.Content.DisposeAsync().ConfigureAwait(false);
#else
requestInfo.Content?.Dispose();
#endif
var responseHandler = GetResponseHandler(requestInfo);
if(responseHandler == null)
{
try
{
await ThrowIfFailedResponse(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
await ThrowIfFailedResponseAsync(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
if(shouldReturnNull(response)) return default;
var rootNode = await GetRootParseNode(response, cancellationToken).ConfigureAwait(false);
var rootNode = await GetRootParseNodeAsync(response, cancellationToken).ConfigureAwait(false);
using var spanForDeserialization = activitySource?.StartActivity(nameof(IParseNode.GetCollectionOfObjectValues));
var result = rootNode?.GetCollectionOfObjectValues<ModelType>(factory);
SetResponseType(result, span);
Expand Down Expand Up @@ -131,16 +136,21 @@ public string? BaseUrl
#endif
{
using var span = startTracingSpan(requestInfo, nameof(SendPrimitiveCollectionAsync));
var response = await GetHttpResponseMessage(requestInfo, cancellationToken, span).ConfigureAwait(false);
var response = await GetHttpResponseMessageAsync(requestInfo, cancellationToken, span).ConfigureAwait(false);
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_0_OR_GREATER
if (requestInfo.Content is not null)
await requestInfo.Content.DisposeAsync().ConfigureAwait(false);
#else
requestInfo.Content?.Dispose();
#endif
var responseHandler = GetResponseHandler(requestInfo);
if(responseHandler == null)
{
try
{
await ThrowIfFailedResponse(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
await ThrowIfFailedResponseAsync(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
if(shouldReturnNull(response)) return default;
var rootNode = await GetRootParseNode(response, cancellationToken).ConfigureAwait(false);
var rootNode = await GetRootParseNodeAsync(response, cancellationToken).ConfigureAwait(false);
using var spanForDeserialization = activitySource?.StartActivity(nameof(IParseNode.GetCollectionOfPrimitiveValues));
var result = rootNode?.GetCollectionOfPrimitiveValues<ModelType>();
SetResponseType(result, span);
Expand Down Expand Up @@ -172,16 +182,21 @@ public string? BaseUrl
public async Task<ModelType?> SendAsync<ModelType>(RequestInformation requestInfo, ParsableFactory<ModelType> factory, Dictionary<string, ParsableFactory<IParsable>>? errorMapping = default, CancellationToken cancellationToken = default) where ModelType : IParsable
{
using var span = startTracingSpan(requestInfo, nameof(SendAsync));
var response = await GetHttpResponseMessage(requestInfo, cancellationToken, span).ConfigureAwait(false);
var response = await GetHttpResponseMessageAsync(requestInfo, cancellationToken, span).ConfigureAwait(false);
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_0_OR_GREATER
if (requestInfo.Content is not null)
await requestInfo.Content.DisposeAsync().ConfigureAwait(false);
#else
requestInfo.Content?.Dispose();
#endif
var responseHandler = GetResponseHandler(requestInfo);
if(responseHandler == null)
{
try
{
await ThrowIfFailedResponse(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
await ThrowIfFailedResponseAsync(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
if(shouldReturnNull(response)) return default;
var rootNode = await GetRootParseNode(response, cancellationToken).ConfigureAwait(false);
var rootNode = await GetRootParseNodeAsync(response, cancellationToken).ConfigureAwait(false);
if(rootNode == null) return default;
using var spanForDeserialization = activitySource?.StartActivity(nameof(IParseNode.GetObjectValue));
var result = rootNode.GetObjectValue<ModelType>(factory);
Expand Down Expand Up @@ -218,14 +233,19 @@ public string? BaseUrl
using var span = startTracingSpan(requestInfo, nameof(SendPrimitiveAsync));
var modelType = typeof(ModelType);
var isStreamResponse = modelType == typeof(Stream);
var response = await GetHttpResponseMessage(requestInfo, cancellationToken, span, isStreamResponse: isStreamResponse).ConfigureAwait(false);
var response = await GetHttpResponseMessageAsync(requestInfo, cancellationToken, span, isStreamResponse: isStreamResponse).ConfigureAwait(false);
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_0_OR_GREATER
if (requestInfo.Content is not null)
await requestInfo.Content.DisposeAsync().ConfigureAwait(false);
#else
requestInfo.Content?.Dispose();
#endif
var responseHandler = GetResponseHandler(requestInfo);
if(responseHandler == null)
{
try
{
await ThrowIfFailedResponse(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
await ThrowIfFailedResponseAsync(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
if(shouldReturnNull(response)) return default;
if(isStreamResponse)
{
Expand All @@ -236,15 +256,19 @@ public string? BaseUrl
#endif
if(result.CanSeek && result.Length == 0)
{
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_0_OR_GREATER
await result.DisposeAsync().ConfigureAwait(false);
#else
result.Dispose();
#endif
return default;
}
SetResponseType(result, span);
return (ModelType)(result as object);
}
else
{
var rootNode = await GetRootParseNode(response, cancellationToken).ConfigureAwait(false);
var rootNode = await GetRootParseNodeAsync(response, cancellationToken).ConfigureAwait(false);
object? result;
using var spanForDeserialization = activitySource?.StartActivity($"Get{modelType.Name.TrimEnd('?')}Value");
if(rootNode == null)
Expand Down Expand Up @@ -336,14 +360,19 @@ public string? BaseUrl
public async Task SendNoContentAsync(RequestInformation requestInfo, Dictionary<string, ParsableFactory<IParsable>>? errorMapping = default, CancellationToken cancellationToken = default)
{
using var span = startTracingSpan(requestInfo, nameof(SendNoContentAsync));
var response = await GetHttpResponseMessage(requestInfo, cancellationToken, span).ConfigureAwait(false);
var response = await GetHttpResponseMessageAsync(requestInfo, cancellationToken, span).ConfigureAwait(false);
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_0_OR_GREATER
if (requestInfo.Content is not null)
await requestInfo.Content.DisposeAsync().ConfigureAwait(false);
#else
requestInfo.Content?.Dispose();
#endif
var responseHandler = GetResponseHandler(requestInfo);
if(responseHandler == null)
{
try
{
await ThrowIfFailedResponse(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
await ThrowIfFailedResponseAsync(response, errorMapping, span, cancellationToken).ConfigureAwait(false);
}
finally
{
Expand Down Expand Up @@ -392,9 +421,9 @@ private static bool shouldReturnNull(HttpResponseMessage response)
/// The attribute name used to indicate whether the error response contained a body.
/// </summary>
public const string ErrorBodyFoundAttributeName = "com.microsoft.kiota.error.body_found";
private async Task ThrowIfFailedResponse(HttpResponseMessage response, Dictionary<string, ParsableFactory<IParsable>>? errorMapping, Activity? activityForAttributes, CancellationToken cancellationToken)
private async Task ThrowIfFailedResponseAsync(HttpResponseMessage response, Dictionary<string, ParsableFactory<IParsable>>? errorMapping, Activity? activityForAttributes, CancellationToken cancellationToken)
{
using var span = activitySource?.StartActivity(nameof(ThrowIfFailedResponse));
using var span = activitySource?.StartActivity(nameof(ThrowIfFailedResponseAsync));
if(response.IsSuccessStatusCode) return;

activityForAttributes?.SetStatus(ActivityStatusCode.Error, "received_error_response");
Expand All @@ -420,7 +449,7 @@ private async Task ThrowIfFailedResponse(HttpResponseMessage response, Dictionar
}
activityForAttributes?.SetTag(ErrorMappingFoundAttributeName, true);

var rootNode = await GetRootParseNode(response, cancellationToken).ConfigureAwait(false);
var rootNode = await GetRootParseNodeAsync(response, cancellationToken).ConfigureAwait(false);
activityForAttributes?.SetTag(ErrorBodyFoundAttributeName, rootNode != null);
var spanForDeserialization = activitySource?.StartActivity(nameof(IParseNode.GetObjectValue));
var result = rootNode?.GetObjectValue(errorFactory);
Expand All @@ -444,9 +473,9 @@ private async Task ThrowIfFailedResponse(HttpResponseMessage response, Dictionar
{
return requestInfo.GetRequestOption<ResponseHandlerOption>()?.ResponseHandler;
}
private async Task<IParseNode?> GetRootParseNode(HttpResponseMessage response, CancellationToken cancellationToken)
private async Task<IParseNode?> GetRootParseNodeAsync(HttpResponseMessage response, CancellationToken cancellationToken)
{
using var span = activitySource?.StartActivity(nameof(GetRootParseNode));
using var span = activitySource?.StartActivity(nameof(GetRootParseNodeAsync));
var responseContentType = response.Content?.Headers?.ContentType?.MediaType?.ToLowerInvariant();
if(string.IsNullOrEmpty(responseContentType))
return null;
Expand All @@ -466,9 +495,9 @@ private async Task ThrowIfFailedResponse(HttpResponseMessage response, Dictionar
private const string ClaimsKey = "claims";
private const string BearerAuthenticationScheme = "Bearer";
private static Func<AuthenticationHeaderValue, bool> filterAuthHeader = static x => x.Scheme.Equals(BearerAuthenticationScheme, StringComparison.OrdinalIgnoreCase);
private async Task<HttpResponseMessage> GetHttpResponseMessage(RequestInformation requestInfo, CancellationToken cancellationToken, Activity? activityForAttributes, string? claims = default, bool isStreamResponse = false)
private async Task<HttpResponseMessage> GetHttpResponseMessageAsync(RequestInformation requestInfo, CancellationToken cancellationToken, Activity? activityForAttributes, string? claims = default, bool isStreamResponse = false)
{
using var span = activitySource?.StartActivity(nameof(GetHttpResponseMessage));
using var span = activitySource?.StartActivity(nameof(GetHttpResponseMessageAsync));
if(requestInfo == null)
throw new ArgumentNullException(nameof(requestInfo));

Expand Down Expand Up @@ -504,7 +533,7 @@ private async Task<HttpResponseMessage> GetHttpResponseMessage(RequestInformatio
activityForAttributes?.SetTag("http.status_code", (int)response.StatusCode);
activityForAttributes?.SetTag("http.flavor", $"{response.Version.Major}.{response.Version.Minor}");

return await RetryCAEResponseIfRequired(response, requestInfo, cancellationToken, claims, activityForAttributes).ConfigureAwait(false);
return await RetryCAEResponseIfRequiredAsync(response, requestInfo, cancellationToken, claims, activityForAttributes).ConfigureAwait(false);
}

private static readonly Regex caeValueRegex = new("\"([^\"]*)\"", RegexOptions.Compiled, TimeSpan.FromMilliseconds(100));
Expand All @@ -515,9 +544,9 @@ private async Task<HttpResponseMessage> GetHttpResponseMessage(RequestInformatio
public const string AuthenticateChallengedEventKey = "com.microsoft.kiota.authenticate_challenge_received";
private static readonly char[] ComaSplitSeparator = [','];

private async Task<HttpResponseMessage> RetryCAEResponseIfRequired(HttpResponseMessage response, RequestInformation requestInfo, CancellationToken cancellationToken, string? claims, Activity? activityForAttributes)
private async Task<HttpResponseMessage> RetryCAEResponseIfRequiredAsync(HttpResponseMessage response, RequestInformation requestInfo, CancellationToken cancellationToken, string? claims, Activity? activityForAttributes)
{
using var span = activitySource?.StartActivity(nameof(RetryCAEResponseIfRequired));
using var span = activitySource?.StartActivity(nameof(RetryCAEResponseIfRequiredAsync));
if(response.StatusCode == HttpStatusCode.Unauthorized &&
string.IsNullOrEmpty(claims) && // avoid infinite loop, we only retry once
(requestInfo.Content?.CanSeek ?? true))
Expand Down Expand Up @@ -559,7 +588,7 @@ private async Task<HttpResponseMessage> RetryCAEResponseIfRequired(HttpResponseM
activityForAttributes?.SetTag("http.retry_count", 1);
requestInfo.Content?.Seek(0, SeekOrigin.Begin);
await DrainAsync(response, cancellationToken).ConfigureAwait(false);
return await GetHttpResponseMessage(requestInfo, cancellationToken, activityForAttributes, responseClaims).ConfigureAwait(false);
return await GetHttpResponseMessageAsync(requestInfo, cancellationToken, activityForAttributes, responseClaims).ConfigureAwait(false);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/http/httpClient/Middleware/RetryHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ private async Task<HttpResponseMessage> SendRetryAsync(HttpResponseMessage respo
retryActivity?.SetTag("http.status_code", response.StatusCode);

// 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);
Task delay = RetryHandler.DelayAsync(response, retryCount, retryOption.Delay, out double delayInSeconds, cancellationToken);

// If client specified a retries time limit, let's honor it
if(retryOption.RetriesTimeLimit > TimeSpan.Zero)
Expand Down Expand Up @@ -176,7 +176,7 @@ private static void AddOrUpdateRetryAttempt(HttpRequestMessage request, int retr
/// <param name="delayInSeconds"></param>
/// <param name="cancellationToken">The cancellationToken for the Http request</param>
/// <returns>The <see cref="Task"/> for delay operation.</returns>
internal static Task Delay(HttpResponseMessage response, int retryCount, int delay, out double delayInSeconds, CancellationToken cancellationToken)
internal static Task DelayAsync(HttpResponseMessage response, int retryCount, int delay, out double delayInSeconds, CancellationToken cancellationToken)
{
delayInSeconds = delay;
if(response.Headers.TryGetValues(RetryAfter, out IEnumerable<string>? values))
Expand Down
2 changes: 1 addition & 1 deletion tests/http/httpClient/Middleware/RetryHandlerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ private async Task DelayTestWithMessage(HttpResponseMessage response, int count,
Message = message;
await Task.Run(async () =>
{
await RetryHandler.Delay(response, count, delay, out _, new CancellationToken());
await RetryHandler.DelayAsync(response, count, delay, out _, new CancellationToken());
Message += " Work " + count;
});
}
Expand Down

0 comments on commit 1b2094d

Please sign in to comment.