Request().WithShouldRetry is not called for failed requests.

[spmanjunath]

Expected behavior

i am trying to overcome intermittent 409 error that occurs while uploading/updating metadata of a file in SharePoint's Document library using Microsoft Graph SDK. To retry failed calls SDK provides WithMaxRetry() and WithShouldRetry() options. The MaxRetry works for error codes 429, and I am assuming that ShouldRetry delegate offers us an option to implement our own evaluation logic for the retries. Based on this assumption, I have the below code:

_graphServiceClientFactory.GetClient().Drives[driveId].Root.ItemWithPath(path).ListItem.Fields.Request()
                       .WithShouldRetry((delay, attempt, httpResponse) =>
                        (attempt <= 5 &&
                        (httpResponse.StatusCode == HttpStatusCode.Conflict)))
                       .UpdateAsync(new FieldValueSet { AdditionalData = dataDictionary });

Actual behavior

In my test the ShouldRetry delegate is never evaluated on failed requests. I am not sure if the above usage is correct (i could not find any reference of its usage), please suggest otherwise.

Thanks.
AB#7188

[spmanjunath]

From reading RetryHandler.cs file there appears to be inconsistency in the way the conditional operator is used w.r.t. Retry in SendAsync() and SendRetryAsync() method. In the below SendAsync() method the ShouldRetry() and RetryOption.ShouldRetry() is subjected to 'and' conditional operator, whereas in the SendRetryAsync() method the ShouldRetry() and RetryOption.ShouldRetry() is evaluated with 'or' conditional operator.

SendAsync() method:

if (ShouldRetry(response) && httpRequest.IsBuffered() && RetryOption.MaxRetry > 0 && RetryOption.ShouldRetry(RetryOption.Delay, 0, response))
{
    response = await SendRetryAsync(response, cancellationToken);
}

SendRetryAsync() method:

 if (!ShouldRetry(response) || !request.IsBuffered() || !RetryOption.ShouldRetry(RetryOption.Delay, retryCount, response))
{
    return response;
}

I am not sure if the above difference is intentional, if the objective of WithShouldRetry() is to provide independent custom evaluation then ShouldRetry() and RetryOption.ShouldRetry() should be using 'or' operator. The SendRetry() method should therefore be as below,

protected async override Task<HttpResponseMessage> SendAsync(HttpRequestMessage httpRequest, CancellationToken cancellationToken)
{
    RetryOption = httpRequest.GetMiddlewareOption<RetryHandlerOption>() ?? RetryOption;

    var response = await base.SendAsync(httpRequest, cancellationToken);

    // Check whether retries are permitted and that the MaxRetry value is a non - negative, non - zero value
    if ((ShouldRetry(response)  || RetryOption.ShouldRetry(RetryOption.Delay, 0, response)) && httpRequest.IsBuffered() && RetryOption.MaxRetry > 0)
    {
        response = await SendRetryAsync(response, cancellationToken);
    }

    return response;
}              

[andrueastman]

Hey, @spmanjunath!
Thanks for raising this issue.
It does indeed seem that if any custom codes not listed here are passed into ShouldRetry should retry delegate will not be evaluated due to the constraints placed at the moment as you have explained.
I will mark this as a bug and so that we can work towards having this resolved.

[andrueastman]

Closed via #242

[brcaswell]

@andrueastman I think there is a misunderstanding here. This issue raised 2 observations:

• SendAsync and SendRetryAsync have inconsistent conditional check logic.
• The second observation related to the logic you reviewed on and addressed with Fix for Request().WithShouldRetry is not called #242

However,

msgraph-sdk-dotnet-core/src/Microsoft.Graph.Core/Requests/Middleware/RetryHandler.cs

Line 118 in 6e1a5a9

if (!ShouldRetry(response) || !request.IsBuffered() || !RetryOption.ShouldRetry(RetryOption.Delay, retryCount, response))

in SendRetryAsync should also be handled (in feature 2.0) and that handling should be consistent with SendAsync conditional check changes, right?

[brcaswell]

@andrueastman I pulled the latest head on feature/2.0 branch, updated RetryHandlerTest to perform a test that attempts more then 2 requests (1 request and the additional retries). that MockHttpMessageHandler type that is used in RetryHandlerTest needs to be deprecated. it only ever allowed for a 2 request scenario and it's unnecessary...

The updated test demonstrated SendRetryAsync returning after 1 Retry (i.e. failing to adher to MaxRetry in RetryHandlerOptions) - in respect and due to this logic check above.

see brcaswell#1 Pull Request that has these changes in it (in respect to branch off of feature 2.0 in forked repo)

branch and repo link brcaswell/msgraph-sdk-dotnet-core:update-retryhandler-test

[andrueastman]

@brcaswell Thanks for explaining this so well with the sample PR.
Are you able to change this to target the feature/2.0 branch of this repo so that we may review and merge in the change?

[brcaswell]

@andrueastman yea, I can do that by creating a new PR.. it seems there is no supported way to retarget the base on that existing PR to target upstream repo branch. probably going to see duplicate mentions in this issue and referenced PR