4
\$\begingroup\$

Let’s say we copy some file using retry strategy (it might be blocked, etc.):

class Processor
{
    public void CopyData() =>
        CopyData(Try.Slow);

    public void CopyData(Try loop) =>
        loop.Execute(() => 
            File.Copy(@"c\a.txt", @"c:\b.txt"));
}

What do you think about names chosen for the following library code identifiers? Would you name them differently?

public abstract class Try
{
    public static Try Repeat(params int[] delays) => new Repeat(delays);
    public static readonly Try Never = Repeat();
    public static readonly Try Once = Repeat(0);
    public static readonly Try Slow = Repeat(0, 500, 1500, 4500, 12000);
    public static readonly Try Fast = Repeat(0, 50, 150, 450, 1200);
    public abstract void Execute(Action action);
}

class Repeat : Try
{
    IReadOnlyList<int> Delays { get; }

    public Repeat(params int[] delays)
    {
        Delays = delays;
    }

    public override void Execute(Action action)
    {
        for(int i=0; i< Delays.Count; i++)
            try
            {
                Thread.Sleep(Delays[i]);
                action();
                return;
            }
            catch
            {
                if (i == Delays.Count - 1)
                    throw;
            }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Somewhat related, the little-known circuit breaker pattern. Martin Fowler has a great article about it here. \$\endgroup\$ – Mathieu Guindon Sep 1 '16 at 3:20
  • \$\begingroup\$ @Mat'sMug Yep, it is in my answer below :) \$\endgroup\$ – Dmitry Nogin Sep 1 '16 at 3:22
  • 2
    \$\begingroup\$ My standard caution whenever someone posts about this pattern: it composes poorly with itself, so be cautious. \$\endgroup\$ – Eric Lippert Sep 1 '16 at 15:27
  • 2
    \$\begingroup\$ My standard caution whenever someone uses Thread.Sleep: workers are expensive; don't pay them to sleep. Schedule the task to be resumed asynchronously, rather than putting the thread to sleep and blocking all progress on this thread. Your code is a disaster waiting to happen on the UI thread. \$\endgroup\$ – Eric Lippert Sep 1 '16 at 15:28
  • \$\begingroup\$ @EricLippert Thanks, I would probably need two methods: Execute (event based with optional cancellation) and ExecuteAsync version (Task.Delay with optional cancellation). We are still need to be synchronous sometimes. \$\endgroup\$ – Dmitry Nogin Sep 1 '16 at 16:53
3
\$\begingroup\$

I think this would be more useful if the user could specify an interval and how many times he wants to retry like:

public static Try Repeat(int dalay, int count) 
    => new Repeat(Enumerable.Repeat(delay, count));

or if he could specify a count and the increment function:

public static Try Repeat(int count, Func<int, int> increment) 
    => new Repeat(Enumerable.Range(1, count + 1).Select(x => increment(x));

where the increment could be:

x => x * 20

I wouldn't provide such members as Fast or Slow because they are very subjective and what for you currently is slow might be in my application still too fast.

Never does not make any sense ;-) Why should I want to never try to execute something? I might as well not write the code at all if it shouldn't run :-P


One more thoght. How about specifying the retry strategy via a generic argument:

class SlowTry : Repeat
{
    public SlowTry() : base(0, 500, 1500, 4500, 12000) { }
}

the Try becomes this:

public abstract class Try
{
    ... stays the same

    public static void Execute<TStrategy>(Action action) where TStrategy : Try, new()
    {
        new TStrategy().Execute(action);
    }
}

use:

Try.Execute<SlowTry>(() => File.Copy(@"c\a.txt", @"c:\b.txt"));

This way the user can easier specify his strategy.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ 1) Yep, Repeat looks like a missing one. 2) Fast and Slow save a lot of typing, I could probably define them somewhere else in the app - like to IORetry.Slow. 3) Never is just a NULL object pattern; it allows deciding at run-time - let's say - it indicates how many times a LogWriter will try to access the DB for this exactly operation. \$\endgroup\$ – Dmitry Nogin Sep 1 '16 at 4:48
  • \$\begingroup\$ Oops ;-) you're right of course about the null pattern, I do it myself all the time. I don't know what I was thinking while writing this... But the fact is, I was awake just for ten minutes at that time. Forgive me ;-D \$\endgroup\$ – t3chb0t Sep 1 '16 at 5:17
  • \$\begingroup\$ @DmitryNogin see the edit. I've added one more idea. \$\endgroup\$ – t3chb0t Sep 1 '16 at 14:42
  • \$\begingroup\$ Actually we can do it without using generics: class IOTry { Try Slow = Try.After(0,1000,2000); }’ and then IOTry.Slow.Execute(() => {})`. It is a way cheaper and simpler to pass an instance instead of type parameter around. \$\endgroup\$ – Dmitry Nogin Sep 1 '16 at 17:01
2
\$\begingroup\$

I think you need to implement a cancellation mechanism. In worst case scenario you will have to wait for almost 20 seconds for CopyData() to return with no way to interrupt this operation, except for maybe aborting the thread (which is hardly an option). This might not be as important for server-side application, but it is extremely important for clients, who should always have the ability to close the application or cancel pending operation. In general, I think using Thread.Sleep is almost always a poor design decision, and it can always be replaced with a wait handle of choice.

Edit: here is a simple example of cancellation, which uses tasks:

public LoopHandle Execute(Action action)
{
    return Execute(ct => action());
}

public LoopHandle Execute(Action<CancellationToken> action)
{
    //You might want to properly dispose CancellationTokenSource later on. 
    //Or you can let GC call its finalizer later. Same goes for tasks.
    var cts = new CancellationTokenSource();
    var token = cts.Token;
    var task = Task.Factory.StartNew(() =>
    {
        for (int i = 0; i < Delays.Count; i++)
        {
            try
            {
                if (token.WaitHandle.WaitOne(Delays[i])) return;
                action(token);
                return;
            }
            catch
            {
                if (i == Delays.Count - 1) throw;
            }
        }
    }, token);
    return new LoopHandle(task, cts);
}

public class LoopHandle
{
    public LoopHandle(Task task, CancellationTokenSource tokenSource)
    {
        _task = task;
        _tokenSource = tokenSource;
    }

    //or you can just expose the task itself
    public void Wait()
    {
        //this will throw if Execute threw
        _task.Wait();
    }

    public void Cancel()
    {
        _tokenSource.Cancel();
    }

    private readonly Task _task;
    private readonly CancellationTokenSource _tokenSource;
}

You can still run it synchronously by calling loop.Execute(...).Wait(). I think this approach is more universal, if the goal is to create general retry strategy, but you can do the same thing without tasks, if you already have the background thread which executes this long-running method.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Have you an idea for the cancellation? I'm not very good at this (I mean cancelling a task that waits for something). \$\endgroup\$ – t3chb0t Sep 1 '16 at 15:16
  • \$\begingroup\$ @t3chb0t, I've added example to my answer. \$\endgroup\$ – Nikita B Sep 2 '16 at 11:13
1
\$\begingroup\$

I think it could be:

public abstract class Try
{
    public static Try After(params int[] delays) => new Repeat(delays);
    public static readonly Try Never = After();
    public static readonly Try Once = After(0);
    public static readonly Try Slow = After(0, 500, 1500, 4500, 12000);
    public static readonly Try Fast = After(0, 50, 150, 450, 1200);
    public static readonly Try InASecond = After(1000);
    public static readonly Try InAMinute = After(60000);
    public abstract void Execute(Action action);

    public Try FailFast() => FailFast(0);
    public Try FailFast(int timeout) => new Breaker(this, timeout);
}

where delays are provided by:

class Repeat : Try
{
    IReadOnlyList<int> Delays { get; }

    public Repeat(params int[] delays)
    {
        Delays = delays;
    }

    public override void Execute(Action action)
    {
        for(int i=0; i< Delays.Count; i++)
            try
            {
                Thread.Sleep(Delays[i]);
                action();
                return;
            }
            catch
            {
                if (i == Delays.Count - 1)
                    throw;
            }
    }
}

and circuit breaker pattern is implemented as:

class Breaker : Try
{
    Try Loop { get; }
    TimeSpan Timeout { get; }
    DateTime Ready { get; set; }

    public Breaker(Try loop, int timeout)
        : this(loop, TimeSpan.FromMilliseconds(timeout))
    {
    }

    public Breaker(Try loop, TimeSpan timeout)
    {
        Loop = loop;
        Timeout = timeout;
        Ready = DateTime.Now;
    }

    public override void Execute(Action action)
    {
        if (Ready > DateTime.Now)
            throw new OperationCanceledException();

        try
        {
            Loop.Execute(action);
        }
        catch
        {
            if (Timeout == TimeSpan.Zero)
                Ready = DateTime.MaxValue;
            else
                Ready = DateTime.Now + Timeout;

            throw;
        }   
    }
}
| improve this answer | |
\$\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.