Today I added a simple wait-then-retry mechanism to a piece of code that occasionally bites my current client. The existing code tried an action and just dumped out if an exception was thrown. In this case, some of the thrown exceptions weren’t fatal -- the command could be retried in a short while (it’s actually an attempt to update a database under transient load).
At first I implemented the wait-before-retry simply, with a deliberate trade-off between complexity and accuracy – as a series of Thread.Sleep steps, so that after each step the thread could yield to another. It looked a little like this:
class ActWithSimpleRetry { private int _allowedAttempts; private int _delay; private Action _customAction; public ActWithSimpleRetry(int allowedAttempts, int delay, Action customAction) { _allowedAttempts = allowedAttempts; _delay = delay; _customAction = customAction; } public void Run() { var attempt = 0; while(attempt++ < _allowedAttempts) { try { // _customAction should throw a CustomException to simulate // the behaviour I wanted to retry on. _customAction(); return; } catch (CustomException) { if (attempt == _allowedAttempts) throw; } YieldingWait(_delay); } } private void YieldingWait(int delay) { var elapsed = 0; var step = 500; delay = 1000 * delay; while(elapsed < delay) { System.Threading.Thread.Sleep(step); elapsed += step; } } }
There we go, professional job done. Looks like I’ve thought about it, it’s not just sat there blocking the thread for delay milliseconds. But hang on: do I even need to yield?
Constructing the wait like this is more complex than it need be: a single Thread.Sleep would do, as I’m not worried about freezing a UI; I’m not checking on “Cancelled” state; and I’m not waiting before synchronizing threads.
If I had been worried about synchronizing threads then this implementation would be clunky, leading me to poll on some shared state. Moreover, the waiting thread could always yield to another, and it might be a while before the waiting thread is able to execute its next wait step. The way I had implemented the wait – with no reference to actual time elapsed – is naive.
A simple Thread.Sleep would be a more honest solution to the problem.
What about situations where you do want to worry about timing accuracy and thread freezing – and coordinating with a cancellation signal on another thread? The alternative is to use a Timer running independently of the main thread. A first attempt might look like this:
public void Run() { var attempt = 0; var timerEvent = new EventWaitHandle(true, EventResetMode.AutoReset); var timer = new System.Timers.Timer(_delay * 1000); timer.Elapsed += (sender, args) => timerEvent.Set(); while (attempt++ < _allowedAttempts && timerEvent.WaitOne()) { try { _customAction(); return; } catch (CustomException) { if (attempt == _allowedAttempts) throw; } timer.Start(); } }
The Timer is created with the desired wait, and configured to signal a WaitHandle on expiry. The main thread then Waits on that handle (and checks the number of attempts) before trying again.
Here I’ve gained some control over the duration of the wait at the cost of some complexity. However – I’m still blocking on the main thread at timerEvent.WaitOne! If I want to entertain the possibility of a clean shutdown, I’ll need to also take a look at waiting on a cancellation signal. That would look something like this:
public void Run() { var attempt = 0; var timerEvent = new EventWaitHandle(true, EventResetMode.AutoReset); // in practice this would be a member field that could be Set // from a separate "cancellation" thread var cancelEvent = new EventWaitHandle(false, EventResetMode.ManualReset); var timer = new System.Timers.Timer(_delay * 1000); timer.Elapsed += (sender, args) => timerEvent.Set(); while (attempt++ < _allowedAttempts && WaitOnSignals(timerEvent, cancelEvent)) { try { _customAction(); return; } catch (CustomException) { if (attempt == _allowedAttempts) throw; } timer.Start(); } // handle the drop through here; probably a cancellation } private bool WaitOnSignals(WaitHandle timer, WaitHandle cancel) { var handleActions = new[] { new { Handle = cancel, Result = false }, new { Handle = timer, Result = true }, }; var handles = handleActions.Select(x => x.Handle).ToArray(); int signalled = WaitHandle.WaitAny(handles); return handleActions[signalled].Result; }
Which is better (and reminds me that Chris Oldwood is always reminding me to wait on two events, not one): it’ll break out of the processing if there’s a cancellation. However, it still blocks at WaitHandle.WaitAny. If, finally, I want to yield to other threads periodically then I’ll have to break out of the wait periodically:
private bool WaitOnSignals(WaitHandle timer, WaitHandle cancel) { var handleActions = new[] { new { Handle = cancel, Result = false }, new { Handle = timer, Result = true }, }; int signalled; var handles = handleActions.Select(x => x.Handle).ToArray(); while((signalled = WaitHandle.WaitAny(handles, 10)) == WaitHandle.WaitTimeout) { } return handleActions[signalled].Result; }
Finally, here I get actual elapsed time before the timer expires and allows work to continue, and can avoid blocking on the timer wait by using Waithandle.WaitAny to automatically handle both timer and cancellation signals. There is a risk that it won’t react to a cancellation or complete signal as soon as possible because it yields control after 10 milliseconds – but if you want to allow other threads a look-in you have to accept that they’ll take some processing time, and that on top of the time taken for context switches.
Bearing in mind the added complexity of this wait and the loose restrictions around thread freezing and cancellation for the original app – I’d be sorely tempted to go for a simple, honest Thread.Sleep instead.
