8
\$\begingroup\$

I've got a method that needs to PUT data to a web API. Sometimes the connection fails, so I needed a way to do retries, but if the retries fail, I still need to capture the exception and re-throw it.

I've got something that I "believe" is working, but I've got a stupid throw at the end. Is there a better, more concise way to do this when working with async tasks?

private const int TotalNumberOfAttempts = 10;

public static async Task<HttpResponseMessage> PutWithRetriesAsync(string url,
    HttpContent content,
    AuthenticationHeaderValue authenticationHeaderValue,
    MediaTypeWithQualityHeaderValue mediaTypeWithQualityHeaderValue)
{
    var numberOfAttempts = 0;
    ExceptionDispatchInfo capturedException;

    do
    {
        try
        {
            return await PutAsync(url, content, authenticationHeaderValue, mediaTypeWithQualityHeaderValue);
        }
        catch (AggregateException ex)
        {
            capturedException = ExceptionDispatchInfo.Capture(ex);
            numberOfAttempts++;
        }
    } while (numberOfAttempts < TotalNumberOfAttempts);

    if (capturedException != null)
    {
        capturedException.Throw();
    }
    throw new Exception("That will never be thrown");
}
\$\endgroup\$
3
\$\begingroup\$

I would rewrite your code into a while (true) loop, that can only be exited using the return in your try or using a throw; inside a condition in your catch:

while (true)
{
    try
    {
        return await PutAsync(url, content, authenticationHeaderValue, mediaTypeWithQualityHeaderValue);
    }
    catch
    {
        numberOfAttempts++;
        if (numberOfAttempts >= TotalNumberOfAttempts)
            throw;
    }
}

This way, you don't need ExceptionDispatchInfo or the useless (but required by the compiler) throw at the end.

I also changed catch (AggregateException ex) to catch all exceptions, because await usually doesn't throw AggregateException (unlike task.Wait() or task.Result).

\$\endgroup\$
  • \$\begingroup\$ This is a good approach. I had originally done the same thing with a do, but without the ExceptionDispatchInfo. I was just under the impression that the EDI was the right way to deal with async exceptions. \$\endgroup\$ – Chase Florell Aug 13 '14 at 15:19
  • 1
    \$\begingroup\$ @ChaseFlorell If you need to catch it in one place and then rethrow it somewhere else, yes. But you don't need to do that here, throw; will work as well and it's simpler. \$\endgroup\$ – svick Aug 13 '14 at 15:20
  • \$\begingroup\$ ah, makes sense. So this approach is equally as valid. gist.github.com/ChaseFlorell/9524b2e8a85dfa5b563e \$\endgroup\$ – Chase Florell Aug 13 '14 at 15:30
  • \$\begingroup\$ @ChaseFlorell Not really, throw ex; on an exception that you caught will overwrite the stack trace of the exception. The entire point of EDI is to avoid that. So, you should use just throw; when you can. And when you can't, use EDI, but don't rethrow an exception using throw ex;. \$\endgroup\$ – svick Aug 13 '14 at 15:40
  • \$\begingroup\$ makes perfect sense. \$\endgroup\$ – Chase Florell Aug 13 '14 at 16:15
1
\$\begingroup\$

How about:

private const int TotalNumberOfAttempts = 10;

public static async Task<HttpResponseMessage> PutWithRetriesAsync(string url,
    HttpContent content,
    AuthenticationHeaderValue authenticationHeaderValue,
    MediaTypeWithQualityHeaderValue mediaTypeWithQualityHeaderValue)
{
    var exceptions = new List<Exception>();
    for(int i = 0; i < TotalNumberOfAttempts; i++)
    {
        try
        {
            return await PutAsync(url, content, authenticationHeaderValue, mediaTypeWithQualityHeaderValue);
        }
        catch (AggregateException ex)
        {
            exceptions.Add(ex);
        }
    } 
    throw new AggregateException(exceptions);
}

You can easily abstract this functionality into an extension method to provide retry logic for any arbitrary Func<Task>.

\$\endgroup\$
  • 1
    \$\begingroup\$ Why are you rethrowing all of the exceptions? I doubt they will give you any additional information, rethrowing just the last one should be enough. \$\endgroup\$ – svick Aug 13 '14 at 14:02
  • \$\begingroup\$ This kind of retry logic is very dangerous and is rarely a good idea. The nature of the connectivity/network failure OP is up against will determine whether or not retry logic makes sense here and what to do with the exceptions he accumulates. \$\endgroup\$ – Ami Aug 13 '14 at 16:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.