Defensive opening of a database connection with option to retry

Question

Occasionally I am getting a SqlException exception on the line marked below. It occurs when we have network problems and the server cannot be found.

public void markComplete(int aTKey)
{
  SqlCommand mySqlCommand = null;
  SqlConnection myConnect = null;
  mySqlCommand = new SqlCommand();
  try
  {
    myConnect = new SqlConnection(ConfigurationManager.ConnectionStrings["foo"].ConnectionString);
    mySqlCommand.Connection = myConnect;
    mySqlCommand.Connection.Open();        //<<<<<<<< EXCEPTION HERE <<<<<<<<<<<<<<<<<<
    mySqlCommand.CommandType = CommandType.Text;
    mySqlCommand.CommandText =
       " UPDATE dbo.tb_bar " +
       " SET    LastUpdateTime    = CONVERT(Time,GETDATE()) " +
       " WHERE  AKey = " + aTKey;
    mySqlCommand.ExecuteNonQuery();
    mySqlCommand.Connection.Close();
  }
  finally
  {
    if(mySqlCommand != null)
    {
      mySqlCommand.Dispose();
    }
  }
}

I have two questions so maybe should split into 2 SO questions:

1. Is my finally statement sufficiently defensive ?
2. Rather than just failing how involved would it be to amend the method so instead of crashing it waits for say 10 minutes and then tries again to open the connection - tries a maximum of 3 times and if still no valid connection it moves on?

Answer 1

1. Use parameters. Do not concatenate strings to create SQL statements. Read about SQL Injection.

2. Instead of using try...finally you can simplify your code with the using statement. You need to dispose all the instances that implements the IDisposable interface, so you also need to use the using statement with the SqlConnection. In fact, it's even more important then disposing the SqlCommand, since it allows ADO.Net to use connection pooling.

3. You don't need to do the entire procedure again and again, just the connection.Open and the ExecuteNonQuery.

4. Using the constructor that accepts a string an SqlConnection saves you the need to set them via properties.

5. You don't need to specify CommandType.Text - it's the default value.

Here is a basic implementation of retry logic with some improvements to your code:

public void markComplete(int aTKey)
{
    var sql = " UPDATE dbo.tb_bar " +
               " SET    LastUpdateTime    = CONVERT(Time,GETDATE()) " +
               " WHERE  AKey = @aTKey";

    using(var myConnect = new SqlConnection(ConfigurationManager.ConnectionStrings["foo"].ConnectionString))
    {
        using(var mySqlCommand = new SqlCommand(sql, myConnect))
        {
            mySqlCommand.Parameters.Add("@aTKey", SqlDbType.Int).Value = aTKey;

            var success = false;
            var attempts = 0;
            do
            {
                attempts++;
                try
                {
                    mySqlCommand.Connection.Open();  
                    mySqlCommand.ExecuteNonQuery();
                    success = true;
                }
                catch(Exception ex)
                {
                    // Log exception here
                    Threading.Thread.Sleep(1000);
                }
            }while(attempts < 3 || !success);
        }   
    }   
}

Update:

Well, I've had some free time and I remember writing a general retry method a few years back. Couldn't find it but here is the general idea:

static void RetryOnException(Action action, int numberOfRetries, int timeoutBetweenRetries)
{
    var success = false;
    var exceptions = new List<Exception>();
    var currentAttempt = 0;
    do
    {
        currentAttempt++;

        try
        {
            action();
            success = true;
        }
        catch(Exception ex)
        {
            exceptions.Add(ex);
            Threading.Thread.Sleep(timeoutBetweenRetries);
        }
    } while(!success || currentAttempt < numberOfRetries);

    // Note: The Exception will only be thrown in case all retries fails.
    // If the action completes without throwing an exception at any point, all exceptions before will be swallowed by this method. You might want to log them for future analysis.
    if(!success && exceptions.Count > 0)
    {
        throw new AggregateException("Failed all {numberOfRetries} retries.", exceptions);
    }
}

Using this method you can retry all sort of things, while keeping your methods simpler and cleaner.
So here is how it should be used:

public void markComplete(int aTKey)
{
    var sql = " UPDATE dbo.tb_bar " +
               " SET    LastUpdateTime    = CONVERT(Time,GETDATE()) " +
               " WHERE  AKey = @aTKey";

    using(var myConnect = new SqlConnection(ConfigurationManager.ConnectionStrings["foo"].ConnectionString))
    {
        using(var mySqlCommand = new SqlCommand(sql, myConnect))
        {
            mySqlCommand.Parameters.Add("@aTKey", SqlDbType.Int).Value = aTKey;
            // You can do this inside a `try...catch` block or let the AggregateException propagate to the calling method
            RetryOnException(
                () => {
                    mySqlCommand.Connection.Open();  
                    mySqlCommand.ExecuteNonQuery();
                }, 3, 1000);
        }   
    }   
}