mkodekar / guava-libraries

Automatically exported from code.google.com/p/guava-libraries
Apache License 2.0
0 stars 0 forks source link

Double.POSITIVE_INFINITY seems to poison the com.google.common.util.concurrent.RateLimiter #1791

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Once a RateLimiter is configured with permitsPerSecond == 
Double.POSITIVE_INFINITY, it can never deliver a non-infinite rate. Tested on 
Guava 17.0.

Test case:

public static void main(String[] args) {
        RateLimiter rateLimiter = RateLimiter.create(Double.POSITIVE_INFINITY);
        final double permitsPerSecond = 1.0;
        rateLimiter.setRate(permitsPerSecond);
        final long l = System.currentTimeMillis();
        final int totalPermits = 60;
        final int permitsEachTime = 1;
        for (int i = 0; i < totalPermits; i++) {
            rateLimiter.acquire(permitsEachTime);
            System.out.print(i + ",");
        }
        final double seconds = 1.0 * (System.currentTimeMillis() - l) / 1000;
        System.out.println();
        System.out.println(seconds);
        System.out.println((totalPermits / permitsPerSecond));
    }

The behavior should be that it prints out the numbers about once a second and 
then prints the two "seconds" metrics roughly in agreement, like 
0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,
30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56
,57,58,59,
59.0
60.0

When the rate limiter is initialized with a non-infinite value (like 10.0), we 
see the desired behavior.

What the code above actually produces is:
0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,
30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56
,57,58,59,
0.002
60.0

showing that the limiter is still issuing permits at an infinite rate.

The culprit seems to be this code:
in com.google.common.util.concurrent.RateLimiter.Bursty#doSetRate
    storedPermits = (oldMaxPermits == 0.0)
          ? 0.0 // initial state
          : storedPermits * maxPermits / oldMaxPermits;
once the rate has been set to infinity, we also have an infinite number of 
stored permits, as well as an infinite number of oldMaxPermits. This division 
yields NaN, which then goes on to ensure that 
com.google.common.util.concurrent.RateLimiter#reserveNextTicket will return 0 
micros to wait.

It seems you may want to look at the whole rateLimiter with infinity in mind. 
Infinity is a reasonable value to configure a rate limiter with, so I think you 
should continue to accept it, but there may be other places where the math 
requires special handling of Infinity.

Let me know if I can help.

Thanks,
-John

Original issue reported on code.google.com by jroes...@gmail.com on 25 Jun 2014 at 2:02

GoogleCodeExporter commented 9 years ago
Interesting. The test method testInfinity_Bursty() is supposed to check this, 
but it must miss some case that your test catches. I'll look into it.

Original comment by cpov...@google.com on 25 Jun 2014 at 2:09

GoogleCodeExporter commented 9 years ago
It looks like the test doesn't catch this because no time elapses between the 
creation and the changing of the rate. As soon as I tell the fake stopwatch 
that time has elapsed, things go haywire.

Original comment by cpov...@google.com on 25 Jun 2014 at 2:19

GoogleCodeExporter commented 9 years ago
For a fix, I think we can get by with the same test as 
SmoothWarmingUp.doSetRate has. I don't have 100% confidence in that -- in 
particular, I can imagine that it's possible for maxPermits or another computed 
value to overflow to POSITIVE_INFINITY in another way -- but based on the facts 
that (a) SmoothWarmingUp is set up for this and (b) we had tests for this, I 
suspect that we'd given at least a passing thought to infinite-rate 
RateLimiters before.

Thanks for the report. I'll post again when the fix is submitted.

Original comment by cpov...@google.com on 25 Jun 2014 at 3:08

GoogleCodeExporter commented 9 years ago
This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<issue id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:08

GoogleCodeExporter commented 9 years ago

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:17

GoogleCodeExporter commented 9 years ago

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:07