migrator / guava-libraries-2

Guava: Google Core Libraries for Java 1.6+
0 stars 0 forks source link

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

Open migrator opened 9 years ago

migrator 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

relevance: 1

migrator commented 9 years ago

summary: Not Defined

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.

status Not Defined creator: cpov...@google.com created at: Jun 25, 2014

migrator commented 9 years ago

summary: Not Defined

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.

status Not Defined creator: cpov...@google.com created at: Jun 25, 2014

migrator commented 9 years ago

summary: Not Defined

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.

status Not Defined creator: cpov...@google.com created at: Jun 25, 2014