2
\$\begingroup\$

I wanted to use the Try helper from @DmitryNogin's General Retry Strategy #2 but each attempt to implement it revealed another thing that is missing.

In my review I mentioned that it would be useful to be able to log the exceptions but later I came to the conclusion that perhaps it would be better to implement something similar to the ParallelLoopResult - this is how the TryResult was formed.

public struct TryResult
{
    public TryResult(bool isCompleted, IEnumerable<Exception> exceptions) : this()
    {
        IsCompleted = isCompleted;
        Exceptions = exceptions;
    }

    public bool IsCompleted { get; private set; }

    public IEnumerable<Exception> Exceptions { get; private set; }
}

Then I added it to the base class:

public abstract class Try
{
    public static readonly Try Never = new Never();
    public static readonly Try Once = Retry(delay: 0, times: 0, ratio: 0);

    public static Try Retry(int delay, int times, double ratio) =>
        RetryAfter(from i in Enumerable.Range(0, times)
                   select delay * Math.Pow(ratio, i) into d
                   select (int)d);

    public static Try RetryAfter(params int[] delays) => RetryAfter(delays.AsEnumerable());
    public static Try RetryAfter(IEnumerable<int> delays) => new Retry(delays);

    public void Execute(Action action) => Execute(action, CancellationToken.None);
    public abstract TryResult Execute(Action action, CancellationToken cancellationToken);

    public Task ExecuteAsync(Action action) => ExecuteAsync(action, CancellationToken.None);
    public Task ExecuteAsync(Action action, CancellationToken cancellationToken) =>
        ExecuteAsync(() => { action(); return Task.CompletedTask; }, cancellationToken);

    public Task ExecuteAsync(Func<Task> action) => ExecuteAsync(action, CancellationToken.None);
    public abstract Task<TryResult> ExecuteAsync(Func<Task> action, CancellationToken cancellationToken);

}

and adjusted the implementations:

class Never : Try
{
    public override TryResult Execute(Action action, CancellationToken cancellationToken)
    {
        return new TryResult(true, Enumerable.Empty<Exception>());
    }
    public override Task<TryResult> ExecuteAsync(Func<Task> action, CancellationToken cancellationToken) 
        => Task.FromResult<TryResult>(new TryResult(true, Enumerable.Empty<Exception>()));
}

the same for Retry:

class Retry : Try
{
    IEnumerable<int> Delays { get; }

    public Retry(IEnumerable<int> delays)
    {
        Delays = delays;
    }

    public override TryResult Execute(Action action, CancellationToken cancellationToken)
    {
        var exceptions = new List<Exception>();

        foreach (var delay in Delays)
        {
            try
            {
                action();
                return new TryResult(true, exceptions);
            }
            catch
            {
                cancellationToken.WaitHandle.WaitOne(delay);
                cancellationToken.ThrowIfCancellationRequested();
            }
        }
        return new TryResult(false, exceptions);
    }

    public override async Task<TryResult> ExecuteAsync(Func<Task> action, CancellationToken cancellationToken)
    {
        var exceptions = new List<Exception>();

        foreach (var delay in Delays)
        {
            try
            {
                await action();
                return new TryResult(true, exceptions);
            }
            catch
            {
                await Task.Delay(delay, cancellationToken);
            }
        }

        return new TryResult(false, exceptions);
    }
}
\$\endgroup\$
10
  • \$\begingroup\$ @DmitryNogin I've been experimenting ;-] what do you think? \$\endgroup\$
    – t3chb0t
    Commented Sep 7, 2016 at 19:45
  • \$\begingroup\$ I rather dislike having to deal with intentional (or unintentional) assignments of null to an IEnumerable<T>, because of this, I'd amend your constructor line of Exceptions = exceptions; to be Exceptions = exceptions ?? Enumerable.Empty<Exception>(); to guarantee something that can be iterated over. \$\endgroup\$ Commented Sep 7, 2016 at 20:13
  • \$\begingroup\$ I really can't read expression bodied functions...however I see them used more and more often (like this example). I'd really like to hear the point of view of someone who uses and appreciates them! (before someone says so...compactness isn't not a good - enough - reason) \$\endgroup\$ Commented Sep 7, 2016 at 21:21
  • \$\begingroup\$ @AdrianoRepetti be prepared for C# 7 - it will be even compacter with decomposition: (string first, string middle, string last) = LookupName(id1); // deconstructing declaration ;-) \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 4:09
  • \$\begingroup\$ @t3chb0t I saw that but I love new tuples for private methods (especially if they will also introduce local functions) and at calling point I feel it's less disorienting (well...at least for me). Still I love clarity of well placed {} otherwise I'd love Python more...am I an old-fashioned nostalgic guy? LOL \$\endgroup\$ Commented Sep 8, 2016 at 9:22

4 Answers 4

1
\$\begingroup\$

Let’s put the cart before the horse – I mean Catch before the Try :)

class Program
{
    static void Main(string[] args)
    {
        using (new Catch<InvalidOperationException>())
        using (new Catch<FormatException>(ex => Console.WriteLine("Oops!")))
            Try.Retry(100, 3, 3).Execute(() =>
            {
                Console.WriteLine("Trying!");
                throw new FormatException();
            });
    }
}

These using Catch statements define exceptions we can tolerate and continue trying. All other exceptions will be interpreted as critical. We can also rethrow or log in the optional handler.

Library classes:

public class Catch<TException> : Catch
    where TException : Exception
{
    public Catch()
        : this(ex => { })
    {
    }

    public Catch(Action<TException> handler)
    {
        Handler = handler;
    }

    protected internal override bool HandleCore(Exception ex)
    {
        if (ex is TException)
        {
            Handler(ex as TException);
            return true;
        }

        if (Previous == null)
            throw ex;
        else
            return Previous.HandleCore(ex);
    }

    Action<TException> Handler { get; }
}

And:

public abstract class Catch : Ambient<Catch>
{
    public static void Handle(Exception ex) =>
        Current?.HandleCore(ex);

    protected internal abstract bool HandleCore(Exception ex);
}

And:

public abstract class Ambient<T> : IDisposable where T : Ambient<T>
{
    static readonly string Id = typeof(T).FullName;
    protected static T Current
    {
        get { return (T)CallContext.LogicalGetData(Id); }
        set { CallContext.LogicalSetData(Id, value); }
    }

    protected Ambient()
    {
        Previous = Current;
        Current = (T)this;
    }

    public void Dispose() => Current = Previous;
    protected T Previous { get; }
}

And we also need to update Retry strategy:

    public override void Execute(Action action, CancellationToken cancellationToken)
    {
        foreach (var delay in Delays)
            try
            {
                action();
                return;
            }
            catch(Exception ex)
            {
                Catch.Handle(ex); // <- NEW LINE
                cancellationToken.WaitHandle.WaitOne(delay);
                cancellationToken.ThrowIfCancellationRequested();
            }

        action();
    }

UPDATE

Let's have this for the Log class - note Log.IfFail():

class Log : Ambient<Log>
{
    public static Exception IfFail(Action action)
    {
        try
        {
            action();
            return null;
        }
        catch(Exception ex)
        {
            Write(ex.Message);
            return ex;
        }
    }

    public static void Write(string line) =>
        Current?.WriteCore(line);

    public Log(string fileName)
    {
        FileName = fileName;
    }

    void WriteCore(string line) => File.AppendAllText(FileName, line);
    string FileName { get; }
}

And:

public static class Error
{
    public static void Rethrow(this Exception ex)
    {
        if (ex != null)
            throw ex;
    }
}

So we can have TryExecute:

    public override bool TryExecute(Action action, CancellationToken cancellationToken)
    {
        foreach (var delay in Delays)
            try
            {
                Log.IfFail(action).Rethrow();
                return true;
            }
            catch(Exception ex)
            {
                Catch.Handle(ex);
                cancellationToken.WaitHandle.WaitOne(delay);
                cancellationToken.ThrowIfCancellationRequested();
            }

        return Log.IfFail(action) == null; // yes, ugly :) let's invent something better
    }

We can pass more context for logging at the moment of ambient log construction.


UPDATE #2 :)

public static class Error
{
    public static void Rethrow(this Exception ex)
    {
        if (ex != null)
            throw ex;
    }

    public static bool ToTryResult(this Exception ex)
    {
        return ex == null;
    }
}

And:

    public override bool TryExecute(Action action, CancellationToken cancellationToken)
    {
        foreach (var delay in Delays)
            try
            {
                Log.IfFail(action).Rethrow();
                return true;
            }
            catch(Exception ex)
            {
                Catch.Handle(ex);
                cancellationToken.WaitHandle.WaitOne(delay);
                cancellationToken.ThrowIfCancellationRequested();
            }

        return Log.IfFail(action).ToTryResult();
    }
\$\endgroup\$
13
  • \$\begingroup\$ catch before try - this is good ;-) but not that unusual after all I would say... in SQL we write select ... from in linq by contrast from ... select - I like out of the box thinking :-] there is however still one piece missing, or is it? we still don't know if the operation was successful or not if we want to use it as a condition for subsequent steps. this was the idea behind the TryResult. \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 4:03
  • \$\begingroup\$ @t3chb0t This is why the very last invocation of action is outside of try catch in Execute above - you will get an old-fashioned exception when all attempts failed. \$\endgroup\$ Commented Sep 8, 2016 at 4:07
  • \$\begingroup\$ haha but I don't want this last exception ;-P I want to get rid of the try/catch and replace it with Retry - too much out of the box thinking I guess. I find it's unnecessary to have both a Retry and another try/catch around it. \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 4:12
  • \$\begingroup\$ It would duplicate all my code, I needed to log the exceptions inside the Retry and then again outside of it... \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 4:13
  • \$\begingroup\$ I would say that you will have more pain than gain with ‘status code return’ schema. .NET Framework Design Guidelines are pretty confident on this and here is the case where it makes all the sense to be consistent. Please also note that last exception is a special case – it does mean failure of the operation and probably needs special handling. I would use Log.IfFail("Oops!", () => action()) instead of last action() above. \$\endgroup\$ Commented Sep 8, 2016 at 4:31
1
\$\begingroup\$

Version #2. Disclaimer: it is also just an experiment (more sleep, less coffee :)

It is dangerous to eat all the exceptions, so I would go with a list of explicitly stated exceptions we expect and could tolerate. The same time, let’s try expose TryExecute version + define a special Catch inherited class, named LogError, which will log all the problems for us:

        using (new Catch<InvalidOperationException>())
        using (new Catch<FormatException>())
        using (new LogError(ex => Console.WriteLine(ex.Message))) // can be default ctor
            Try.Retry(100, 3, 3).TryExecute(() =>
            {
                Console.WriteLine("Trying!");
                throw new FormatException();
            });

Where (almost everything is the same):

public abstract class Try
{
    // …
    public abstract bool TryExecute(Action action, CancellationToken cancellationToken);
    // …
}

And:

class Retry : Try
{
    // …
    public override bool TryExecute(Action action, CancellationToken cancellationToken)
    {
        foreach (var delay in Delays)
            try
            {
                action();
                return true;
            }
            catch (Exception ex)
            {
                Catch.Handle(ex);
                cancellationToken.WaitHandle.WaitOne(delay);
                cancellationToken.ThrowIfCancellationRequested();
            }

        try
        {
            action();
            return true;
        }
        catch(Exception ex)
        {
            Catch.Handle(ex);
            return false;
        }
    }
    //…
}

And:

public class LogError : Catch<Exception>
{
    public LogError()
        : this(ex => Log.Write(ex.ToString()))
    {
    }

    public LogError(Action<Exception> action)
        : base(ex => { action(ex); return false; })
    {
    }
}

And:

public class Catch<TException> : Catch
    where TException : Exception
{
    public Catch()
        : this(ex => true)
    {
    }

    public Catch(Action<TException> handler)
        : this(ex => { handler(ex); return true; })
    {
    }

    public Catch(Predicate<TException> handler)
    {
        Handler = handler;
    }

    protected internal override bool HandleCore(Exception ex)
    {
        if (ex is TException)
            if (Handler(ex as TException))
                return true;

        if (Previous != null)
            return Previous.HandleCore(ex);

        throw ex;
    }

    Predicate<TException> Handler { get; }
}

And:

public abstract class Catch : Ambient<Catch>
{
    public static void Handle(Exception ex) =>
        Current?.HandleCore(ex);

    protected internal abstract bool HandleCore(Exception ex);
}

And:

public class Ambient<T> : IDisposable where T : Ambient<T>
{
    static readonly string Id = typeof(T).FullName;
    protected static T Current
    {
        get { return (T)CallContext.LogicalGetData(Id); }
        set { CallContext.LogicalSetData(Id, value); }
    }

    protected Ambient()
    {
        Previous = Current;
        Current = (T)this;
    }

    public void Dispose() => Current = Previous;
    protected T Previous { get; }
}
\$\endgroup\$
1
  • \$\begingroup\$ Let me see - I'll some time to anylize it :-] \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 17:31
1
\$\begingroup\$

Usage:

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

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

Templates:

public class IOTry
{
    public static readonly IRepeatable Slow = new Retry(Repeater.Repeat(delay: 1000, times: 4, ratio: 3));
    public static readonly IRepeatable Fast = new Retry(Repeater.Repeat(delay: 100, times: 4, ratio: 3));
}

Library code:

public interface IRepeatable
{
    void Execute(Action action);
    void Execute(Action action, CancellationToken cancellationToken);
}

Core class:

public class Repeater : IRepeatable
{
    private readonly IEnumerable<int> _delays;

    private Repeater(IEnumerable<int> delays)
    {
        _delays = delays;
    }

    public static Repeater Repeat(int delay, int times, double ratio) =>
        RepeatAfter(from i in Enumerable.Range(0, times)
                    select delay * Math.Pow(ratio, i) into d
                    select (int)d);

    public static Repeater RepeatAfter(params int[] delays) => RepeatAfter(delays.AsEnumerable());
    public static Repeater RepeatAfter(IEnumerable<int> delays) => new Repeater(delays);

    public void Execute(Action action) => Execute(action, CancellationToken.None);

    public void Execute(Action action, CancellationToken cancellationToken)
    {
        foreach (var delay in _delays)
        {
            action();
        }
    }
}

Decorator:

public class Retry : IRepeatable
{
    private readonly Repeater _repeater;

    public Retry(Repeater repeater)
    {
        _repeater = repeater;
    }

    public void Execute(Action action)
    {
        Execute(action, CancellationToken.None);
    }

    public void Execute(Action action, CancellationToken cancellationToken)
    {
        _repeater.Execute(() =>
        {
            try
            {
                action();
            }
            catch
            {
                cancellationToken.ThrowIfCancellationRequested();
            }
        }, cancellationToken);
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ @DmitryNogin what do you think about this? I think I might have found the ultimate design ;-) \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 18:51
  • \$\begingroup\$ @DmitryNogin strictly the loop doesn't belong the the try/retry so we can actaully refactor it into a different class that does only this. then the retry can by a decorator. \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 18:52
  • \$\begingroup\$ @DmitryNogin I removed the async code for simplicity. \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 18:53
  • \$\begingroup\$ @DmitryNogin there is however still one issure, I'm not sure how I should report the exceptions yet. \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 18:57
  • \$\begingroup\$ Let’s try this approach in a function style :) Please see my new answer. \$\endgroup\$ Commented Sep 8, 2016 at 20:05
1
\$\begingroup\$

Version #3 – Functional:

class Program
{
    public static readonly Retry ThreeTimes = (Action action) =>
        action
            .Log()
            .Ignore<FormatException>()
            .Ignore<InvalidOperationException>()
            .TryExecute(3);

    static void Main(string[] args)
    {
        Test(ThreeTimes);
    }

    static void Test(Retry retry)
    {
        Console.WriteLine(retry(() => { throw new FormatException(); }));
    }
}

Where:

public delegate bool Retry(Action action);

public static class RetryPattern
{
    public static Action Log(this Action action) => 
        () =>
        {
            try { action(); }
            catch (Exception ex)
            {
                Console.WriteLine(ex.Message);
                throw;
            }
        };

    public static Action Ignore<TException>(this Action action)
        where TException : Exception =>
        action.Catch<TException>(ex => { });

    public static Action Catch<TException>(this Action action, Action<TException> handler)
        where TException : Exception => () =>
        {
            try { action();}
            catch (TException ex)
            {
                handler(ex);
                throw new TemporaryException();
            }
        };

    public static bool TryExecute(this Action action, int retry)
    {
        for (int i = 0; i < retry; i++)
            try
            {
                action();
                return true;
            }
            catch (TemporaryException)
            {                    
            }

        try
        {
            action();
            return true;
        }
        catch(TemporaryException)
        {
            return false;
        }
    }
}

class TemporaryException : Exception
{
}
\$\endgroup\$
2
  • \$\begingroup\$ It's getting better and better ;-] \$\endgroup\$
    – t3chb0t
    Commented Sep 8, 2016 at 20:06
  • \$\begingroup\$ @t3chb0t Yep, this one is very pytonish :) \$\endgroup\$ Commented Sep 8, 2016 at 20:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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