Attempt to reconnect, with timeout

Question

I have a situation in which a program, communicating over the network, can timeout or otherwise fail to establish a connection through no fault of my application or its user. If and when this happens, an exception is thrown. The solution is obvious; simply try again a couple more times, and if multiple tries don't work then this is really a problem.

I've accomplished this by wrapping the original try-catch in a while loop with a retry counter, producing the following basic pattern:

var retryCount = 0;
while(true)
{
 try
 {
 AttemptToConnect();
 break;
 }
 catch(TimeoutException tex)
 {
 if(++retryCount < 3) continue;
 throw; //or handle error and break/return
 }
}

Do any of you see any problems? The biggest smell, prompting me to ask in the first place, is the conditionless loop, which would be infinite except that the "happy path" breaks out of the loop manually, while the terminating error will throw out. This has been called out before as a potential bug generator, if it's ever modified in the future (for instance to catch a second type of exception, or to do something else that could throw an exception).

• svick: Personally, I don't think that while (true) is a code smell.
• Hardrada: I don't know if this is relevant to your situation, but I wrote something along these lines which you can find here: codereview.stackexchange.com/questions/19868/…
• Mathieu Guindon: Might be overkill, but... ever heard of the circuit breaker pattern? timross.wordpress.com/2008/02/10/…
• Kyle: Perhaps consider resetting the retry count if you successfully connect, otherwise you may disconnect after separate connect failures followed by successes. Unless of course this is the intended behaviour.

Answer 1

Yeah, I'd probably redo it with an actual condition. That way, intent looks clear and is not dependent on internal code to break, continue, etc.

const int NumberOfRetries = 3;
var retryCount = NumberOfRetries;
var success = false;
while(!success && retryCount > 0)
{
 try
 {
 AttemptToConnect();
 success = true;
 }
 catch(TimeoutException tex)
 {
 retryCount--;
 if (retryCount == 0)
 {
 throw; //or handle error and break/return
 }
 }
}

• unholysampler: Change the in-lined 3 to a constant or a configurable parameter and this would be a great solution.
• Max: This could also be extended by, say that AttemptToConnect() returns different int-success values, you could handle different cases in your try depending on what you want to do.
• Powerslave: @Max I don't think one should return such values, as failure cases are what exceptions are for. Instead, AttemptToConnect() should probably return a Connection instance and the success flag should be dropped in favor of connection == null, which would explain straight away that we keep trying to connect until we get an actual connection or run out of retries. Also that if (retryCount == 0) ... thingy could be improved, I believe...

Answer 2

I actually like the author's code. It's short and clear.

My only (minor) comment would be the if statement...

 if(++retryCount < 3) continue;
 throw; //or handle error and break/return

I would do;

 if(++retryCount > 2)
 throw; //or handle error and break/return

But that's only because I think it reads a little better.

Answer 3

With minor changes I would refactor it as follows:

var retriesLeft = 3;
var connectionEstablished=false;
while(retriesLeft>0)
{
 try
 {
 AttemptToConnect();
 connectionEstablished=true;
 break;
 }
 catch(TimeoutException tex)
 {
 retriesLeft-=1;
 }
}
if (connectionEstablished==false) throw new TimeoutException(); // or whatever
// Do stuff 

Answer 4

I would like to add few changes to the existing. When the attempt to connection fails, it is always better to retry the attempt after some delay.

1. Declare the _retryCounter and _connectionRetryInterval outside scope of the class, and make it configurable. For simplicity I am assigning values directly.

   private int _connectionRetryInterval = 3000;
   private int _maxRetryCount = 3;
   

2. Modified code is as below

   var retryCount = 0;
   var connectionEstablished = false;
   while (retryCount < _maxRetryCount)
   {
    try
    {
    AttemptToConnect();
    connectionEstablished = true;
    break;
    }
    catch (TimeoutException tex)
    {
    // log the exception with retry count.
    System.Threading.Thread.Sleep(_connectionRetryInterval);
    retryCount += 1;
    }
   }
   if (connectionEstablished == false)
   {
    throw new TimeoutException(); // or whatever
   }