What is good C# coding style for catching SQLException and retrying

Question

I have a method that calls a SQLServer function to perform a free text search against a table. That function will occasionally on the first call result in a SQLException: "Word breaking timed out for the full-text query string". So typically I want to retry that request because it will succeed on subsequent requests. What is good style for structuring the retry logic. At the moment I have the following:

var retryCount = 0;
var results = new List<UserSummaryDto>();
using (var ctx = new UsersDataContext(ConfigurationManager.ConnectionStrings[CONNECTION_STRING_KEY].ConnectionString))
{
    for (; ; )
    {
        try
        {
            results = ctx.SearchPhoneList(value, maxRows)
                         .Select(user => user.ToDto())
                         .ToList();
            break;
        }
        catch (SqlException)
        {
            retryCount++;
            if (retryCount > MAX_RETRY) throw;
        }
    }
}

return results;

Answer 1

I'd change the exception handling to only retry on certain errors:

• 1204, 1205 deadlocks
• -2 timeout
• -1 connection broken

These are the basic "retryable" errors

catch (SqlException ex)
{
    if !(ex.Number == 1205 || ex.Number == 1204 || ... )
    {
        throw
    }
    retryCount++;
    if (retryCount > MAX_RETRY) throw;
}

Edit, I clean forgot about waits so you don't hammer the SQL box:

• Add a 500 ms wait on deadlock
• Add a 5 sec delay on timeout

Edit 2:

I'm a Developer DBA, don't do much C#. My answer was to correct exception processing for the calls...

Answer 2

Thanks for all the feedback. I'm answering this myself so I can incorporate elements from the answers given. Please let me know if I've missed something. My method becomes:

var results = new List<UserSummaryDto>();
Retry<UsersDataContext>(ctx => results = ctx.SearchPhoneList(value, maxRows)
                                            .Select(user => user.ToDto())
                                            .ToList());
return results;

And I've refactored the original method for reuse. Still lots of levels of nesting. It also relies on there being a default constructor for the data context which may be too restrictive. @Martin, I considered including your PreserveStackTrace method but in this case I don't think it really adds enough value - good to know for future reference thanks:

private const int MAX_RETRY = 2;
private const double LONG_WAIT_SECONDS = 5;
private const double SHORT_WAIT_SECONDS = 0.5;
private static readonly TimeSpan longWait = TimeSpan.FromSeconds(LONG_WAIT_SECONDS);
private static readonly TimeSpan shortWait = TimeSpan.FromSeconds(SHORT_WAIT_SECONDS);
private enum RetryableSqlErrors
{
    Timeout = -2,
    NoLock = 1204,
    Deadlock = 1205,
    WordbreakerTimeout = 30053,
}

private void Retry<T>(Action<T> retryAction) where T : DataContext, new()
{
    var retryCount = 0;
    using (var ctx = new T())
    {
        for (;;)
        {
            try
            {
                retryAction(ctx);
                break;
            }
            catch (SqlException ex)
                when (ex.Number == (int) RetryableSqlErrors.Timeout &&
                      retryCount < MAX_RETRY)
            {
                Thread.Sleep(longWait);
            }
            catch (SqlException ex)
                when (Enum.IsDefined(typeof(RetryableSqlErrors), ex.Number) &&
                      retryCount < MAX_RETRY)
            {
                Thread.Sleep(shortWait);
            }
            retryCount++;
        }
    }
}

Answer 3

My enum of retryables for sql looks like this:

SqlConnectionBroken = -1,
SqlTimeout = -2,
SqlOutOfMemory = 701,
SqlOutOfLocks = 1204,
SqlDeadlockVictim = 1205,
SqlLockRequestTimeout = 1222,
SqlTimeoutWaitingForMemoryResource = 8645,
SqlLowMemoryCondition = 8651,
SqlWordbreakerTimeout = 30053

Answer 4

It's not good style, but sometimes you have to do it, because you simply can't change existing code and have to deal with it.

I am using the following generic method for this scenario. Note the PreserveStackTrace() method, which can sometimes be very helpful in a re-throw scenario.

public static void RetryBeforeThrow<T>(Action action, int retries, int timeout) where T : Exception
{
    if (action == null)
        throw new ArgumentNullException("action", string.Format("Argument '{0}' cannot be null.", "action"));

    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);
}

You would call it like that:

RetryBeforeThrow<SqlException>(() => MethodWhichFails(), 3, 100);

Answer 5

There is no good style for doing something like this. You'd be better off figuring out why the request fails the first time but succeeds the second time.

It seems possible that Sql Server has to initially compile an execution plan and then execute the query. So the first call fails because the combined times exceed your timeout property, and succeeds the second time because the execution plan is already compiled and saved.

I don't know how UsersDataContext works, but it may be the case that you have the option to Prepare the query before actually executing it.

Real Answer: If I had to do this, I would retry just once and not again, like this:

var results = new List<UserSummaryDto>();
using (var ctx = new 
    UsersDataContext(ConfigurationManager.ConnectionStrings[CONNECTION_STRING_KEY].ConnectionString))
{
        try
        {
            results = ctx.SearchPhoneList(value, maxRows)
                         .Select(user => user.ToDto())
                         .ToList();
            break;
        }
        catch (SqlException)
        {
            try
            {
                results = ctx.SearchPhoneList(value, maxRows)
                         .Select(user => user.ToDto())
                         .ToList();
                break;
            }
            catch (SqlException)
            {
                // set return value, or indicate failure to user however
            }
        }
    }
}

return results;

While I might trust you to not abuse the retry process, you'd be tempting your successor to increase the retry count as a quick fix.

Answer 6

I think annotating a method with an aspect specifying the retry count would result in more structured code, although it needs some infrastructure coding.

Answer 7

You can simply use SqlConnectionStringBuilder properties to sql connection retry.

var conBuilder = new SqlConnectionStringBuilder("Server=.;Database=xxxx;Trusted_Connection=True;MultipleActiveResultSets=true"); conBuilder.ConnectTimeout = 90; conBuilder.ConnectRetryInterval = 15; conBuilder.ConnectRetryCount = 6;

Note:- Required .Net 4.5 or later.

Answer 8

Pull the relevant code out into its own method, then use recursion.

Pseudo-code:

try
{
    doDatabaseCall();
}
catch (exception e)
{
    //Check exception object to confirm its the error you've been experiencing as opposed to the server being offline.
    doDatabaseCall();
}