How to reuse code that reopens connection?

Question

Our production server kills inactive connections, so our API needs to restore them when they are needed. The following code works, but it is very repetitive:

   private const int MaxRetryCount = 3;

    public static SqlDataReader RestoreConnectionAndExecuteReader(SqlCommand command)
    {
        int retryCount = 0;

        while (retryCount++ < MaxRetryCount)
        {
            try
            {
                if (command.Connection.State == ConnectionState.Closed)
                    command.Connection.Open();
                return command.ExecuteReader();
            }
            catch(Exception e)
            {
                if(!e.Message.ToLower().Contains("transport-level error has occurred"))
                {
                    throw;
                }
            }
        }
        throw new Exception("Failed to restore connection for command:"+command.CommandText);
    }

    public static void RestoreConnectionAndExecuteNonQuery(SqlCommand command)
    {
        var retryCount = 0;
        while(retryCount++ < MaxRetryCount)
        {
            try
            {
                if (command.Connection.State == ConnectionState.Closed)
                    command.Connection.Open();
                command.ExecuteNonQuery();
                return;
            }
            catch(Exception e)
            {
                if (!e.Message.ToLower().Contains("transport-level error has occurred"))
                {
                    throw;
                }
            }
        }
        throw new Exception("Failed to restore connection for command:" + command.CommandText);
    }

How could I refactor my code and eliminate duplication? I need to keep the signatures of these methods, as they are used all over the system.

Have you thought about Extracting the Methods to an Interface..? also there is no duplication so to speak one method is Void while the other returns an SQLReader you could probably eliminate this by using one method and using out params just a suggestion — MethodMan, Feb 02 '12 at 21:59
@DJKRAZE: yes, I did. However, the first method returns SqlDataReader, while the second is void. — Arne Lund, Feb 02 '12 at 22:01
you could split out duplicate logic into another method which you call from all of your existing methods and return a bool or the SqlCommand object itself (reconnected) then run your operations on it...NonQuery, ExecuteReader etc — Christopher Johnson, Feb 02 '12 at 22:03
I'd be interested in knowing why and when do you need to call these methods since normally the [ADO.NET Connection-Pool](http://msdn.microsoft.com/en-us/library/8xx3tyca.aspx) controls the `ConnectionState` internally. So even if it's closed it might be reusable and a `Connection.Open` does not necessarily mean that there's an overhead. It's only a flag for the pool. In general it's not a good idea to keep a connection open. — Tim Schmelter, Feb 02 '12 at 22:06
@ChristopherJohnson is Correct.. I was also thinking he could extract to an Interface as well what are your thoughts on that @Christopher..? — MethodMan, Feb 02 '12 at 22:07
@TimSchmelter well, I do not know a simpler approach that works. I am testing as follows: I am debugging, then I kill the connection in SSMS, and the code is supposed to restore and succeed. — Arne Lund, Feb 02 '12 at 22:13
@Arne: I'm not sure if you got me. In my opinion you combat symptoms instead of the core problem. Why aren't you closing the connections when you've used them? You're telling the connection-pool that this connection cannot be used because you need it immediately. This causes unused connections and even might result in an exception, but it definitely decreases performance. http://stackoverflow.com/a/670842/284240 — Tim Schmelter, Feb 02 '12 at 22:25
@TimSchmelter I hear what you are saying. To implement what you are suggesting, I'd need some more refactoring. — Arne Lund, Feb 02 '12 at 22:32
@Arne: I cannot show you code because these methods are redundant. You should not use a global,custom Database-Management class because that's a good source for nasty errors. Create, open, use and close a connection and all related objects(SqlCommand,SqlDataAdapter,etc.) in your DAL. Have also a look at the [using statement](http://msdn.microsoft.com/en-us/library/yh598w02%28v=vs.100%29.aspx) which automatically disposes objects(connections will be closed implicitely). — Tim Schmelter, Feb 02 '12 at 22:38
@TimSchmelter: I cannot get your suggestion to work, and posted another question: http://stackoverflow.com/questions/9133473/why-is-my-using-statement-not-closing-connection — Arne Lund, Feb 03 '12 at 18:19

score 5 · Accepted Answer · answered Feb 02 '12 at 22:07

5

private const int MaxRetryCount = 3;

public static T RestoreConnectionAndExecute<T>(SqlCommand command, Func<SqlCommand, T> func)
{
    int retryCount = 0;

    while (retryCount++ < MaxRetryCount)
    {
        try
        {
            if (command.Connection.State == ConnectionState.Closed)
                command.Connection.Open();
            return func(command);
        }
        catch(Exception e)
        {
            if(!e.Message.ToLower().Contains("transport-level error has occurred"))
            {
                throw;
            }
        }
    }
    throw new Exception("Failed to restore connection for command:"+command.CommandText);

} 

public static SqlDataReader RestoreConnectionAndExecuteReader(SqlCommand command)
{
    return RestoreConnectionAndExecute(command, c => c.ExecuteReader());
}

public static int RestoreConnectionAndExecuteNonQuery(SqlCommand command)
{
    return RestoreConnectionAndExecute(command, c => c.ExecuteNonQuery());
}

answered Feb 02 '12 at 22:07

Roy Goode

2,940
20
22

bleh...beat me to it :( This is basically what I was referring to in my comment. – Christopher Johnson Feb 02 '12 at 22:08
technically you change the signature of the non-query method. It shouldn't break code except in exceptional cases, but it's still worth mentioning. – Servy Feb 02 '12 at 22:10
Don't you have type mismatch in the `RestoreConnectionAndExecuteNonQuery` ? – Damian Leszczyński - Vash Feb 02 '12 at 22:11
SqlCommand.ExecuteNonQuery() returns an int, so yes I changed the return type from void. Should not affect existing code. If you don't need the int, the method can be changed to a void and the 'return' keyword removed. – Roy Goode Feb 02 '12 at 22:13

score 1 · Answer 2 · answered Feb 02 '12 at 22:08

private const int MaxRetryCount = 3;

        public static SqlDataReader RestoreConnectionAndExecuteReader(SqlCommand command)
        {
            return RestoreConnectionAndExecuteQueryHelper(command, true);
        }

        public static void RestoreConnectionAndExecuteNonQuery(SqlCommand command)
        {
            RestoreConnectionAndExecuteQueryHelper(command, false);
        }

        private static SqlDataReader RestoreConnectionAndExecuteQueryHelper(SqlCommand command, bool returnReader)
        {
            var retryCount = 0;
            while (retryCount++ < MaxRetryCount)
            {
                try
                {
                    if (command.Connection.State == ConnectionState.Closed)
                        command.Connection.Open();
                    if (returnReader)
                    {
                        return command.ExecuteReader();
                    }
                    else
                    {
                        command.ExecuteNonQuery();
                        return null;
                    }
                }
                catch (Exception e)
                {
                    if (!e.Message.ToLower().Contains("transport-level error has occurred"))
                    {
                        throw;
                    }
                }
            }
            throw new Exception("Failed to restore connection for command:" + command.CommandText);
        }

score 0 · Answer 3 · answered Feb 02 '12 at 22:07

The common part for those method is the connection retrieving process. You could create a new static method that is responsible for that called for example retrieve connection that take command as argument and return it if connection is established in other cases throws an exception.

score 0 · Answer 4 · answered Feb 02 '12 at 22:11

There's a few things I'm not loving in the code sample. But, to explicitly answer your question on removing duplication - extract out the common code to a method that takes a delegate.

private TReturn RestoreConnectionAndExecute<T>(SqlCommand command, Func<SqlCommand, TReturn> execute) 
{
   int retryCount = 0;
   while (retryCount++ < MaxRetryCount)
   {
       try
       {
          if (command.Connection.State == ConnectionState.Close) 
              command.Connection.Open();
          return execute(command);
       } 
       catch(Exception e)
       {
          ...
   }
}

public SqlDataReader RestoreConnectionAndExecuteReader(SqlCommand command) 
{
   return this.RestoreConnectionAndExecute(command, c => c.ExecuteReader());
}

public void RestoreConnectionAndExecuteNonQuery(SqlCommand command)
{
   // Ignore return
   this.RestoreConnectionAndExecute(command, c => c.ExecuteNonQuery());
}

You should really rethink a few things, though. Including:

Catching specific exceptions
Using Exception.Number or ErrorCode instead of the message (which will change in localized versions, and possibly in updated FX versions)
Using statements for IDisposable resources
Throwing specific exceptions

In your version of the answer, the method should be RestoreConnectionAndExecute not RestoreConnectionAndExecute — Roy Goode, Feb 02 '12 at 22:16
+1 I switched to Exception.Number. However, using statements did not fix my problem. — Arne Lund, Feb 03 '12 at 21:00

How to reuse code that reopens connection?

4 Answers4

Linked

Related