18
\$\begingroup\$

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).

\$\endgroup\$
  • 4
    \$\begingroup\$ Personally, I don't think that while (true) is a code smell. \$\endgroup\$ – svick Sep 10 '13 at 17:19
  • \$\begingroup\$ 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/… \$\endgroup\$ – Hardrada Sep 10 '13 at 23:41
  • \$\begingroup\$ Might be overkill, but... ever heard of the circuit breaker pattern? timross.wordpress.com/2008/02/10/… \$\endgroup\$ – Mathieu Guindon Sep 11 '13 at 0:28
  • \$\begingroup\$ 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. \$\endgroup\$ – Kyle Dec 6 '13 at 7:43
12
\$\begingroup\$

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
      }
   }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Change the in-lined 3 to a constant or a configurable parameter and this would be a great solution. \$\endgroup\$ – unholysampler Sep 10 '13 at 18:24
  • \$\begingroup\$ 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. \$\endgroup\$ – Max Dec 6 '13 at 10:26
  • \$\begingroup\$ @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... \$\endgroup\$ – Powerslave Nov 11 '15 at 15:23
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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     
\$\endgroup\$
1
\$\begingroup\$

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
    }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.