Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graph: fix backoff overflow #4421

Merged
merged 1 commit into from
Mar 6, 2023
Merged

graph: fix backoff overflow #4421

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 69 additions & 1 deletion graph/src/util/backoff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl ExponentialBackoff {
}

pub fn delay(&self) -> Duration {
let mut delay = self.base.saturating_mul(1 << self.attempt);
let mut delay = self.base.saturating_mul(1u32 << self.attempt.min(31));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to make sure that next_attempt never increments self.attempt to more than 31. We can also do the min here in addition, but I think we should also clamp self.attempt to 31 explicitly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought it would be better to let it increment to have a sense how many total attempts there were, i.e. for logging.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. That makes sense

if delay > self.ceiling {
delay = self.ceiling;
}
Expand All @@ -50,3 +50,71 @@ impl ExponentialBackoff {
self.attempt = 0;
}
}

#[cfg(test)]
mod tests {
use super::*;
use std::time::Instant;

#[test]
fn test_delay() {
let mut backoff =
ExponentialBackoff::new(Duration::from_millis(500), Duration::from_secs(5));

// First delay should be base (0.5s)
assert_eq!(backoff.next_attempt(), Duration::from_millis(500));

// Second delay should be 1s (base * 2^1)
assert_eq!(backoff.next_attempt(), Duration::from_secs(1));

// Third delay should be 2s (base * 2^2)
assert_eq!(backoff.next_attempt(), Duration::from_secs(2));

// Fourth delay should be 4s (base * 2^3)
assert_eq!(backoff.next_attempt(), Duration::from_secs(4));

// Seventh delay should be ceiling (5s)
assert_eq!(backoff.next_attempt(), Duration::from_secs(5));

// Eighth delay should also be ceiling (5s)
assert_eq!(backoff.next_attempt(), Duration::from_secs(5));
}

#[test]
fn test_overflow_delay() {
let mut backoff =
ExponentialBackoff::new(Duration::from_millis(500), Duration::from_secs(45));

// 31st should be ceiling (45s) without overflowing
backoff.attempt = 31;
assert_eq!(backoff.next_attempt(), Duration::from_secs(45));
assert_eq!(backoff.next_attempt(), Duration::from_secs(45));

backoff.attempt = 123456;
assert_eq!(backoff.next_attempt(), Duration::from_secs(45));
}

#[tokio::test]
async fn test_sleep_async() {
let mut backoff =
ExponentialBackoff::new(Duration::from_secs_f32(0.1), Duration::from_secs_f32(0.2));

let start = Instant::now();
backoff.sleep_async().await;
let elapsed = start.elapsed();

assert!(elapsed >= Duration::from_secs_f32(0.1) && elapsed < Duration::from_secs_f32(0.15));

let start = Instant::now();
backoff.sleep_async().await;
let elapsed = start.elapsed();

assert!(elapsed >= Duration::from_secs_f32(0.2) && elapsed < Duration::from_secs_f32(0.25));

let start = Instant::now();
backoff.sleep_async().await;
let elapsed = start.elapsed();

assert!(elapsed >= Duration::from_secs_f32(0.2) && elapsed < Duration::from_secs_f32(0.25));
}
}