From 05f561836f0a250cf38f0d9a124b403b10712808 Mon Sep 17 00:00:00 2001
From: Shashikanth Reddy Potu <spotu@twitter.com>
Date: Mon, 4 Nov 2024 17:09:14 +0000
Subject: [PATCH] finagle-core: ExponentialJitteredBackoff random zero bugfix

**Problem:** `ExponentialJitteredBackoff.next` throws an exception occasionally.
``` java.lang.IllegalArgumentException:
     at java.base/java.util.concurrent.ThreadLocalRandom.nextLong(ThreadLocalRandom.java:361)
     at com.twitter.finagle.util.Rng$$anon$2.nextLong(Rng.scala:71) ERR
     at com.twitter.finagle.Backoff$ExponentialJittered.next(Backoff.scala:331) ```
**Bug:** `Rng.nextLong(n)` generates a number from 0 (inclusive) to n
(exclusive). When it generates 0 as random number. The next occurrence of
backoff.next would try fetching `Rng.nextLong(0)` which is illegal argument.

**Fix:** Add 1 to the generated random, so that `Rng.nextLong(_)` can accept the
number. Since the random generated is exclusive on the `n` provided, this
doesn't cause overflow as well.

JIRA Issues: STOR-8734

Differential Revision: https://phabricator.twitter.biz/D1178528
---
 CHANGELOG.rst                                 |  1 +
 .../scala/com/twitter/finagle/Backoff.scala   |  5 ++--
 .../com/twitter/finagle/BackoffTest.scala     | 26 +++++++++++++++----
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index d6078407028..f9ff9d09e7a 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -31,6 +31,7 @@ Bug Fixes
 
 * finagle-memcached: Fixed support for running memcached tests with external memcached. Added README with
   instructions under finagle/finagle-memcached. ``PHAB_ID=D1120240``
+* finagle-core: Fixed a bug in `ExponentialJitteredBackoff` where `rng` can be 0. ``PHAB_ID=D1178528``
 
 
 Breaking API Changes
diff --git a/finagle-core/src/main/scala/com/twitter/finagle/Backoff.scala b/finagle-core/src/main/scala/com/twitter/finagle/Backoff.scala
index fceb29c5894..05830206fdc 100644
--- a/finagle-core/src/main/scala/com/twitter/finagle/Backoff.scala
+++ b/finagle-core/src/main/scala/com/twitter/finagle/Backoff.scala
@@ -328,8 +328,9 @@ object Backoff {
       if (maxBackoff == maximum) {
         new Const(maximum)
       } else {
-        val random = Duration.fromNanoseconds(rng.nextLong(maxBackoff.inNanoseconds))
-        new ExponentialJittered(random, maximum, attempt + 1, rng)
+        // to avoid the case of random being 0
+        val random = 1 + rng.nextLong(maxBackoff.inNanoseconds)
+        new ExponentialJittered(Duration.fromNanoseconds(random), maximum, attempt + 1, rng)
       }
     }
 
diff --git a/finagle-core/src/test/scala/com/twitter/finagle/BackoffTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/BackoffTest.scala
index c1f7c5d92ec..5683f6a064d 100644
--- a/finagle-core/src/test/scala/com/twitter/finagle/BackoffTest.scala
+++ b/finagle-core/src/test/scala/com/twitter/finagle/BackoffTest.scala
@@ -147,15 +147,21 @@ class BackoffTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks {
     forAll(exponentialGen) {
       case (startMs: Long, maxMs: Long, seed: Long) =>
         val rng = Rng(seed)
-        val backoff: Backoff =
-          new ExponentialJittered(startMs.millis, maxMs.millis, 1, Rng(seed))
-        val result: ArrayBuffer[Duration] = new ArrayBuffer[Duration]()
         var start = startMs.millis
+        val max = maxMs.millis
+        val backoff: Backoff = new ExponentialJittered(start, max, 1, Rng(seed))
+        val result: ArrayBuffer[Duration] = new ArrayBuffer[Duration]()
         for (attempt <- 1 to 7) {
           result.append(start)
-          start = nextStart(start, maxMs.millis, rng, attempt)
+          start = nextStart(start, max, rng, attempt)
         }
         verifyBackoff(backoff, result.toSeq, exhausted = false)
+
+        // Verify case where Rng returns 0.
+        val zeroRng = getZeroRng(rng)
+        val zeroRngBackoff: Backoff = new ExponentialJittered(start, max, 1, zeroRng)
+        val expectedResults = Seq(start, nextStart(start, max, zeroRng, 1))
+        verifyBackoff(zeroRngBackoff, expectedResults, exhausted = false)
     }
 
     def nextStart(start: Duration, max: Duration, rng: Rng, attempt: Int): Duration = {
@@ -163,7 +169,7 @@ class BackoffTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks {
       // to avoid Long overflow
       val maxBackoff = if (start >= max / shift) max else start * shift
       if (maxBackoff == max) max
-      else Duration.fromNanoseconds(rng.nextLong(maxBackoff.inNanoseconds))
+      else Duration.fromNanoseconds(1 + rng.nextLong(maxBackoff.inNanoseconds))
     }
   }
 
@@ -331,4 +337,14 @@ class BackoffTest extends AnyFunSuite with ScalaCheckDrivenPropertyChecks {
     }
     assert(actualBackoff.isExhausted == exhausted)
   }
+
+  private[this] def getZeroRng(rng: Rng): Rng = new Rng {
+    override def nextDouble(): Double = rng.nextDouble()
+
+    override def nextInt(n: Int): Int = rng.nextInt(n)
+
+    override def nextInt(): Int = rng.nextInt()
+
+    override def nextLong(n: Long): Long = 0L
+  }
 }