RateLimiter SmoothWarmingUp does not limit if warmup period is smaller than 1000 NANOSECONDS

[gerabarna]

Hi!

We have encountered an issue when a Rate limiter has been set up with 1 permits per second, and a ZERO duration. This resulted in a RateLimiter, which gave out permits without any limitation. Further investigation revealed that any warmup period below 1 MICROSECOND will result in a non-limiting rate limiter.

A very simplistic reproducer:

    @Test
    void givenARateLimiterWithWarmupBelowOneMillisec_WhenRequesting_ThenRateLimitIsStillRespected() {
        RateLimiter limiter = RateLimiter.create(1, 999, TimeUnit.NANOSECONDS);
        final long start = System.currentTimeMillis();
        for (int j = 0; j < 5; j++) {
            limiter.acquire();
        }
        final long elapsed = System.currentTimeMillis() - start;
        Assertions.assertTrue(elapsed > 4000);
    }

I believe RateLimiter should either validate the input, and disallow such warmup settings, or hanle it porperly

[netdpb]

What is the actual elapsed time? RateLimiter interprets the warmup period in microseconds, so anything less than 1 microsecond is interpreted as zero. But that should mean you get one permit per second, so your elapsed time should be right at 4 seconds.

[gerabarna]

Added some logging to the simple reproducer above:

    @Test
    void givenARateLimiterWithWarmupBelowOneMillisec_WhenRequesting_ThenRateLimitIsStillRespected() {
        RateLimiter limiter = RateLimiter.create(1, 999, TimeUnit.NANOSECONDS);
        final long start = System.currentTimeMillis();
        long last = start;
        for (int j = 0; j < 5; j++) {
            limiter.acquire();
            final long current = System.currentTimeMillis();
            System.out.println(ZonedDateTime.now() + " elapsed " + (current - last) + "ms");
            last = current;
        }
        final long elapsed = System.currentTimeMillis() - start;
        Assertions.assertTrue(elapsed > 4000);
    }

the 5 permits were given within milliseconds 2023-05-10T09:52:31.365726541Z[UTC] elapsed 0ms 2023-05-10T09:52:31.369954164Z[UTC] elapsed 4ms 2023-05-10T09:52:31.370025221Z[UTC] elapsed 1ms 2023-05-10T09:52:31.370064898Z[UTC] elapsed 0ms 2023-05-10T09:52:31.370106134Z[UTC] elapsed 0ms

So the 1 second rate limit was clearely not respected.

I understand that we went below the time resolution. In that case I suppose the input validation should reject the arguments. There was an attempt at this in RateLimiter line 194 checkArgument(warmupPeriod >= 0, "warmupPeriod must not be negative: %s", warmupPeriod); unfortunately that does not take the resolution into account, and also allows 0. I suppose I can make a small PR, and add a check for the resolution, though the situation is a bit misleading, as the duration based factory method refers to NANOSECONDS:

public static RateLimiter create(double permitsPerSecond, Duration warmupPeriod) {
    return create(permitsPerSecond, toNanosSaturated(warmupPeriod), TimeUnit.NANOSECONDS);
}

So I am not sure what would be the correct direction for the fix

[netdpb]

Do you see the same behavior when calling create(1, Duration.ZERO)?

[gerabarna]

yes

[netdpb]

How important is it to support <1µs warmup time? That sounds awfully close to no warmup time. A simple solution would be either to throw in that case or to default to a non-warmup limiter.

[gerabarna]

As far as I could understand the RateLimiter classes, limiters without warmup time also have the behaviour of accumulating permits and allow short bursts from the accumulated pool. ( as I saw the SmoothBursty subclass is created ) Our use-case unfortunately cannot allow any such bursts, even if the RateLimiter was left idle for a time. the closest way to simulate such behaviour was to set the lowest possible warmup time instead. If you have a better suggestion on how to get a RateLimiter without any accumulation, I would be happy to switch.

Regardless, the point of this report was that validation should rule out an incorrectly constructed warmup limiter, which gives out permits without any limiting. Which currently happens with a low enough warmup time. Simply throwing an exception would achieve this I believe.

[weizheeeeee]

I have encountered the same issue recently, and I believe you can simply use the constructor with only one param of RateLimiter to generate your limiter, and I believe that solution will meet your demand.

[cpovirk]

(It sounds like this is probably the same as https://github.com/google/guava/issues/2730, so let's consolidate the discussion there.)