CODEWITHSUNDEEP
c#exception
franklores
franklores

Reputation: 375

Is this good practice?

I have this extension method that allows me to retry an operation if there is an exception, a typical use is trying to write to a file, but for some reason I can't so I retry a bit later...

The extension looks like:

public static void WithRetry<T>(this Action action, int timeToWait = 500, int timesToRetry = 3) where T : Exception
{
    int retryCount = 0;

    bool successful = false;

    do
    {
        try
        {
            action();
            successful = true;
        }
        catch (T)
        {
            retryCount++;
            Thread.Sleep(timeToWait);
            if (retryCount == timesToRetry) throw;
        }
        catch (Exception)
        {
            throw;
        }
    } while (retryCount < timesToRetry && !successful);
}

Visual studio tells me that I'm swallowing an exception in the first catch block, is this bad?

Thanks.

Upvotes: 1

Views: 201

Answers (2)

nvoigt
nvoigt

Reputation: 77285

The warning is correct, you swallow exceptions. If you retry 10 times you will never know what went wrong the first 9 times, you only get exception number 10.

Maybe that's what you want. Personally, I would put all the occurring exceptions into an AggregateException and throw that when you hit your retry count.

Maybe like this:

    public static void WithRetry<T>(this Action action, int timeToWait = 500, int timesToRetry = 3) where T : Exception
    {
        var exceptions = new List<Exception>();

        for (int tryIndex = 0; tryIndex < timesToRetry; tryIndex++)
        {
            try
            {
                action();

                return;
            }
            catch (T t)
            {
                exceptions.Add(t);
            }

            Thread.Sleep(timeToWait);
        }

        throw new AggregateException(exceptions);
    }

Upvotes: 2

Carbine
Carbine

Reputation: 7903

The warning is exactly what you are trying to achieve. You are swallowing the exceptions (timesToRetry-1) times. On the last try only you are actually throwing the exception. Until then all the exceptions will be swallowed and lost. Since this is the behavior you are trying to achieve. There is no harm in suppressing the message.

But as @HimBromBeere stated remove the catch(Exception) block. Also you can try logging the exception on each re-try because you will loose this data. What if different kind of exception is thrown each time. There is no way to be sure.

Upvotes: 2

Related Questions