ExecuteSuspendFunction Fails as delayMs Unexpectedly Becomes Zero

[JoosJuliet]

Resilience4j version:1.17.0
Java version:17
Kotlin version: 1.6.10

I have configured core.IntervalFunction.ofExponentialRandomBackoff with an initial value of 1 and a multiplier of 2.0. Additionally, I designated executeSuspendFunction as a suspend function to handle retries upon failures. However, I've observed that sometimes retries fail and sometimes they succeed. This prompted me to examine the code further.

Upon inspecting executeSuspendFunction, I noticed that when an exception occurs, if delayMs is less than 1, the exception is rethrown. Internally, there are instances where delayMs calculates to 0, which appears to be unintended behavior and thus seems to be a edge case

javaCopy code
} catch (e: Exception) {
    val delayMs = retryContext.onError(e)
    if (delayMs < 1) {
        throw e
    } else {
        delay(delayMs)
    }
}

The root cause of this issue lies in the implementation of the randomize function. When current is 1, the resulting value can range from 0.5 to 1.5.

javaCopy code
static double randomize(final double current, final double randomizationFactor) {
    final double delta = randomizationFactor * current;
    final double min = current - delta;
    final double max = current + delta;

    return (min + (Math.random() * (max - min + 1)));
}

If the return value is 0.5, the delay value generated by ofExponentialRandomBackoff might be cast to long as 0.

javaCopy code
return (long) randomize(interval, randomizationFactor);

This means that the variable delayMs within executeSuspendFunction becomes 0, causing an exception to be thrown and the process to terminate.

Setting a retry interval to 0 is not in line with the intentions of this project. Notably, the code specifies that the initial interval setting should not be less than 1.

javaCopy code
static void checkInterval(long intervalMillis) {
    if (intervalMillis < 1) {
        throw new IllegalArgumentException(
            "Illegal argument interval: " + intervalMillis + " millis is less than 1");
    }
}

May I proceed with handling this edge case?

[RobWin]

Hello,
thank you for debugging and finding the issue.
What would be your proposed fix?

[JoosJuliet]

Hi,
What about changing the 1 to 0 in executeSuspendFunction

 if (delayMs < 1)?

I think there shouldn’t be any issues with backward compatibility, but what do you think?

[RobWin]

I think I would prefer that the random function can only return 1 ms as a minimum

[JoosJuliet]

Nice. How about trying it this way?
To modify the randomize function so that it cannot return less than 1 millisecond as the minimum value, you can adjust the return statement to ensure that the minimum possible value is capped at 1.

static double randomize(final double current, final double randomizationFactor) {
    final double delta = randomizationFactor * current;
    final double min = current - delta;
    final double max = current + delta;

    double randomizedValue = min + (Math.random() * (max - min + 1));
    return Math.max(1.0, randomizedValue);  // Ensures that the minimum value returned is 1
}

[RobWin]

Looks good to me

[JoosJuliet]

I've created a PR, could you take a look? Also, should I include the following test code?
It seems like a good addition since it involves a change in the limits.

public void shouldAcceptRandomizationFactorOfOne() {
    final Duration duration = Duration.ofMillis(100);
    final float largestFactor = 1.0f;  

    Try<IntervalFunction> attempt = Try.of(() -> IntervalFunction.ofRandomized(duration, oneFactor));

    // Assert that the attempt is successful and does not result in an exception
    assertThat(attempt.isSuccess()).isTrue();
    assertThat(attempt.getCause()).isNull();
}