1
Vote

Incorrect handling of overflow of retry intervals in the exponential backoff retry strategy

description

Consider the implementation of ExponentialBackoff::GetShouldRetry()
public override ShouldRetry GetShouldRetry()
{
    return delegate(int currentRetryCount, Exception lastException, out TimeSpan retryInterval)
    {
        if (currentRetryCount < this.retryCount)
        {
            var random = new Random();

            var delta = (int)((Math.Pow(2.0, currentRetryCount) - 1.0) * random.Next((int)(this.deltaBackoff.TotalMilliseconds * 0.8), (int)(this.deltaBackoff.TotalMilliseconds * 1.2)));
            var interval = (int)Math.Min(checked(this.minBackoff.TotalMilliseconds + delta), this.maxBackoff.TotalMilliseconds);

            retryInterval = TimeSpan.FromMilliseconds(interval);

            return true;
        }

        retryInterval = TimeSpan.Zero;
        return false;
    };
}
There are two issues with this code:
  1. The checked scope doesn't contain the actual calculation that's likely to overflow. When an overflow occurs, it will likely wrap back to some large negative integer that, when added to this.minBackoff.TotalMilliseconds, will produce a slightly smaller negative integer that will cause interval to be negative.
  2. Even if the checked scope did include the calculation, as far as I could follow the code there's no place expecting an OverflowException, so for example the ExecuteAsync task will fail at that point (even if further retries were due).

comments