finagle-core: Fix rate limiter if negative rate or after window

Problem

When rate is negative then request could be executed, and
after one elapsed time windows rate is not well modified.

Solution

Refuse request when rate is negative and reset correctly limit
after one elapsed time windows.

Signed-off-by: Vladimir Kostyukov <vkostyukov@twitter.com>

#766

Differential Revision: https://phabricator.twitter.biz/D322952

finagle-core/src/main/scala/com/twitter/finagle/service/RateLimitingFilter.scala

@@ -11,9 +11,11 @@ import com.twitter.finagle.{RefusedByRateLimiter, Service, SimpleFilter}
 class LocalRateLimitingStrategy[Req](categorizer: Req => String, windowSize: Duration, rate: Int)
     extends (Req => Future[Boolean]) {
 
+  require(rate > 0, s"rate $rate must be strictly positive.")
+
   private[this] val rates = mutable.HashMap.empty[String, (Int, Time)]
 
-  def apply(req: Req) = synchronized {
+  def apply(req: Req): Future[Boolean] = synchronized {
     val now = Time.now
     val id = categorizer(req)
     val (remainingRequests, timestamp) = rates.getOrElse(id, (rate, now))
@@ -25,8 +27,9 @@ class LocalRateLimitingStrategy[Req](categorizer: Req => String, windowSize: Dur
       if (remainingRequests > 0) {
         rates(id) = (remainingRequests - 1, timestamp)
         true
-      } else
+      } else {
         false
+      }
     }
 
     Future.value(accept)

finagle-core/src/test/scala/com/twitter/finagle/service/RateLimitingFilterTest.scala

@@ -9,13 +9,15 @@ import org.mockito.Matchers
 import org.mockito.Matchers._
 import com.twitter.conversions.DurationOps._
 import com.twitter.finagle.Service
-import com.twitter.util.{Await, Time, Future}
+import com.twitter.util.{Await, Duration, Future, Time}
 
 @RunWith(classOf[JUnitRunner])
 class RateLimitingFilterTest extends FunSuite with MockitoSugar {
-  class RateLimitingFilterHelper {
+
+  class RateLimitingFilterHelper(duration: Duration = 1.second, rate: Int = 5) {
     def categorize(i: Int) = (i % 5).toString
-    val strategy = new LocalRateLimitingStrategy[Int](categorize, 1.second, 5)
+
+    val strategy = new LocalRateLimitingStrategy[Int](categorize, duration, rate)
     val filter = new RateLimitingFilter[Int, Int](strategy)
     val service = mock[Service[Int, Int]]
     when(service.close(any)) thenReturn Future.Done
@@ -24,7 +26,7 @@ class RateLimitingFilterTest extends FunSuite with MockitoSugar {
     val rateLimitedService = filter andThen service
   }
 
-  test("RateLimitingFilter should Execute requests below rate limit") {
+  test("RateLimitingFilter should execute requests below rate limit") {
     val h = new RateLimitingFilterHelper
     import h._
 
@@ -37,7 +39,7 @@ class RateLimitingFilterTest extends FunSuite with MockitoSugar {
     }
   }
 
-  test("RateLimitingFilter should Refuse request if rate is above limit") {
+  test("RateLimitingFilter should refuse one request above rate limit") {
     val h = new RateLimitingFilterHelper
     import h._
 
@@ -55,8 +57,7 @@ class RateLimitingFilterTest extends FunSuite with MockitoSugar {
   }
 
   test(
-    "RateLimitingFilter should Execute different categories of requests and keep a window per category"
-  ) {
+    "RateLimitingFilter should execute different categories of requests and keep a window per category") {
     val h = new RateLimitingFilterHelper
     import h._
 
@@ -70,4 +71,11 @@ class RateLimitingFilterTest extends FunSuite with MockitoSugar {
       }
     }
   }
+
+  test("RateLimitingFilter should raise exception if rate limit is negative") {
+    intercept[IllegalArgumentException] {
+      new RateLimitingFilterHelper(rate = 0)
+    }
+  }
+
 }