From 14207d2bef729fa765304f5e6e6ba0ea1a80e961 Mon Sep 17 00:00:00 2001 From: Dionysusnu <39664950+Dionysusnu@users.noreply.github.com> Date: Mon, 10 May 2021 18:48:07 +0200 Subject: [PATCH 1/6] Use retry-after header --- src/commands/sync.rs | 2 +- src/roblox_web_api.rs | 9 ++++-- src/sync_backend.rs | 71 +++++++++++++++++++++++++------------------ 3 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/commands/sync.rs b/src/commands/sync.rs index c3521c3..7899e2d 100644 --- a/src/commands/sync.rs +++ b/src/commands/sync.rs @@ -759,7 +759,7 @@ impl SyncError { pub fn is_rate_limited(&self) -> bool { match self { Self::Backend { - source: SyncBackendError::RateLimited, + source: SyncBackendError::RateLimited(_), } => true, _ => false, } diff --git a/src/roblox_web_api.rs b/src/roblox_web_api.rs index 258294c..8e6b7a1 100644 --- a/src/roblox_web_api.rs +++ b/src/roblox_web_api.rs @@ -4,7 +4,7 @@ use std::{ }; use reqwest::{ - header::{HeaderValue, COOKIE}, + header::{HeaderMap, HeaderValue, COOKIE}, Client, Request, Response, StatusCode, }; use serde::{Deserialize, Serialize}; @@ -171,6 +171,7 @@ impl RobloxApiClient { Err(RobloxApiError::ResponseError { status: response.status(), body, + headers: response.headers().clone(), }) } } @@ -245,5 +246,9 @@ pub enum RobloxApiError { }, #[error("Roblox API returned HTTP {status} with body: {body}")] - ResponseError { status: StatusCode, body: String }, + ResponseError { + status: StatusCode, + body: String, + headers: HeaderMap, + }, } diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 03e8e83..5c5e6b9 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, io, path::Path, thread, time::Duration}; +use std::{borrow::Cow, cmp::max, io, path::Path, thread, time::Duration}; use fs_err as fs; use reqwest::StatusCode; @@ -64,8 +64,17 @@ impl<'a> SyncBackend for RobloxSyncBackend<'a> { Err(RobloxApiError::ResponseError { status: StatusCode::TOO_MANY_REQUESTS, + headers, .. - }) => Err(Error::RateLimited), + }) => Err(Error::RateLimited( + headers + .get("retry-after") + .unwrap() + .to_str() + .unwrap() + .parse() + .unwrap(), + )), Err(err) => Err(err.into()), } @@ -112,43 +121,45 @@ impl SyncBackend for DebugSyncBackend { /// data. pub struct RetryBackend { inner: InnerSyncBackend, - delay: Duration, - attempts: usize, + min_delay: Duration, + max_attempts: usize, } impl RetryBackend { /// Creates a new backend from another SyncBackend. The max_retries parameter gives the number /// of times the backend will try again (so given 0, it acts just as the original SyncBackend). /// The delay parameter provides the amount of time to wait between each upload attempt. - pub fn new(inner: InnerSyncBackend, max_retries: usize, delay: Duration) -> Self { + pub fn new(inner: InnerSyncBackend, retries: usize, min_delay: Duration) -> Self { Self { inner, - delay, - attempts: max_retries + 1, + min_delay, + max_attempts: retries + 1, } } } impl SyncBackend for RetryBackend { fn upload(&mut self, data: UploadInfo) -> Result { - for index in 0..self.attempts { - if index != 0 { - log::info!( - "tarmac is being rate limited, retrying upload ({}/{})", - index, - self.attempts - 1 - ); - thread::sleep(self.delay); - } + for index in 1..=self.max_attempts { let result = self.inner.upload(data.clone()); - match result { - Err(Error::RateLimited) => {} + Err(Error::RateLimited(retry_after)) => { + let time = max(self.min_delay, Duration::new(retry_after, 0)); + log::info!( + "tarmac is being rate limited, retrying upload after {:?} ({} of {} retries)", + time, + index, + self.max_attempts - 1 + ); + thread::sleep(time); + if index == self.max_attempts { + return Err(Error::RateLimited(retry_after)); + } + } _ => return result, } } - - Err(Error::RateLimited) + unreachable!() } } @@ -158,7 +169,7 @@ pub enum Error { NoneBackend, #[error("Tarmac was rate-limited trying to upload assets. Try again in a little bit.")] - RateLimited, + RateLimited(u64), #[error(transparent)] Io { @@ -235,8 +246,8 @@ mod test { fn upload_again_if_rate_limited() { let mut counter = 0; let inner = CountUploads::new(&mut counter).with_results(vec![ - Err(Error::RateLimited), - Err(Error::RateLimited), + Err(Error::RateLimited(10)), + Err(Error::RateLimited(5)), Err(Error::NoneBackend), ]); let mut backend = RetryBackend::new(inner, 5, retry_duration()); @@ -251,8 +262,8 @@ mod test { let mut counter = 0; let success = UploadResponse { id: 10 }; let inner = CountUploads::new(&mut counter).with_results(vec![ - Err(Error::RateLimited), - Err(Error::RateLimited), + Err(Error::RateLimited(10)), + Err(Error::RateLimited(5)), Ok(success.clone()), ]); let mut backend = RetryBackend::new(inner, 5, retry_duration()); @@ -267,10 +278,10 @@ mod test { fn upload_returns_rate_limited_when_retries_exhausted() { let mut counter = 0; let inner = CountUploads::new(&mut counter).with_results(vec![ - Err(Error::RateLimited), - Err(Error::RateLimited), - Err(Error::RateLimited), - Err(Error::RateLimited), + Err(Error::RateLimited(10)), + Err(Error::RateLimited(10)), + Err(Error::RateLimited(10)), + Err(Error::RateLimited(10)), ]); let mut backend = RetryBackend::new(inner, 2, retry_duration()); @@ -278,7 +289,7 @@ mod test { assert_eq!(counter, 3); assert!(match upload_result { - Error::RateLimited => true, + Error::RateLimited(_) => true, _ => false, }); } From 40b7cf25335555250aa91540cfd535a9e19ef42c Mon Sep 17 00:00:00 2001 From: Dionysusnu <39664950+Dionysusnu@users.noreply.github.com> Date: Mon, 10 May 2021 23:13:21 +0200 Subject: [PATCH 2/6] Fix bug in retry message --- src/sync_backend.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 5c5e6b9..2d122a2 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -146,10 +146,10 @@ impl SyncBackend for RetryBackend { let time = max(self.min_delay, Duration::new(retry_after, 0)); log::info!( - "tarmac is being rate limited, retrying upload after {:?} ({} of {} retries)", + "tarmac is being rate limited, retrying upload after {:?} ({} of {} tries failed)", time, index, - self.max_attempts - 1 + self.max_attempts ); thread::sleep(time); if index == self.max_attempts { From f3092b2292a2afb5f2a5cc374850fad45b554884 Mon Sep 17 00:00:00 2001 From: Dionysusnu <39664950+Dionysusnu@users.noreply.github.com> Date: Wed, 12 May 2021 13:14:18 +0200 Subject: [PATCH 3/6] Add error for no retry-after header Change RateLimited from tuple to struct variant --- src/commands/sync.rs | 2 +- src/roblox_web_api.rs | 3 +++ src/sync_backend.rs | 38 ++++++++++++++++++++------------------ 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/commands/sync.rs b/src/commands/sync.rs index 7899e2d..e568271 100644 --- a/src/commands/sync.rs +++ b/src/commands/sync.rs @@ -759,7 +759,7 @@ impl SyncError { pub fn is_rate_limited(&self) -> bool { match self { Self::Backend { - source: SyncBackendError::RateLimited(_), + source: SyncBackendError::RateLimited { .. }, } => true, _ => false, } diff --git a/src/roblox_web_api.rs b/src/roblox_web_api.rs index 8e6b7a1..c3b9c7f 100644 --- a/src/roblox_web_api.rs +++ b/src/roblox_web_api.rs @@ -251,4 +251,7 @@ pub enum RobloxApiError { body: String, headers: HeaderMap, }, + + #[error("Tarmac did not receive a retry-after header from Roblox")] + MissingRetryHeader, } diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 2d122a2..6c3561e 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -66,15 +66,15 @@ impl<'a> SyncBackend for RobloxSyncBackend<'a> { status: StatusCode::TOO_MANY_REQUESTS, headers, .. - }) => Err(Error::RateLimited( - headers + }) => Err(Error::RateLimited { + wait_seconds: headers .get("retry-after") - .unwrap() + .ok_or(RobloxApiError::MissingRetryHeader)? .to_str() .unwrap() .parse() .unwrap(), - )), + }), Err(err) => Err(err.into()), } @@ -143,8 +143,8 @@ impl SyncBackend for RetryBackend { - let time = max(self.min_delay, Duration::new(retry_after, 0)); + Err(Error::RateLimited { wait_seconds }) => { + let time = max(self.min_delay, Duration::new(wait_seconds, 0)); log::info!( "tarmac is being rate limited, retrying upload after {:?} ({} of {} tries failed)", time, @@ -153,7 +153,7 @@ impl SyncBackend for RetryBackend return result, @@ -168,8 +168,10 @@ pub enum Error { #[error("Cannot upload assets with the 'none' target.")] NoneBackend, - #[error("Tarmac was rate-limited trying to upload assets. Try again in a little bit.")] - RateLimited(u64), + #[error( + "Tarmac was rate-limited trying to upload assets. Try again in `{wait_seconds}` seconds." + )] + RateLimited { wait_seconds: u64 }, #[error(transparent)] Io { @@ -246,8 +248,8 @@ mod test { fn upload_again_if_rate_limited() { let mut counter = 0; let inner = CountUploads::new(&mut counter).with_results(vec![ - Err(Error::RateLimited(10)), - Err(Error::RateLimited(5)), + Err(Error::RateLimited { wait_seconds: 10 }), + Err(Error::RateLimited { wait_seconds: 5 }), Err(Error::NoneBackend), ]); let mut backend = RetryBackend::new(inner, 5, retry_duration()); @@ -262,8 +264,8 @@ mod test { let mut counter = 0; let success = UploadResponse { id: 10 }; let inner = CountUploads::new(&mut counter).with_results(vec![ - Err(Error::RateLimited(10)), - Err(Error::RateLimited(5)), + Err(Error::RateLimited { wait_seconds: 10 }), + Err(Error::RateLimited { wait_seconds: 5 }), Ok(success.clone()), ]); let mut backend = RetryBackend::new(inner, 5, retry_duration()); @@ -278,10 +280,10 @@ mod test { fn upload_returns_rate_limited_when_retries_exhausted() { let mut counter = 0; let inner = CountUploads::new(&mut counter).with_results(vec![ - Err(Error::RateLimited(10)), - Err(Error::RateLimited(10)), - Err(Error::RateLimited(10)), - Err(Error::RateLimited(10)), + Err(Error::RateLimited { wait_seconds: 10 }), + Err(Error::RateLimited { wait_seconds: 10 }), + Err(Error::RateLimited { wait_seconds: 10 }), + Err(Error::RateLimited { wait_seconds: 10 }), ]); let mut backend = RetryBackend::new(inner, 2, retry_duration()); @@ -289,7 +291,7 @@ mod test { assert_eq!(counter, 3); assert!(match upload_result { - Error::RateLimited(_) => true, + Error::RateLimited { .. } => true, _ => false, }); } From 678f2b9c3cd88f390bb025176efdb4718ed75f85 Mon Sep 17 00:00:00 2001 From: Dionysusnu <39664950+Dionysusnu@users.noreply.github.com> Date: Fri, 14 May 2021 16:23:32 +0200 Subject: [PATCH 4/6] Make `wait_seconds` an `Option` --- src/sync_backend.rs | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 6c3561e..58190a8 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -73,7 +73,7 @@ impl<'a> SyncBackend for RobloxSyncBackend<'a> { .to_str() .unwrap() .parse() - .unwrap(), + .ok(), }), Err(err) => Err(err.into()), @@ -144,7 +144,7 @@ impl SyncBackend for RetryBackend { - let time = max(self.min_delay, Duration::new(wait_seconds, 0)); + let time = max(self.min_delay, Duration::new(wait_seconds.unwrap_or(0), 0)); log::info!( "tarmac is being rate limited, retrying upload after {:?} ({} of {} tries failed)", time, @@ -159,7 +159,7 @@ impl SyncBackend for RetryBackend return result, } } - unreachable!() + Err(Error::RateLimited { wait_seconds: None }) } } @@ -168,10 +168,8 @@ pub enum Error { #[error("Cannot upload assets with the 'none' target.")] NoneBackend, - #[error( - "Tarmac was rate-limited trying to upload assets. Try again in `{wait_seconds}` seconds." - )] - RateLimited { wait_seconds: u64 }, + #[error("Tarmac was rate-limited trying to upload assets.{}", .wait_seconds.map_or(String::from(""), |seconds| format!(" Try again in {} seconds.", seconds)))] + RateLimited { wait_seconds: Option }, #[error(transparent)] Io { @@ -248,8 +246,12 @@ mod test { fn upload_again_if_rate_limited() { let mut counter = 0; let inner = CountUploads::new(&mut counter).with_results(vec![ - Err(Error::RateLimited { wait_seconds: 10 }), - Err(Error::RateLimited { wait_seconds: 5 }), + Err(Error::RateLimited { + wait_seconds: Some(10), + }), + Err(Error::RateLimited { + wait_seconds: Some(5), + }), Err(Error::NoneBackend), ]); let mut backend = RetryBackend::new(inner, 5, retry_duration()); @@ -264,8 +266,12 @@ mod test { let mut counter = 0; let success = UploadResponse { id: 10 }; let inner = CountUploads::new(&mut counter).with_results(vec![ - Err(Error::RateLimited { wait_seconds: 10 }), - Err(Error::RateLimited { wait_seconds: 5 }), + Err(Error::RateLimited { + wait_seconds: Some(10), + }), + Err(Error::RateLimited { + wait_seconds: Some(5), + }), Ok(success.clone()), ]); let mut backend = RetryBackend::new(inner, 5, retry_duration()); @@ -280,10 +286,18 @@ mod test { fn upload_returns_rate_limited_when_retries_exhausted() { let mut counter = 0; let inner = CountUploads::new(&mut counter).with_results(vec![ - Err(Error::RateLimited { wait_seconds: 10 }), - Err(Error::RateLimited { wait_seconds: 10 }), - Err(Error::RateLimited { wait_seconds: 10 }), - Err(Error::RateLimited { wait_seconds: 10 }), + Err(Error::RateLimited { + wait_seconds: Some(10), + }), + Err(Error::RateLimited { + wait_seconds: Some(10), + }), + Err(Error::RateLimited { + wait_seconds: Some(10), + }), + Err(Error::RateLimited { + wait_seconds: Some(10), + }), ]); let mut backend = RetryBackend::new(inner, 2, retry_duration()); From 5ac4d64c72029c09d2906589362738c2b309cdc9 Mon Sep 17 00:00:00 2001 From: Dionysusnu <39664950+Dionysusnu@users.noreply.github.com> Date: Fri, 14 May 2021 16:30:01 +0200 Subject: [PATCH 5/6] Don't error if no retry-after header --- src/roblox_web_api.rs | 3 --- src/sync_backend.rs | 9 ++++----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/roblox_web_api.rs b/src/roblox_web_api.rs index c3b9c7f..8e6b7a1 100644 --- a/src/roblox_web_api.rs +++ b/src/roblox_web_api.rs @@ -251,7 +251,4 @@ pub enum RobloxApiError { body: String, headers: HeaderMap, }, - - #[error("Tarmac did not receive a retry-after header from Roblox")] - MissingRetryHeader, } diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 58190a8..4ca5304 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -69,11 +69,10 @@ impl<'a> SyncBackend for RobloxSyncBackend<'a> { }) => Err(Error::RateLimited { wait_seconds: headers .get("retry-after") - .ok_or(RobloxApiError::MissingRetryHeader)? - .to_str() - .unwrap() - .parse() - .ok(), + .map(|header| header.to_str().ok()) + .flatten() + .map(|header| header.parse().ok()) + .flatten(), }), Err(err) => Err(err.into()), From 88678c9bcfa6918bb11456acfd2d8f1ddeedce1c Mon Sep 17 00:00:00 2001 From: Dionysusnu <39664950+Dionysusnu@users.noreply.github.com> Date: Mon, 26 Jul 2021 21:31:40 +0200 Subject: [PATCH 6/6] Simplify .map(..).flatten() chains --- src/sync_backend.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 4ca5304..739eda3 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -69,10 +69,8 @@ impl<'a> SyncBackend for RobloxSyncBackend<'a> { }) => Err(Error::RateLimited { wait_seconds: headers .get("retry-after") - .map(|header| header.to_str().ok()) - .flatten() - .map(|header| header.parse().ok()) - .flatten(), + .and_then(|header| header.to_str().ok()) + .and_then(|header| header.parse().ok()), }), Err(err) => Err(err.into()),