From 6887cac858500f86db65632ed6b5ead6d0f2e5d1 Mon Sep 17 00:00:00 2001 From: oltrep Date: Mon, 20 Jul 2020 09:38:39 -0700 Subject: [PATCH 1/7] Add retry backend and new command line options --- CHANGELOG.md | 2 + src/commands/sync.rs | 37 ++++++---- src/options.rs | 9 +++ src/sync_backend.rs | 156 ++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 189 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24c89ea..56ded1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased Changes +* Added support for automatically retrying uploads when being rate limited. ([#42](https://github.com/Roblox/tarmac/issues/43)) + ## 0.6.0 (2020-07-09) * Added support for automatically retrying image uploads when image names are moderated. ([#26](https://github.com/Roblox/tarmac/issues/26)) * Added `create-cache-map` subcommand to aid in prepopulating asset caches. ([#38](https://github.com/Roblox/tarmac/pull/38)) diff --git a/src/commands/sync.rs b/src/commands/sync.rs index 89cd375..f7a2751 100644 --- a/src/commands/sync.rs +++ b/src/commands/sync.rs @@ -3,6 +3,7 @@ use std::{ env, io::{self, BufWriter, Write}, path::{Path, PathBuf}, + time::Duration, }; use fs_err as fs; @@ -21,14 +22,23 @@ use crate::{ options::{GlobalOptions, SyncOptions, SyncTarget}, roblox_web_api::{RobloxApiClient, RobloxApiError}, sync_backend::{ - DebugSyncBackend, Error as SyncBackendError, NoneSyncBackend, RobloxSyncBackend, - SyncBackend, UploadInfo, + DebugSyncBackend, Error as SyncBackendError, NoneSyncBackend, RetryBackend, + RobloxSyncBackend, SyncBackend, UploadInfo, }, }; +fn sync_session(session: &mut SyncSession, options: &SyncOptions, mut backend: B) { + if let Some(retry) = options.retry { + let mut retry_backend = RetryBackend::new(backend, retry, Duration::from_secs(options.retry_delay)); + session.sync_with_backend(&mut retry_backend); + } else { + session.sync_with_backend(&mut backend); + } +} + pub fn sync(global: GlobalOptions, options: SyncOptions) -> Result<(), SyncError> { - let fuzzy_config_path = match options.config_path { - Some(v) => v, + let fuzzy_config_path = match &options.config_path { + Some(v) => v.to_owned(), None => env::current_dir()?, }; @@ -39,21 +49,20 @@ pub fn sync(global: GlobalOptions, options: SyncOptions) -> Result<(), SyncError session.discover_configs()?; session.discover_inputs()?; - match options.target { + match &options.target { SyncTarget::Roblox => { - let mut backend = - RobloxSyncBackend::new(&mut api_client, session.root_config().upload_to_group_id); - - session.sync_with_backend(&mut backend); + let group_id = session.root_config().upload_to_group_id; + sync_session( + &mut session, + &options, + RobloxSyncBackend::new(&mut api_client, group_id), + ); } SyncTarget::None => { - let mut backend = NoneSyncBackend; - - session.sync_with_backend(&mut backend); + sync_session(&mut session, &options, NoneSyncBackend); } SyncTarget::Debug => { - let mut backend = DebugSyncBackend::new(); - session.sync_with_backend(&mut backend); + sync_session(&mut session, &options, DebugSyncBackend::new()); } } diff --git a/src/options.rs b/src/options.rs index 73180f5..cb84d5e 100644 --- a/src/options.rs +++ b/src/options.rs @@ -71,6 +71,15 @@ pub struct SyncOptions { #[structopt(long)] pub target: SyncTarget, + /// When provided, Tarmac will upload again at most the given number of times + /// when it encounters rate limitation errors. + #[structopt(long)] + pub retry: Option, + + /// The number of seconds to wait between each re-upload attempts. + #[structopt(long, default_value = "60")] + pub retry_delay: u64, + /// The path to a Tarmac config, or a folder containing a Tarmac project. pub config_path: Option, } diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 6a442db..396a731 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, io, path::Path}; +use std::{borrow::Cow, io, path::Path, thread, time::Duration}; use fs_err as fs; use reqwest::StatusCode; @@ -10,10 +10,12 @@ pub trait SyncBackend { fn upload(&mut self, data: UploadInfo) -> Result; } +#[derive(Clone, Debug, PartialEq, Eq)] pub struct UploadResponse { pub id: u64, } +#[derive(Clone, Debug)] pub struct UploadInfo { pub name: String, pub contents: Vec, @@ -105,6 +107,44 @@ impl SyncBackend for DebugSyncBackend { } } +/// Performs the retry logic for rate limitation errors. The struct wraps a SyncBackend so that +/// when a RateLimited error occurs, the thread sleeps for a moment and then tries to reupload the +/// data. +pub struct RetryBackend { + inner: InnerSyncBackend, + delay: Duration, + 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 { + Self { + inner, + delay, + attempts: max_retries + 1, + } + } +} + +impl SyncBackend for RetryBackend { + fn upload(&mut self, data: UploadInfo) -> Result { + (0..self.attempts) + .map(|index| { + if index != 0 { + thread::sleep(self.delay); + } + self.inner.upload(data.clone()) + }) + .find(|result| { + !matches!(result, Err(Error::RateLimited)) + }) + .unwrap_or(Err(Error::RateLimited)) + } +} + #[derive(Debug, Error)] pub enum Error { #[error("Cannot upload assets with the 'none' target.")] @@ -125,3 +165,117 @@ pub enum Error { source: RobloxApiError, }, } + +#[cfg(test)] +mod test { + use super::*; + + #[allow(unused_must_use)] + mod test_retry_backend { + use super::*; + + struct CountUploads<'a> { + counter: &'a mut usize, + results: Vec>, + } + + impl<'a> CountUploads<'a> { + fn new(counter: &'a mut usize) -> Self { + Self { + counter, + results: Vec::new(), + } + } + + fn with_results(mut self, results: Vec>) -> Self { + self.results = results; + self.results.reverse(); + self + } + } + + impl<'a> SyncBackend for CountUploads<'a> { + fn upload(&mut self, _data: UploadInfo) -> Result { + (*self.counter) += 1; + self.results.pop() + .unwrap_or(Err(Error::NoneBackend)) + } + } + + fn any_upload_info() -> UploadInfo { + UploadInfo { + name: "foo".to_owned(), + contents: Vec::new(), + hash: "hash".to_owned(), + } + } + + fn retry_duration() -> Duration { Duration::from_millis(1) } + + #[test] + fn upload_at_least_once() { + let mut counter = 0; + let mut backend = RetryBackend::new( + CountUploads::new(&mut counter), + 0, + retry_duration(), + ); + + backend.upload(any_upload_info()); + + assert_eq!(counter, 1); + } + + #[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::NoneBackend), + ]); + let mut backend = RetryBackend::new(inner, 5, retry_duration()); + + backend.upload(any_upload_info()); + + assert_eq!(counter, 3); + } + + #[test] + fn upload_returns_first_success_result() { + let mut counter = 0; + let success = UploadResponse { id: 10 }; + let inner = CountUploads::new(&mut counter) + .with_results(vec![ + Err(Error::RateLimited), + Err(Error::RateLimited), + Ok(success.clone()), + ]); + let mut backend = RetryBackend::new(inner, 5, retry_duration()); + + let upload_result = backend.upload(any_upload_info()).unwrap(); + + assert_eq!(counter, 3); + assert_eq!(upload_result, success); + } + + #[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), + ]); + let mut backend = RetryBackend::new(inner, 2, retry_duration()); + + let upload_result = backend.upload(any_upload_info()).unwrap_err(); + + assert_eq!(counter, 3); + assert!(matches!(upload_result, Error::RateLimited)); + } + } +} From 8b2f6240e41a2b48013f60429f136cf629d4de04 Mon Sep 17 00:00:00 2001 From: oltrep Date: Mon, 20 Jul 2020 09:44:53 -0700 Subject: [PATCH 2/7] fix format --- src/commands/sync.rs | 3 ++- src/sync_backend.rs | 53 +++++++++++++++++++------------------------- 2 files changed, 25 insertions(+), 31 deletions(-) diff --git a/src/commands/sync.rs b/src/commands/sync.rs index f7a2751..c3521c3 100644 --- a/src/commands/sync.rs +++ b/src/commands/sync.rs @@ -29,7 +29,8 @@ use crate::{ fn sync_session(session: &mut SyncSession, options: &SyncOptions, mut backend: B) { if let Some(retry) = options.retry { - let mut retry_backend = RetryBackend::new(backend, retry, Duration::from_secs(options.retry_delay)); + let mut retry_backend = + RetryBackend::new(backend, retry, Duration::from_secs(options.retry_delay)); session.sync_with_backend(&mut retry_backend); } else { session.sync_with_backend(&mut backend); diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 396a731..eab5c86 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -138,9 +138,7 @@ impl SyncBackend for RetryBackend SyncBackend for CountUploads<'a> { fn upload(&mut self, _data: UploadInfo) -> Result { (*self.counter) += 1; - self.results.pop() - .unwrap_or(Err(Error::NoneBackend)) + self.results.pop().unwrap_or(Err(Error::NoneBackend)) } } @@ -210,16 +207,15 @@ mod test { } } - fn retry_duration() -> Duration { Duration::from_millis(1) } + fn retry_duration() -> Duration { + Duration::from_millis(1) + } #[test] fn upload_at_least_once() { let mut counter = 0; - let mut backend = RetryBackend::new( - CountUploads::new(&mut counter), - 0, - retry_duration(), - ); + let mut backend = + RetryBackend::new(CountUploads::new(&mut counter), 0, retry_duration()); backend.upload(any_upload_info()); @@ -229,12 +225,11 @@ mod test { #[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::NoneBackend), - ]); + let inner = CountUploads::new(&mut counter).with_results(vec![ + Err(Error::RateLimited), + Err(Error::RateLimited), + Err(Error::NoneBackend), + ]); let mut backend = RetryBackend::new(inner, 5, retry_duration()); backend.upload(any_upload_info()); @@ -246,12 +241,11 @@ mod test { fn upload_returns_first_success_result() { let mut counter = 0; let success = UploadResponse { id: 10 }; - let inner = CountUploads::new(&mut counter) - .with_results(vec![ - Err(Error::RateLimited), - Err(Error::RateLimited), - Ok(success.clone()), - ]); + let inner = CountUploads::new(&mut counter).with_results(vec![ + Err(Error::RateLimited), + Err(Error::RateLimited), + Ok(success.clone()), + ]); let mut backend = RetryBackend::new(inner, 5, retry_duration()); let upload_result = backend.upload(any_upload_info()).unwrap(); @@ -263,13 +257,12 @@ mod test { #[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), - ]); + let inner = CountUploads::new(&mut counter).with_results(vec![ + Err(Error::RateLimited), + Err(Error::RateLimited), + Err(Error::RateLimited), + Err(Error::RateLimited), + ]); let mut backend = RetryBackend::new(inner, 2, retry_duration()); let upload_result = backend.upload(any_upload_info()).unwrap_err(); From 78f624834b8e9e3261e41c89851540cda79b5d3e Mon Sep 17 00:00:00 2001 From: oltrep Date: Mon, 20 Jul 2020 09:52:48 -0700 Subject: [PATCH 3/7] remove matches macro --- src/sync_backend.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/sync_backend.rs b/src/sync_backend.rs index eab5c86..c794186 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -138,7 +138,10 @@ impl SyncBackend for RetryBackend false, + _ => true, + }) .unwrap_or(Err(Error::RateLimited)) } } From 6d479be28496e322c12284a86256e22dad35e470 Mon Sep 17 00:00:00 2001 From: oltrep Date: Mon, 20 Jul 2020 11:15:14 -0700 Subject: [PATCH 4/7] remove matches macro in test --- src/sync_backend.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/sync_backend.rs b/src/sync_backend.rs index c794186..3ae4ceb 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -271,7 +271,10 @@ mod test { let upload_result = backend.upload(any_upload_info()).unwrap_err(); assert_eq!(counter, 3); - assert!(matches!(upload_result, Error::RateLimited)); + assert!(match upload_result { + Error::RateLimited => true, + _ => false, + }); } } } From 66d76628ba7d089cf17e1147c8a51d59b8c6f520 Mon Sep 17 00:00:00 2001 From: oltrep Date: Wed, 22 Jul 2020 13:59:10 -0700 Subject: [PATCH 5/7] convert iterator to for loop --- src/sync_backend.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 3ae4ceb..1ba3270 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -131,18 +131,19 @@ impl RetryBackend { impl SyncBackend for RetryBackend { fn upload(&mut self, data: UploadInfo) -> Result { - (0..self.attempts) - .map(|index| { - if index != 0 { - thread::sleep(self.delay); - } - self.inner.upload(data.clone()) - }) - .find(|result| match result { - Err(Error::RateLimited) => false, - _ => true, - }) - .unwrap_or(Err(Error::RateLimited)) + for index in 0..self.attempts { + if index != 0 { + thread::sleep(self.delay); + } + let result = self.inner.upload(data.clone()); + + match result { + Err(Error::RateLimited) => {} + _ => return result, + } + } + + Err(Error::RateLimited) } } From 370426e23482965d6ad4472226cbbe1ef1818273 Mon Sep 17 00:00:00 2001 From: oltrep Date: Wed, 22 Jul 2020 14:40:32 -0700 Subject: [PATCH 6/7] add log message --- src/sync_backend.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/sync_backend.rs b/src/sync_backend.rs index 1ba3270..03e8e83 100644 --- a/src/sync_backend.rs +++ b/src/sync_backend.rs @@ -133,6 +133,11 @@ impl SyncBackend for RetryBackend 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); } let result = self.inner.upload(data.clone()); From e0c2c285228c34ae97b8c6da07422ea0563a9b62 Mon Sep 17 00:00:00 2001 From: oltrep Date: Wed, 22 Jul 2020 15:01:55 -0700 Subject: [PATCH 7/7] update readme --- README.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/README.md b/README.md index b8ed7c9..dcd8017 100644 --- a/README.md +++ b/README.md @@ -113,6 +113,8 @@ Usage: ```bash tarmac sync [] \ --target + --retry + --retry-delay <60> ``` To sync the project in your current working directory with the Roblox cloud, use: @@ -125,6 +127,11 @@ To validate that all inputs are already synced, use the `none` target: tarmac sync --target none ``` +When tarmac gets rate limited while syncing to Roblox, use the `--retry` argument to automatically attempt to re-upload. This will tell tarmac how many times it can attempt to re-upload each asset. The `--retry-delay` sets the number of seconds to wait between each attempt. +```bash +tarmac sync --target roblox --retry 3 +``` + ### `tarmac upload-image` Uploads a single image as a decal and prints the ID of the resulting image asset to stdout.