resilience4j / resilience4j

Resilience4j is a fault tolerance library designed for Java8 and functional programming
Apache License 2.0
9.73k stars 1.33k forks source link

RateLimiter: changeLimitForPeriod does not work #1073

Closed rvit34 closed 2 years ago

rvit34 commented 4 years ago

Resilience4j version: 1.5.0

Java version: 1.8

This test reproduces the issue:

    @Test
    public void changeLimitForPeriodTest() {
        // create limiter with rate 10 rps
        final RateLimiterConfig config = RateLimiterConfig.custom()
                .limitRefreshPeriod(Duration.ofSeconds(1))
                .limitForPeriod(10)
                .timeoutDuration(Duration.ofMillis(0))
                .build();

        final RateLimiterRegistry registry = RateLimiterRegistry.of(config);
        final RateLimiter limiter = registry.rateLimiter("test");

        long t1 = System.currentTimeMillis();
        Assert.assertTrue(limiter.acquirePermission());
        Assert.assertTrue(limiter.acquirePermission());
        Assert.assertTrue(limiter.acquirePermission());

        // change limit up to 5 rps
        limiter.changeLimitForPeriod(5);

        Assert.assertTrue(limiter.acquirePermission()); 
        Assert.assertTrue(limiter.acquirePermission());
        Assert.assertTrue(limiter.acquirePermission()); // should be false?
        Assert.assertTrue(limiter.acquirePermission());
        Assert.assertTrue(limiter.acquirePermission());

        long t2 = System.currentTimeMillis();
        System.out.println("time spent(millis): " + (t2-t1));
        Assert.assertFalse(limiter.acquirePermission()); // fails here
    }
storozhukBM commented 4 years ago

Hi @rvit34 Thank you for listing this case for us, but I'm afraid this is expected behaviour. The documentation for changeLimitForPeriod method provides an explanation: NOTE! New limit won't affect current period permissions and will apply only from next one.

    /**
     * Dynamic rate limiter configuration change. This method allows to change count of permissions
     * available during refresh period. NOTE! New limit won't affect current period permissions and
     * will apply only from next one.
     *
     * @param limitForPeriod new permissions limit
     */
    void changeLimitForPeriod(int limitForPeriod);
rvit34 commented 4 years ago

It would be useful, especially in tests, to don't wait for the next period. Is it possible to add optional argument to the method to apply new limit immediately?

storozhukBM commented 4 years ago

@rvit34 I agree, this would be a convenient feature. Would you like to create PR?