How can I improve this exception retry scenario?

Question

I have a web service method I am calling which is 3rd party and outside of my domain. For some reason now and again the web service fails with a gateway timeout. It is intermittent and a call to it directly after a failed attempt can succeed.

Now I am left with a coding dilemma, I have code that should do the trick, but the code looks like amateur hour, as you'll see below.

Is this really bad code, or acceptable given the usage? If it's not acceptable, how can I improve it?

Please try hard to keep a straight face while looking at it.

try
{
    MDO = OperationsWebService.MessageDownload(MI);
}
catch
{
    try
    {
        MDO = OperationsWebService.MessageDownload(MI);
    }
    catch
    {
        try
        {
            MDO = OperationsWebService.MessageDownload(MI);
        }
        catch
        {
            try
            {
                MDO = OperationsWebService.MessageDownload(MI);
            }
            catch 
            {
                try
                {
                    MDO = OperationsWebService.MessageDownload(MI);
                }
                catch (Exception ex)
                {
                    // 5 retries, ok now log in and deal with the error.
                }
            }
        }
    }
}

Auxiliary

I just have to give you +1 for the awesome nesting! – Centralize

@Alastair: Apparently, you don't. (The question has no votes) – Germanism

/me try{ keep_straight_face } catch { laugh } finally { FAIL! } – Supplemental

Don't upvote this question :) but I had to ask :) – Auxiliary

@JL: Had to modify my comment in the spirit of your question! :) – Supplemental

@Tommieb75 - very funny... :) – Auxiliary

Answer 1

You can do it in a loop.

Exception firstEx = null;
for(int i=0; i<5; i++) 
{
    try
    {
        MDO = OperationsWebService.MessageDownload(MI);
        firstEx = null;
        break; 
    }
    catch(Exception ex)
    {
        if (firstEx == null) 
        {
            firstEx = ex;
        }
        Thread.Sleep(100 * (i + 1));
    }
}
if (firstEx != null) 
{
    throw new Exception("WebService call failed after 5 retries.", firstEx);
}

Afeard

Look at Martin Buberl's answer below (#2270771), which uses PreserveStackTrace feature. Otherwise, the stack trace will point to the "throw" statement, and not the real place of the exception. – Emanate

@Mark Lakata, using reflection to invoke internal methods is a horrible idea and very likely to lead to long term maintenance problems when it suddenly stops working in production. Worse yet, you end up with an exception handler that throws an exception in unusual situations and you lose the original exception entirely. There is no need to "preserve" the stack trace because when you create an exception that wraps another exception, the original stack trace is 100% preserved already in the innerException. – Afeard

The first exception is apt to be more useful than the last, so it's probably good to let that be the InnerException. It's really too bad there's no standard way of indicating that an exception should have more than one inner exception, though, since it's possible that all of the exceptions might hold useful information. – Batfish

@supercat, this situation doesn't come up all that often and doesn't exist anywhere in the BCL that I know of. It is straightforward to create a custom ExceptionCollection exception that includes a bunch of other exceptions and prints them all out when ToString is called. In our actual applications we typically log every exception and then just throw the first. – Afeard

I agree the first is often the most useful, but if exceptions include detail information, it may be helpful to include such information for all exceptions. For example, a TimeoutException for a packetized I/O class might include information about how long the class waited, and what sort of data was received (e.g. received 45 bytes, but no valid packet header). It may be helpful to know, if there are five TimeoutExceptions, whether the remote system was sending anything but what it sent wasn't recognized, or whether the remote system was simply silent. – Batfish

Answer 2

Here's another way you might try:

// Easier to change if you decide that 5 retries isn't right for you
Exception exceptionKeeper = null;
for (int i = 0; i < MAX_RETRIES; ++i)
{
    try
    {
       MDO = OperationsWebService.MessageDownload(MI);
       break;  // correct point from Joe - thanks.
    }
    catch (Exception ex)
    {
        exceptionKeeper = ex;
        // 5 retries, ok now log and deal with the error.
    }  
}

I think it documents the intent better. It's less code as well; easier to maintain.

Ivories

That looks much neater indeed, I guess in the catch I check for the value of MAX_RETRIES? – Auxiliary

I don't think this works. It would go into the catch block on the first exception. – Prosit

probably want a 'break' at the end of the try. :) – Halfmoon

+1 I was writing almost the exact same thing before I saw the orange banner. – Eulogium

No, you'll just exit the loop if MAX_RETRIES is 5. – Kizer

In the above code , how can I keep the original exception after MAX_RETRIES is met? – Auxiliary

@JL: at the start of the loop just declare a variable of type Exception, and set this equal to the very first exception that is caught. Then, once the retry count is hit, just re-throw it. – Lubricant

Answer 3

All of the answers so far assume that the reaction to any exception should be to retry the operation. This is a good assumption right up until it's a false assumption. You could easily be retrying an operation that is damaging your system, all because you didn't check the exception type.

You should almost never use a bare "catch", nor "catch (Exception ex). Catch a more-specific exception - one you know you can safely recover from.

Galenical

Good point. But doesn't an exception get raised before it damages the system? Isn't that the point? – Prosit

@Plynx: I was exaggerating a little. The operation being retried might have initiated some expensive processing, then thrown an exception, indicating "disk full" or something else that means the operation will never succeed. Retrying it would only waste system resources and never result in a success. – Galenical

+1 In the author's scenario he states the problem is a communications one, not dealing with the local system. However I agree with the general reasoning. – Blake

@jdk: if he knows it's a communications problem, he should be catching CommunicationsException and TimeoutException, not Exception. – Galenical

S.: Good additional points to supplement the answer. +1 for that comment too :) – Blake

Answer 4

Try a loop, with some kind of limit:

int retryCount = 5;
var done = false;
Exception error = null;
while (!done && retryCount > 0)
{
    try
    {
        MDO = OperationsWebService.MessageDownload(MI);
        done = true;
    }
    catch (Exception ex)
    {
        error = ex;
    }
    if (done)
        break;

    retryCount--;
}

Schlenger

Answer 5

You should use recursion (or a loop), and should only retry if you got the error you expected.

For example:

static void TryExecute<TException>(Action method, Func<TException, bool> retryFilter, int maxRetries) where TException : Exception {
    try {
        method();
    } catch(TException ex) {
        if (maxRetries > 0 && retryFilter(ex))
            TryExecute(method, retryFilter, maxRetries - 1);
        else
            throw;
    }
}

EDIT: With a loop:

static void TryExecute<TException>(Action method, Func<TException, bool> retryFilter, int maxRetries) where TException : Exception {
    while (true) {
        try {
            method();
            return;
        } catch(TException ex) {
            if (maxRetries > 0 && retryFilter(ex))
                maxRetries--;
            else
                throw;
        }
    }
}

---

You can try to prevent future errors in retryFilter, perhaps by Thread.Sleep.

If the last retry fails, this will throw the last exception.

Germanism

As almost everyone else mentioned, you can, and probably should, use a loop. – Germanism

Answer 6

Here is some retry logic we are using. We don't do this a lot and I was going to pull it out and document it as our Retry Pattern/Standard. I had to wing it when I first wrote it so I came here to see if I was doing it correctly. Looks like I was. The version below is fully commented. See below that for an uncommented version.

#region Retry logic for SomeWebService.MyMethod
// The following code wraps SomeWebService.MyMethod in retry logic
// in an attempt to account for network failures, timeouts, etc.

// Declare the return object for SomeWebService.MyMethod outside of
// the following for{} and try{} code so that we have it afterwards.
MyMethodResult result = null;

// This logic will attempt to retry the call to SomeWebService.MyMethod
for (int retryAttempt = 1; retryAttempt <= Config.MaxRetryAttempts; retryAttempt++)
{
    try
    {
        result = SomeWebService.MyMethod(myId);

        // If we didn't get an exception, then that (most likely) means that the
        // call was successful so we can break out of the retry logic.
        break;
    }
    catch (Exception ex)
    {
        // Ideally we want to only catch and act on specific
        // exceptions related to the failure. However, in our
        // testing, we found that the exception could be any type
        // (service unavailable, timeout, database failure, etc.)
        // and attempting to trap every exception that was retryable
        // was burdensome. It was easier to just retry everything
        // regardless of the cause of the exception. YMMV. Do what is
        // appropriate for your scenario.

        // Need to check to see if there will be another retry attempt allowed.
        if (retryAttempt < Config.MaxRetryAttempts)
        {
            // Log that we are re-trying
            Logger.LogEvent(string.Format("Retry attempt #{0} for SomeWebService.MyMethod({1})", retryAttempt, myId);

            // Put the thread to sleep. Rather than using a straight time value for each
            // iteration, we are going to multiply the sleep time by how many times we
            // have currently tried to call the method. This will allow for an easy way to
            // cover a broader range of time without having to use higher retry counts or timeouts.
            // For example, if MaxRetryAttempts = 10 and RetrySleepSeconds = 60, the coverage will
            // be as follows:
            // - Retry #1 - Sleep for 1 minute
            // - Retry #2 - Sleep for 2 minutes (covering three minutes total)
            // - Retry #10 - Sleep for 10 minutes (and will have covered almost an hour of downtime)
            Thread.Sleep(retryAttempt * Config.RetrySleepSeconds * 1000);
        }
        else
        {
            // If we made it here, we have tried to call the method several
            // times without any luck. Time to give up and move on.

            // Moving on could either mean:
            // A) Logging the exception and moving on to the next item.
            Logger.LogError(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", MyId), ex);
            // B) Throwing the exception for the program to deal with.
            throw new Exception(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", myId), ex);
            // Or both. Your code, your call.
        }
    }
}
#endregion

I like Samuel Neff's example of using an exception variable to see if it completely failed or not. That would have made some of the evaluations in my logic a little simpler. I could go either way. Not sure that either way has a significant advantage over the other. However, at this point in time, I'm not going to change how we do it. The important thing is to document what you are doing and why so that some idiot doesn't come through behind you and muck with everything.

Just for kicks though, to get a better idea if the code is any shorter or cleaner one way or the other, I pulled out all the comments. They came out exactly the same number of lines. I went ahead and compiled the two versions and ran them through Reflector Code Metrics and got the following:

Metric: Inside-Catch / Outside-For
CodeSize: 197 / 185
CyclomaticComplexity: 3 / 3
Instructions: 79 / 80
Locals: 6 / 7

Final exception logic inside the catch (22 lines):

MyMethodResult result = null;
for (int retryAttempt = 1; retryAttempt <= Config.MaxRetryAttempts; retryAttempt++)
{
    try
    {
        result = SomeWebService.MyMethod(myId);
        break;
    }
    catch (Exception ex)
    {
        if (retryAttempt < Config.MaxRetryAttempts)
        {
            Logger.LogEvent(string.Format("Retry attempt #{0} for SomeWebService.MyMethod({1})", retryAttempt, myId);
            Thread.Sleep(retryAttempt * Config.RetrySleepSeconds * 1000);
        }
        else
        {
            Logger.LogError(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", MyId), ex);
            throw new Exception(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", myId), ex);
        }
    }
}

Final exception logic after the for-loop (22 lines):

MyMethodResult result = null;
Exception retryException = null;
for (int retryAttempt = 1; retryAttempt <= Config.MaxRetryAttempts; retryAttempt++)
{
    try
    {
        result = SomeWebService.MyMethod(myId);
        retryException = null;
        break;
    }
    catch (Exception ex)
    {
        retryException = ex;
        Logger.LogEvent(string.Format("Retry attempt #{0} for SomeWebService.MyMethod({1})", retryAttempt, myId);
        Thread.Sleep(retryAttempt * Config.RetrySleepSeconds * 1000);
    }
}
if (retryException != null)
{
    Logger.LogError(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", MyId), ex);
    throw new Exception(string.Format("Max Retry Attempts Exceeded for SomeWebService.MyMethod({0})", myId), ex);
}

Ascomycete

Answer 7

I'm using the following generic method for a retry scenario. I especially want to draw attention to the PreserveStackTrace method which helps to preserve the full call stack trace, because (as I learned the hard way) neither throw or throw ex yields the complete call stack trace information.

public static void RetryBeforeThrow<T>(Action action, int retries, int timeout) where T : Exception
{
    int tries = 1;

    do
    {
        try
        {
            action();
            return;
        }
        catch (T ex)
        {
            if (retries <= 0)
            {
                PreserveStackTrace(ex);
                throw;
            }

            Thread.Sleep(timeout);
        }
    }
    while (tries++ < retries);
}

/// <summary>
/// Sets a flag on an <see cref="T:System.Exception"/> so that all the stack trace information is preserved 
/// when the exception is re-thrown.
/// </summary>
/// <remarks>This is useful because "throw" removes information, such as the original stack frame.</remarks>
/// <see href="http://weblogs.asp.net/fmarguerie/archive/2008/01/02/rethrowing-exceptions-and-preserving-the-full-call-stack-trace.aspx"/>
public static void PreserveStackTrace(Exception ex)
{
    MethodInfo preserveStackTrace = typeof(Exception).GetMethod("InternalPreserveStackTrace", BindingFlags.Instance | BindingFlags.NonPublic);
    preserveStackTrace.Invoke(ex, null);
}

Jobyna

Well done, but recently there is a better way: https://mcmap.net/q/535668/-how-can-i-improve-this-exception-retry-scenario – Tholos

Answer 8

int cnt=0;
bool cont = true;
while (cont)
{
     try
     {
         MDO = OperationsWebService.MessageDownload(MI);
         cont = false; 
     }
     catch (Exception ex) 
     { 
         ++cnt;
         if (cnt == 5)
         {
             // 5 retries, ok now log and deal with the error. 
            cont = false;
         }
     } 
}

UPDATED : Fixed code based on comments.

Gruel

Answer 9

As everyone else has pointed out the correct approach is to wrap your try/catch inside some loop with a MAX_RETRY of some sort.

You might also consider adding a timeout between each loop iteration. Otherwise you're likely to burn through your retry counter before the transient issue has had a chance to resolve itself.

Lubricant

Answer 10

An improvement to a few other answers is not to rethrow at the end, by using catch (...) when ... (since C# 6)

Exception filters are preferable to catching and rethrowing because they leave the stack unharmed.

/// <summary>
/// Retries an async action when a specific exception occurs, on last try the exception is not catched
/// </summary>
public static async Task RetryAsync<T>(Func<Task> action, int tries = 3, int sleep = 0, ILogger? logger = null) where T : Exception
{
    while (true)
    {
        try
        {
            await action();
            return;
        }
        catch (T ex) when (--tries > 0)
        {
            logger?.LogError(ex, "{Message}\nRetrying ({tries} remaining)...", ex.Message, tries);
            await Task.Delay(sleep);
        }
    }
}

/// <summary>
/// Retries an action when a specific exception occurs, on last try the exception is not catched
/// </summary>
public static void Retry<T>(Action action, int tries = 3, int sleep = 0, ILogger? logger = null) where T : Exception
{
    while (true)
    {
        try
        {
            action();
            return;
        }
        catch (T ex) when (--tries > 0)
        {
            logger?.LogError(ex, "{Message}\nRetrying ({tries} remaining)...", ex.Message, tries);
            Thread.Sleep(sleep);
        }
    }
}

//TODO: we need additional versions to return results

Fiddle

Tholos