Method for Access to a Resource with 'Retry' Logic

Question

I am trying to implement so me 'retry' logic in a Data Access layer where the DB occassionaly times out (and the method is called several thousand times a day). This is the best I can come up with, but I don't like throwing the exception if the code falls out of my while loop; it doesn't feel like a succinct solution. Can it be improved?

NB: No need for multi-threaded access.

private static currentDbAttempt
private static Int32? GetDbValue()
 {
 currentDbAttempt = 1;
 while(currentDbAttempt <= MAX_DB_ATTEMPTS)
 {
 try
 {
 Int32? nextId = [some DB work]
 return nextId.Value;
 }
 catch (Exception ex)
 {
 if (currentDbAttempt == MAX_DB_ATTEMPTS)
 throw new Exception(string.Format("Error in [method name]. Tried {0} times",
 MAX_DB_ATTEMPTS), ex);
 _log.WarnFormat("Error in [method name]: {0}. Try count {1}", 
 ex.Message, currentDbAttempt++);
 }
 }
 // Code smell...
 throw new Exception("[method name] - shouldn't reach here.");
 }

Answer 1

I agree that having to throw that exception to appease the compiler is ugly. There's also another bit of unpleasantness which isn't quite so egregious, but helps point to what's actually wrong here:

Your while loop condition is

while(currentDbAttempt <= MAX_DB_ATTEMPTS)

But inside the same loop, you do

if (currentDbAttempt == MAX_DB_ATTEMPTS)

So what's happening, causing both these problems, is that you're trying to do something inside the loop which is actually meant to happen when the loop is finished. That should happen outside the loop. Fortunately in this case it's a simple fix- just move your failure exception outside the loop:

static Int32? GetDbValue()
 {
 currentDbAttempt = 1;
 Exception lastException;
 while(currentDbAttempt <= MAX_DB_ATTEMPTS)
 {
 try
 {
 Int32? nextId = [some DB work]
 return nextId.Value;
 }
 catch (Exception ex)
 {
 lastException = ex;
 _log.WarnFormat("Error in [method name]: {0}. Try count {1}", 
 ex.Message, currentDbAttempt++);
 }
 }
 throw new Exception(string.Format("Error in [method name]. Tried {0} times",
 MAX_DB_ATTEMPTS), lastException);
 }

---

Doing the above uncovered something else: you swallow the exceptions most of the time, but somewhat arbitrarily expose the last one as an inner exception if you fail. This does make sense on the assumption that the exceptions will always occur for the same reason (e.g. timeout), but you might consider using an AggregateException or similar to expose all of them.

---

Either the method should have an int return value, or you should be returning nextId instead of nextId.Value. Either you have a potential InvalidOperationException waiting to happen if nextId can truly not have a value, or you're needlessly giving your caller extra work having to do its own HasValue check.

---

You're doing a worrying amount with statics. Not only are you holding state in currentDbAttempt, you're also accessing what I can only assume is some dependency in _log. While you say multi-threading isn't an issue, over-reliance on statics also sacrifices your ability to take full advantage of OO features like polymorphism.

The currentDbAttempt variable is fixible easily enough by just moving it inside the method, where it should be anyway. But I think more generally you should make sure you're not making things static over-eagerly.

---

The way you increment currentDbAttempt is a good example of being "too clever". It's much less clear than if you just had an extra currentDbAttempt++ statement outside the WarnFormat line, and that would also be far less prone to accidentally introducing bugs if you later come back to maintain this method.

Even better, why not pick an idiomatic option and use a for loop?

• Ryan: Fantastic response, I agree with just about every point and will refactor. Thanks a lot for the advice.