Guava: RateLimiter.create called with zero warmup fails with real clock

[greggdonovan]

We recently hit an issue where calling RateLimiter.create(16.0, 0, TimeUnit.SECONDS) failed to rate limit after an upgrade from Guava 17 to 21. I tried to reproduce this using the FakeStopwatch but could only get it to reproduce with a real clock.

I'll be happy to submit this as PR if the change below is useful.

Bisecting tells me this changed in this commit.

bisect.sh

#!/usr/bin/env bash

function back() {
  cd ~/development/guava
  git co .
}
trap back EXIT

JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk1.7.0_80.jdk/Contents/Home

# I can't get guava-gwt to compile :-(
sed -i '' 's_<module>guava-gwt</module>__' pom.xml

mvn clean install -Dmaven.test.skip=true || exit 125
cd guava-tests
{ mvn test -Dtest=com.google.common.util.concurrent.RateLimiterZeroWarmupTest && exit 0; } || exit 127

guava-tests/test/com/google/common/util/concurrent/RateLimiterZeroWarmupTest.java

package com.google.common.util.concurrent;

import java.util.concurrent.TimeUnit;
import junit.framework.TestCase;

public class RateLimiterZeroWarmupTest extends TestCase {

  public void testWarmUpWithZeroWarmupAndRealClock() {
    RateLimiter limiter = RateLimiter.create(16.0, 0, TimeUnit.SECONDS);
    int totalPermits = 1000;
    double totalSleep = 0;
    for (int i = 0; i < totalPermits; i++) {
      totalSleep += limiter.acquire();
    }
    assertFalse("totalSleep should be more than 60s, not 0", totalSleep == 0.0d); // <-- this fails
  }
}

Using the FakeStopwatch fails to reproduce:

diff --git a/guava-tests/test/com/google/common/util/concurrent/RateLimiterTest.java b/guava-tests/test/com/google/common/util/concurrent/RateLimiterTest.java
index b143bd94a..0c4de6c86 100644
--- a/guava-tests/test/com/google/common/util/concurrent/RateLimiterTest.java
+++ b/guava-tests/test/com/google/common/util/concurrent/RateLimiterTest.java
@@ -312,6 +312,14 @@ public class RateLimiterTest extends TestCase {
         "R0.20, R0.10, R0.10, R0.10"); // #7 (cont.), note, this matches #5
   }

+  public void testWarmUpWithZeroWarmup() {
+    RateLimiter rateLimiter = RateLimiter.create(stopwatch, 5.0, 0, TimeUnit.SECONDS, 3.0);
+    for (int i = 0; i < 5; i++) {
+      rateLimiter.acquire(); // #1
+      }
+      assertEvents("R0.00, R0.20, R0.20, R0.20, R0.20" /* #1 */); // <-- this works
+  }
+
   public void testBurstyAndUpdate() {
     RateLimiter rateLimiter = RateLimiter.create(stopwatch, 1.0);
     rateLimiter.acquire(1); // no wait

Guava workaround to use SmoothBursty when warmupSeconds == 0:

diff --git a/guava/src/com/google/common/util/concurrent/RateLimiter.java b/guava/src/com/google/common/util/concurrent/RateLimiter.java
index 945e2d168..cc99bbcbe 100644
--- a/guava/src/com/google/common/util/concurrent/RateLimiter.java
+++ b/guava/src/com/google/common/util/concurrent/RateLimiter.java
@@ -159,6 +159,9 @@ public abstract class RateLimiter {
    */
   public static RateLimiter create(double permitsPerSecond, long warmupPeriod, TimeUnit unit) {
     checkArgument(warmupPeriod >= 0, "warmupPeriod must not be negative: %s", warmupPeriod);
+    if (warmupPeriod == 0) {
+      return create(permitsPerSecond);
+    }
     return create(
         SleepingStopwatch.createFromSystemTimer(), permitsPerSecond, warmupPeriod, unit, 3.0);
   }

[liufuyang]

We encountered this same thing by setting warmupPeriod = 0, and the bad thing was we thought it was safe to change warmupPeriod = 5 to warmupPeriod = 0. But it actually make the rate limiter not limit any rate anymore, and created a huge CPU and memory cost in production.
Would be nice to fix this in one way or another https://github.com/google/guava/pull/3724
@cpovirk @cpovirk

[BirdHowl]

I've observed this issue in the latest version of guava as well.

Running this snippet:

import com.google.common.util.concurrent.RateLimiter;
import java.util.Duration;
public class App {
    public static void main(String[] args) {
        final RateLimiter rateLimiter = RateLimiter.create(1.0, Duration.ZERO);
        while (true) {
            System.out.println("Loop start: " + System.currentTimeMillis());
            rateLimiter.acquire();
        }
    }
}

Gives me this type of output (the loop repeating several times within a single millisecond):

Loop start: 1585178916022
Loop start: 1585178916022
Loop start: 1585178916022
Loop start: 1585178916022
Loop start: 1585178916022
Loop start: 1585178916022
Loop start: 1585178916022
Loop start: 1585178916022

Which seems ludicrous and has surprised everybody I've shown this to.

---

I see two reasonable resolutions in this case:

• treating a call to RateLimiter.create(someLimit, Duration.ZERO) as equivalent to RateLimiter.create(someLimit)
• defining passing a zero warmup as illegal

I think most people who would expect the first behavior, but if you consider the fact that the smoothing rate limiter also uses the warm up period to cool down, it seems undefined how the rate limiter should behave when it can freely warm up and cool down after no time - is it always warm or always cold? Halfway in-between?