Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How would I retry a failed method? #25

Open
SuperJMN opened this issue Oct 26, 2017 · 6 comments
Open

How would I retry a failed method? #25

SuperJMN opened this issue Oct 26, 2017 · 6 comments
Labels

Comments

@SuperJMN
Copy link

I'm calling a remote service that is rate-limited. If you invoke a method lots of times, it ends up throwing a Exception. I would like to be able to retry a call until it finished successfully. The current code I have is this (notice that I only added a do-loop around your original code :)

        public class ExceptionHandlingInterceptor : AsyncInterceptorBase
        {
            protected override async Task InterceptAsync(IInvocation invocation, Func<IInvocation, Task> proceed)
            {
                bool rateLimit;
                do
                {
                    try
                    {
                        // Cannot simply return the the task, as any exceptions would not be caught below.
                        await proceed(invocation).ConfigureAwait(false);
                        rateLimit = false;
                    }
                    catch (FaceAPIException ex)
                    {
                        if (ex.ErrorCode == "RateLimitExceeded")
                        {
                            rateLimit = true;
                        }
                        else
                        {
                            Log.Error($"Error calling {invocation.Method.Name}.", ex);
                            throw;
                        }
                    }
                    catch (Exception ex)
                    {
                        Log.Error($"Error calling {invocation.Method.Name}.", ex);
                        throw;
                    }

                } while (rateLimit);
            }

            protected override async Task<T> InterceptAsync<T>(IInvocation invocation, Func<IInvocation, Task<T>> proceed)
            {
                bool ratelimit = false;
                T retValue = default(T);
                do
                {
                    try
                    {
                        // Cannot simply return the the task, as any exceptions would not be caught below.
                        retValue = await proceed(invocation).ConfigureAwait(false);
                    }
                    catch (FaceAPIException ex)
                    {
                        if (ex.ErrorCode == "RateLimitExceeded")
                        {
                            ratelimit = true;                            
                        }
                        else
                        {
                            Log.Error($"Error calling {invocation.Method.Name}.", ex);
                            throw;
                        }
                    }
                    catch (Exception ex)
                    {
                        Log.Error($"Error calling {invocation.Method.Name}.", ex);
                        throw;
                    }

                } while (ratelimit);

                return retValue;
            }            
        }

Thanks in advance!

@JSkimming
Copy link
Owner

@SuperJMN I don't use DynamicProxy for retry, I use Polly instead. Does your retry code work for synchronous calls, and using DynamicProxy directly without using this library?

@SuperJMN
Copy link
Author

SuperJMN commented Oct 27, 2017

With DynamicProxy ONLY it doesn't work, because the methods in the service are async-based (they return Tasks). I was using a wrapper before, that has a custom retry logic (using System.Reactive) and an Exponential Kick Off Retry policy, but I would like to do all the logic intercepting the calls.

@JSkimming
Copy link
Owner

@SuperJMN I asked if it works for synchronous calls. I'm trying to understand if this is a constraint of DynamicProxy or this library.

Though as I said, I don't use DynamicProxy, therefore it's not a scenario with which I'm familiar. In that regard maybe you're better off trying stack overflow instead.

@SuperJMN
Copy link
Author

Oh, OK.

I'm not completely sure if it work to retry sync calls. But I think they should work.

Also, I don't know if the problem is with your library, but it's possible that the reason why the retry doesn't work is that the async method is called once, and it the task it receives is faulted, then, the faulted task is returned in each iteration. In other words, it doesn't actually retry the operation, it just returns the same faulted tasks and the loop is repeated forever.

Could you please check the code? for example, make a sample class with a method with the following behaviors
a) when it's invoke the 1st time, throw an exception.
b) the 2nd time and successive calls don't throw any exception.

Using your AsyncInterceptor and the do-loop I put, it should loop forever.

Thanks for your patience!

@JSkimming
Copy link
Owner

@SuperJMN I'm afraid I don't have the time to perform an investigation at the moment. I may have a look at a later date, but I'm afraid I cannot commit to investigating this anytime soon.

Can you try it? There are extensive tests, you could try it out as a test. If you find an issue, and you submit a PR with a failing test, then I could pick it it from there if you're unable to find a fix yourself.

PRs are always welcome :-)

@ndrwrbgs
Copy link
Collaborator

ndrwrbgs commented Nov 5, 2017

@SuperJMN If you're blocked by this, you can use the following work around class. However, this uses Reflection and relies on internal details of Castle.Core, so I would not recommend it for production code or as a true fix. It works with Castle.Core v4.2.0 net46. I'll continue looking into more proper solutions to the problem.

Primary caveats:

  1. It may not work if you are attaching MULTIPLE interceptors to the same object (it may, I just haven't tested it and the logic there seems the most prone to be brittle)
  2. It's not a drop-in-place solution, as I need to wrap your implementing methods. You'll need to rename your InterceptAsync implementations to InterceptAsyncCore.
namespace Castle.DynamicProxy.Hack
{
    using System;
    using System.Reflection;
    using System.Threading.Tasks;
    using Castle.DynamicProxy;

    public abstract class AsyncInterceptorBaseFix : AsyncInterceptorBase
    {
        private readonly FieldInfo currentInterceptorIndexField;

        protected AsyncInterceptorBaseFix()
        {
            // Because awaits will make it decrease it's nice little counter before it's done, we wrap that little counter up <3
            // Little counters need love too
            this.currentInterceptorIndexField = typeof(AbstractInvocation).GetField(
                "currentInterceptorIndex",
                BindingFlags.NonPublic | BindingFlags.Instance);
        }

        protected sealed override async Task InterceptAsync(IInvocation invocation, Func<IInvocation, Task> proceed)
        {
            // Artificially increment currentInterceptorIndex, we don't actually want to increment it but we DO want to prevent the decrement the base class performs
            int currentValue = (int)this.currentInterceptorIndexField.GetValue(invocation);
            this.currentInterceptorIndexField.SetValue(invocation, (object)(currentValue + 1));

            try
            {
                // base class will decrement currentInterceptorIndex right after the first await returns control, ensure that is immediate with Task.Yield
                await Task.Yield();
                await InterceptAsyncCore(invocation, proceed);
            }
            finally
            {
                // Decrement currentInterceptorIndex (reset to what it was before)
                this.currentInterceptorIndexField.SetValue(invocation, currentValue);
            }
        }

        protected sealed override async Task<TResult> InterceptAsync<TResult>(IInvocation invocation, Func<IInvocation, Task<TResult>> proceed)
        {
            // Artificially increment currentInterceptorIndex, we don't actually want to increment it but we DO want to prevent the decrement the base class performs
            int currentValue = (int)this.currentInterceptorIndexField.GetValue(invocation);
            this.currentInterceptorIndexField.SetValue(invocation, (object)(currentValue + 1));

            try
            {
                // base class will decrement currentInterceptorIndex right after the first await returns control, ensure that is immediate with Task.Yield
                await Task.Yield();
                return await InterceptAsyncCore(invocation, proceed);
            }
            finally
            {
                // Decrement currentInterceptorIndex (reset to what it was before)
                this.currentInterceptorIndexField.SetValue(invocation, currentValue);
            }
        }

        protected abstract Task InterceptAsyncCore(IInvocation invocation, Func<IInvocation, Task> proceed);

        protected abstract Task<TResult> InterceptAsyncCore<TResult>(
            IInvocation invocation,
            Func<IInvocation, Task<TResult>> proceed);
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants