From fd8e193fce361a7396942cab901111af0b088139 Mon Sep 17 00:00:00 2001
From: Tuncer Ayaz <tuncer.ayaz@gmail.com>
Date: Wed, 20 Feb 2019 16:18:17 +0000
Subject: [PATCH] Disallow bad retry delay/jitter combination

When stress testing #1, I noticed that it's possible for
get_retry_timeout() to do something unexpected and throw an error.  By
disallowing retry_jitter >= retry_delay, and also changing the way
get_retry_timeout() works, we avoid that possibility.
---
 examples/example.rs |  2 +-
 src/errors.rs       |  1 +
 src/redlock.rs      | 25 ++++++++++++++-----------
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/examples/example.rs b/examples/example.rs
index ee7acd5..9a2afd2 100644
--- a/examples/example.rs
+++ b/examples/example.rs
@@ -9,7 +9,7 @@ fn example() -> RedlockResult<()> {
                                                "redis2.example.com",
                                                "redis3.example.com"],
                                    retry_count: 10,
-                                   retry_delay: time::Duration::from_millis(400),
+                                   retry_delay: time::Duration::from_millis(401),
                                    retry_jitter: 400,
                                    drift_factor: 0.01,
                                })?;
diff --git a/src/errors.rs b/src/errors.rs
index 81f5d6a..49d33a6 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -10,6 +10,7 @@ quick_error!{
     TimeError(err: time::SystemTimeError) { from(err: time::SystemTimeError) -> (err) }
     NoServerError { description("Redlock must be initialized with at least one redis server") }
     TimeoutError { description("Redlock request timeout") }
+    DelayJitterError { description("Retry jitter must be smaller than retry delay") }
     LockExpired { description("The lock has already expired") }
     UnableToLock { description("Unable to lock the resource") }
     UnableToUnlock { description("Unable to unlock the resource") }
diff --git a/src/redlock.rs b/src/redlock.rs
index 5d51fc3..dca2409 100644
--- a/src/redlock.rs
+++ b/src/redlock.rs
@@ -46,7 +46,7 @@ pub struct Config<T>
     pub addrs: Vec<T>,
     pub retry_count: u32,
     pub retry_delay: Duration,
-    pub retry_jitter: u32,
+    pub retry_jitter: u64,
     pub drift_factor: f32,
 }
 
@@ -55,7 +55,7 @@ impl Default for Config<&'static str> {
         Config {
             addrs: vec!["redis://127.0.0.1"],
             retry_count: 10,
-            retry_delay: Duration::from_millis(400),
+            retry_delay: Duration::from_millis(401),
             retry_jitter: 400,
             drift_factor: 0.01,
         }
@@ -67,7 +67,7 @@ pub struct Redlock {
     clients: Vec<redis::Client>,
     retry_count: u32,
     retry_delay: Duration,
-    retry_jitter: u32,
+    retry_jitter: u64,
     drift_factor: f32,
     quorum: usize,
 }
@@ -78,6 +78,9 @@ impl Redlock {
         if config.addrs.is_empty() {
             return Err(RedlockError::NoServerError);
         }
+        if Duration::from_millis(config.retry_jitter) >= config.retry_delay {
+            return Err(RedlockError::DelayJitterError);
+        }
         let mut clients = Vec::with_capacity(config.addrs.len());
         for addr in config.addrs {
             clients.push(redis::Client::open(addr)?)
@@ -231,11 +234,11 @@ impl Redlock {
     }
 
     fn get_retry_timeout(&self) -> Duration {
-        let jitter = self.retry_jitter as i32 * thread_rng().gen_range(-1, 1);
-        if jitter >= 0 {
-            self.retry_delay.add(Duration::from_millis(jitter as u64))
+        let jitter = thread_rng().gen_range(-1, 1);
+        if jitter == -1 {
+            self.retry_delay.sub(Duration::from_millis(self.retry_jitter))
         } else {
-            self.retry_delay.sub(Duration::from_millis(-jitter as u64))
+            self.retry_delay.add(Duration::from_millis(self.retry_jitter))
         }
     }
 }
@@ -288,7 +291,7 @@ mod tests {
         static ref REDLOCK: Redlock = Redlock::new::<&str>(Config {
             addrs: vec!["redis://127.0.0.1"],
             retry_count: 10,
-            retry_delay: Duration::from_millis(400),
+            retry_delay: Duration::from_millis(401),
             retry_jitter: 400,
             drift_factor: 0.01,
         }).unwrap();
@@ -301,7 +304,7 @@ mod tests {
         let default_config = Config::default();
         assert_eq!(default_config.addrs, vec!["redis://127.0.0.1"]);
         assert_eq!(default_config.retry_count, 10);
-        assert_eq!(default_config.retry_delay, Duration::from_millis(400));
+        assert_eq!(default_config.retry_delay, Duration::from_millis(401));
         assert_eq!(default_config.retry_jitter, 400);
         assert_eq!(default_config.drift_factor, 0.01);
     }
@@ -312,7 +315,7 @@ mod tests {
         Redlock::new::<&str>(Config {
                                  addrs: vec![],
                                  retry_count: 10,
-                                 retry_delay: Duration::from_millis(400),
+                                 retry_delay: Duration::from_millis(401),
                                  retry_jitter: 400,
                                  drift_factor: 0.01,
                              })
@@ -324,7 +327,7 @@ mod tests {
         let redlock = Redlock::new(Config::default()).unwrap();
         assert_eq!(redlock.clients.len(), 1);
         assert_eq!(redlock.retry_count, 10);
-        assert_eq!(redlock.retry_delay, Duration::from_millis(400));
+        assert_eq!(redlock.retry_delay, Duration::from_millis(401));
     }
 
     #[test]