Asynchronous retry method

Question

I've written an asynchronous retry method as an answer for this question. I'd like to get your opinion of the implementation and whether there are better ways to implement this. You could also implement this with async-await but I thought this would be a more efficient implementation.

public static Task RetryAsync(Func<bool> retryFunc, CancellationToken cancellationToken, int retryInterval)
{
 var tcs = new TaskCompletionSource<object>();
 var timer = new Timer((state) =>
 {
 var taskCompletionSource = (TaskCompletionSource<object>)state;
 if (!taskCompletionSource.Task.IsCompleted)
 {
 try
 {
 if (cancellationToken.IsCancellationRequested)
 {
 taskCompletionSource.SetException(new TaskCanceledException("RetryAsync cancelled"));
 }
 else if (retryFunc())
 {
 taskCompletionSource.SetResult(null);
 }
 }
 catch (Exception ex)
 {
 taskCompletionSource.SetException(ex);
 }
 }
 }, tcs, 0, retryInterval);
 //// Once the task is complete, dispose of the timer so it doesn't keep firing.
 tcs.Task.ContinueWith(t => timer.Dispose(),
 CancellationToken.None,
 TaskContinuationOptions.ExecuteSynchronously,
 TaskScheduler.Default);
 return tcs.Task;
}

• Vogel612: Hello and Welcome to Code Review! Unknowingly you became the second contestant in our new CR-Game. For more information see rags-to-riches. Feel free to drop by in Code Review Chat or on Code Review Meta ;)
• RubberDuck: Please see this meta post. Is it okay to update my questions for an iterative review?. TL;DR: We prefer you ask a new follow up question if you wish to have revised code reviewed.

Answer 1

You could also implement this with async-await but I thought this would be a more efficient implementation.

1. When performance matters, don't guess, measure. When it doesn't matter (which is 97 % of the time according to some), write code that is readable and maintainable.
2. Why do you think small increase in efficiency (most likely less than 1 ms) would matter here, when the retry interval is probably going to be hundreds of milliseconds or more (and can't effectively be less than 15 ms)?

---

Func<bool> retryFunc

Consider adding another overload that allows you to retry async functions (i.e. Func<Task<bool>> retryFunc).

---

CancellationToken cancellationToken

It might make sense to make this an optional parameter, some users might not need cancellation.

---

int retryInterval

I think it would be better to use TimeSpan here, that way both your code and the code of your users becomes more clear. If you want to keep using int, document very clearly the unit used, possibly by even renaming the parameter to something like retryIntervalMs.

You don't want to be the next Mars Climate Orbiter.

---

When the CancellationToken is canceled, why do you wait for the timer tick to cancel the returned Task? You could use Register() to make sure the Task is canceled as soon as the CancellationToken is.

---

if (!taskCompletionSource.Task.IsCompleted)

What's the purpose of this check? If it's because you're worried that retryFunc might run longer than retryInterval, then I think your logic is flawed. When I have an operation that takes 1 minute to run and I ask for retry after 5 seconds, I probably don't want to have 12 instances of the operation running at the same time.

You could achieve this by using Change().

And even when you want this behavior, you should probably also switch to using TrySet versions of the TaskCompletionSource methods, to avoid unnecessary exceptions.

---

if (cancellationToken.IsCancellationRequested)
{
 taskCompletionSource.SetException(new TaskCanceledException("RetryAsync cancelled"));
}

This way, the Task will be in the Faulted state. To get it to the correct Canceled state, use SetCanceled().

---

//// Once the task is complete, dispose of the timer so it doesn't keep firing.
tcs.Task.ContinueWith(t => timer.Dispose(),
 CancellationToken.None,
 TaskContinuationOptions.ExecuteSynchronously,
 TaskScheduler.Default);

This closure is important also because it keeps the Timer rooted, so it prevents it from being GCed prematurely. I would expand the comment to explain that.

(Also, why are you using four slashes for a comment? Two are enough.)

Answer 2

The suggested approach needs a timeout implementation and moreover, it has issues of calling the same function again if it is not already completed, due to the retry of timer mechanism.