Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

We've had a situation whereby we have to cater for certain exceptions and retry a particular method whenever these exceptions occur. This is required in various parts of the system. In addition, we (might) want to invoke a 'callback' upon the need to retry.

I'll show you the code, then give you an example of useage:

    public static void InvokeActionWithRetry(Action action, int retryAttempts, Action retryCallback = null, params Type[] exceptionTypes)
    {
        do
        {
            try {
                action();
                break;
            }
            catch (Exception ex)
            {
                   if ((exceptionTypes?.Length > 0 && exceptionTypes.Contains(ex.GetType()))
                        || retryAttempts == 0)
                    throw;

                if (retryCallback != null)
                    retryCallback();
            }

            retryAttempts--;
        }
        while (retryAttempts >= 0);
    }

So, an example (although not a real-world one), may be:

ActionHelper.InvokeActionWithRetry(() => {
    Print(document);
},
5,
() => {
    RebootPrinter();
},
typeof(PrinterNeedsRestartException));

Is there anything that can be improved with the static method I've written?

I'm pretty new to using actions/funcs so I'm open to suggestions about how I can refactor this.

I also feel the type evaluation is a little 'hacky'.

EDIT:

I just noticed I'm not checking the types collection passed in prior to calling Contains on it. Naughty. I'll rework that bit and update the question.

share|improve this question
1  
My standard caution for this pattern is: it does not compose with itself. If the code to fetch a byte tries three times and then fails, and the code to fetch a string tries three times and then fails, and the code to fetch the header tries three times and then fails, and the code to fetch the document tries three times and then fails, and the code to fetch the repository tries three times and then fails then you just retried fetching that byte 243 times when the network cable was unplugged. – Eric Lippert Feb 17 at 17:54
    
Lest you think that is an unrealistic example, there are real-world cases of nested retries waiting days before producing the "network cable is unplugged" error message due to thousands of retries. This pattern particularly composes poorly with the "wait five seconds and retry" variation. – Eric Lippert Feb 17 at 17:56
    
Related: circuit breaker pattern – Mat's Mug Feb 17 at 18:47

Consider this example:

InvokeActionWithRetry(() => 
        { 
            Console.WriteLine("action"); 
            throw new Exception(); 
        }, 
    3);

It prints 'action' four times. That's not what I expect from the API (but is what I expected from reading the code). I'd expect the number I pass in to be the total number of invocations - not the number of retries after the first attempt.


You need to check this arguments for this method as there is a lot of scope to pass in the weird values. e.g. what if retryAttempts is a negative number? That doesn't make sense!

You also need to check action for null.


share|improve this answer
    
Nice RobH! Stuff I didn't think about it. It's evidently not defensive enough. – JᴀʏMᴇᴇ Feb 17 at 10:25

You should specify the exceptions to retry on instead of the exceptions to fail on. This will prevent you from retrying on unexpected/newly introduced exceptions.

Don't reuse parameters.

If you're using C# 6 you should use exception filtering instead of catch {...throw;}.

Depending on the amount of exceptions you plan to filter on it might be better to create a HashSet<T> from the params.

public static void InvokeActionWithRetry(Action action, int attempts, Action retryCallback = null, params Type[] exceptionTypes)
{
    if(action == null) throw new ArgumentNullException(nameof(action));
    if(attempts < 0) throw new ArgumentOutOfRangeException(nameof(attempts), nameof(attempts) + " must be positive");

    var exceptionFilter = (exceptionTypes?.Length > 0) 
        ? new HashSet<Type>(exceptionTypes) 
        : new HashSet<Type>();

    var lastAttempt = attempts - 1;
    for(int attempt = 0; attempt < attempts; attempt++)
    {
        try {
            action();
            break;
        }
        catch (Exception ex) when (exceptionFilter.Contains(ex.GetType()) && attempt != lastAttempt)
        {
            if (retryCallback != null)
                retryCallback();
        }
    }
}
share|improve this answer
1  
Nice! I'd simplify to var exceptionFilter = new HashSet<Type>(exceptionTypes ?? Enumerable.Empty<Type>()) or similar. – RobH Feb 17 at 15:13
    
You should probably also throw an Exception if attempts == 0 because otherwise the function does nothing, which is probably silent unexpected behaviour (e.g. someone interprets the number as number of retries) – Falco Feb 17 at 16:08

1. Functionality extension - besides already mentioned answers, I would extend your functionality with a back-off mechanism which can prove useful in some situations (e.g. a SQL deadlock). Since the options are quite a few now, these can be contained into a special class:

    public class ActionRetryOptions
    {
        // general data
        public Action Action { get; set; }
        public uint AttemptCount { get; set; } = 3;

        // retry data
        public Action RetryCallback { get; set; }

        // backoff mechanism
        public bool UseBackoff { get; set; } = false;
        public TimeSpan BackoffInitialInterval { get; set; } = TimeSpan.FromSeconds(1);
        public int BackoffFactor { get; set; } = 1;

        // fail fast
        public IList<Type> FailFastExceptionTypes { get; set; }
    }

I have also added some defaults to make it easier to use for the caller.

2. Changed function to support back-off

[edit] Changed BackoffInterval from int to TimeSpan as suggested by RobH

    public static void InvokeActionWithRetry(ActionRetryOptions retryOptions)
    {
        if (retryOptions == null)
            throw new ArgumentNullException(nameof(retryOptions));

        if (retryOptions.Action == null)
            throw new ArgumentNullException(nameof(retryOptions.Action));

        if (retryOptions.BackoffFactor < 1)
            throw new ArgumentException("BackoffInterval must greater or equal to 1");

        // backoff initialization
        int backOffTime = (int) retryOptions.BackoffInitialInterval.TotalMilliseconds;
        var random = new Random();

        for (uint attempt = retryOptions.AttemptCount; attempt > 0; attempt --)
        {
            try
            {
                retryOptions.Action();
                break;
            }
            catch (Exception ex)
            {
                if ((retryOptions.FailFastExceptionTypes?.Count > 0 && retryOptions.FailFastExceptionTypes.Contains(ex.GetType()))
                     || attempt <= 1)
                    throw;

                // back-off
                if (retryOptions.UseBackoff)
                {
                    int sleepTime = (int)(backOffTime * random.NextDouble());
                    Thread.Sleep(sleepTime);
                    backOffTime *= retryOptions.BackoffFactor;
                }

                if (retryOptions.RetryCallback != null)
                    retryOptions.RetryCallback();
            }
        }
    }

I have considered a simple back-off where I start with a certain period and multiply by a factor.

3. Simple test

   static void Main(string[] args)
   {
        try
        {
            var retryOptions = new ActionRetryOptions()
            {
                Action = () =>
                    {
                        Console.WriteLine("Action with error");
                        throw new Exception();
                    },
                UseBackoff = true,
                BackoffFactor = 2,
                AttemptCount = 5,
                RetryCallback = () =>
                {
                    Console.WriteLine("Retry callback");
                },
                FailFastExceptionTypes = new List<Type>() { typeof(SqlException) }
            };

            InvokeActionWithRetry(retryOptions);
        }
        catch (Exception exc)
        {
            Console.WriteLine("Unhandled exception - " + exc.ToString());
        }
        Console.ReadLine();
    }
share|improve this answer
    
I think BackoffInitialInterval should be a TimeSpan. – RobH Feb 17 at 15:15
    
@RobH - yes, it would be more flexible and make more sense. Thanks. – Alexei Feb 17 at 15:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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