Remove Option from list of failed transfers

This commit addresses #67 (comment)
awslabs · Nov 6, 2024 · de6d920 · de6d920
1 parent 245e47b
commit de6d920
Show file tree

Hide file tree

Showing 8 changed files with 25 additions and 37 deletions.
diff --git a/aws-s3-transfer-manager/src/operation/download_objects.rs b/aws-s3-transfer-manager/src/operation/download_objects.rs
@@ -79,7 +79,7 @@ impl DownloadObjects {
 #[derive(Debug)]
 pub(crate) struct DownloadObjectsState {
     input: DownloadObjectsInput,
-    failed_downloads: Mutex<Option<Vec<FailedDownloadTransfer>>>,
+    failed_downloads: Mutex<Vec<FailedDownloadTransfer>>,
     successful_downloads: AtomicU64,
     total_bytes_transferred: AtomicU64,
 }
@@ -90,7 +90,7 @@ impl DownloadObjectsContext {
     fn new(handle: Arc<crate::client::Handle>, input: DownloadObjectsInput) -> Self {
         let state = Arc::new(DownloadObjectsState {
             input,
-            failed_downloads: Mutex::new(None),
+            failed_downloads: Mutex::new(Vec::new()),
             successful_downloads: AtomicU64::default(),
             total_bytes_transferred: AtomicU64::default(),
         });

diff --git a/aws-s3-transfer-manager/src/operation/download_objects/handle.rs b/aws-s3-transfer-manager/src/operation/download_objects/handle.rs
@@ -28,7 +28,8 @@ impl DownloadObjectsHandle {
             join_result??;
         }
 
-        let failed_downloads = self.ctx.state.failed_downloads.lock().unwrap().take();
+        let failed_downloads =
+            std::mem::take(&mut *self.ctx.state.failed_downloads.lock().unwrap());
         let successful_downloads = self.ctx.state.successful_downloads.load(Ordering::SeqCst);
         let total_bytes_transferred = self
             .ctx

diff --git a/aws-s3-transfer-manager/src/operation/download_objects/output.rs b/aws-s3-transfer-manager/src/operation/download_objects/output.rs
@@ -13,7 +13,7 @@ pub struct DownloadObjectsOutput {
     pub objects_downloaded: u64,
 
     /// A list of failed object transfers
-    pub failed_transfers: Option<Vec<FailedDownloadTransfer>>,
+    pub failed_transfers: Vec<FailedDownloadTransfer>,
 
     // FIXME - likely remove when progress is implemented?
     /// Total number of bytes transferred
@@ -32,11 +32,8 @@ impl DownloadObjectsOutput {
     }
 
     /// A slice of failed object transfers
-    ///
-    /// If no value was sent for this field, a default will be set. If you want to determine if no value was
-    /// set, use `.failed_transfers.is_none()`
     pub fn failed_transfers(&self) -> &[FailedDownloadTransfer] {
-        self.failed_transfers.as_deref().unwrap_or_default()
+        self.failed_transfers.as_slice()
     }
 
     /// The number of bytes successfully transferred (downloaded)
@@ -50,7 +47,7 @@ impl DownloadObjectsOutput {
 #[derive(Debug, Default)]
 pub struct DownloadObjectsOutputBuilder {
     pub(crate) objects_downloaded: u64,
-    pub(crate) failed_transfers: Option<Vec<FailedDownloadTransfer>>,
+    pub(crate) failed_transfers: Vec<FailedDownloadTransfer>,
     pub(crate) total_bytes_transferred: u64,
 }
 
@@ -71,21 +68,19 @@ impl DownloadObjectsOutputBuilder {
     /// To override the contents of this collection use
     /// [`set_failed_transfers`](Self::set_failed_transfers)
     pub fn failed_transfers(mut self, input: FailedDownloadTransfer) -> Self {
-        self.failed_transfers
-            .get_or_insert_with(Vec::new)
-            .push(input);
+        self.failed_transfers.push(input);
         self
     }
 
-    /// A list of failed object transfers
-    pub fn set_failed_transfers(mut self, input: Option<Vec<FailedDownloadTransfer>>) -> Self {
+    /// Set a list of failed object transfers
+    pub fn set_failed_transfers(mut self, input: Vec<FailedDownloadTransfer>) -> Self {
         self.failed_transfers = input;
         self
     }
 
-    /// A list of failed object transfers
-    pub fn get_failed_transfers(&self) -> &Option<Vec<FailedDownloadTransfer>> {
-        &self.failed_transfers
+    /// Get a list of failed object transfers
+    pub fn get_failed_transfers(&self) -> &[FailedDownloadTransfer] {
+        self.failed_transfers.as_slice()
     }
 
     /// The number of bytes successfully transferred (downloaded)

diff --git a/aws-s3-transfer-manager/src/operation/download_objects/worker.rs b/aws-s3-transfer-manager/src/operation/download_objects/worker.rs
@@ -111,17 +111,14 @@ pub(super) async fn download_objects(
                     // the tasks on error rather than relying on join and then drop.
                     FailedTransferPolicy::Abort => return Err(err),
                     FailedTransferPolicy::Continue => {
-                        let mut guard = ctx.state.failed_downloads.lock().unwrap();
-                        let mut failures = mem::take(&mut *guard).unwrap_or_default();
+                        let mut failures = ctx.state.failed_downloads.lock().unwrap();
 
                         let failed_transfer = FailedDownloadTransfer {
                             input: job.input(&ctx),
                             error: err,
                         };
 
                         failures.push(failed_transfer);
-
-                        let _ = mem::replace(&mut *guard, Some(failures));
                     }
                 }
             }

diff --git a/aws-s3-transfer-manager/src/operation/upload_objects.rs b/aws-s3-transfer-manager/src/operation/upload_objects.rs
@@ -63,11 +63,11 @@ impl UploadObjects {
     }
 }
 
-/// DownloadObjects operation specific state
+/// UploadObjects operation specific state
 #[derive(Debug)]
 pub(crate) struct UploadObjectsState {
     input: UploadObjectsInput,
-    failed_uploads: Mutex<Option<Vec<FailedUploadTransfer>>>,
+    failed_uploads: Mutex<Vec<FailedUploadTransfer>>,
     successful_uploads: AtomicU64,
     total_bytes_transferred: AtomicU64,
 }
@@ -78,7 +78,7 @@ impl UploadObjectsContext {
     fn new(handle: Arc<crate::client::Handle>, input: UploadObjectsInput) -> Self {
         let state = Arc::new(UploadObjectsState {
             input,
-            failed_uploads: Mutex::new(None),
+            failed_uploads: Mutex::new(Vec::new()),
             successful_uploads: AtomicU64::default(),
             total_bytes_transferred: AtomicU64::default(),
         });

diff --git a/aws-s3-transfer-manager/src/operation/upload_objects/handle.rs b/aws-s3-transfer-manager/src/operation/upload_objects/handle.rs
@@ -26,7 +26,7 @@ impl UploadObjectsHandle {
             join_result??;
         }
 
-        let failed_uploads = self.ctx.state.failed_uploads.lock().unwrap().take();
+        let failed_uploads = std::mem::take(&mut *self.ctx.state.failed_uploads.lock().unwrap());
         let successful_uploads = self.ctx.state.successful_uploads.load(Ordering::SeqCst);
         let total_bytes_transferred = self
             .ctx

diff --git a/aws-s3-transfer-manager/src/operation/upload_objects/output.rs b/aws-s3-transfer-manager/src/operation/upload_objects/output.rs
@@ -13,7 +13,7 @@ pub struct UploadObjectsOutput {
     objects_uploaded: u64,
 
     /// The list of failed uploads
-    failed_transfers: Option<Vec<FailedUploadTransfer>>,
+    failed_transfers: Vec<FailedUploadTransfer>,
 
     // FIXME - likely remove when progress is implemented (let's be consistent with downloads for now)?
     /// Total number of bytes transferred
@@ -33,7 +33,7 @@ impl UploadObjectsOutput {
 
     /// The list of failed uploads
     pub fn failed_transfers(&self) -> &[FailedUploadTransfer] {
-        self.failed_transfers.as_deref().unwrap_or_default()
+        self.failed_transfers.as_slice()
     }
 
     /// The number of bytes successfully transferred (uploaded)
@@ -47,7 +47,7 @@ impl UploadObjectsOutput {
 #[derive(Debug, Default)]
 pub struct UploadObjectsOutputBuilder {
     pub(crate) objects_uploaded: u64,
-    pub(crate) failed_transfers: Option<Vec<FailedUploadTransfer>>,
+    pub(crate) failed_transfers: Vec<FailedUploadTransfer>,
     pub(crate) total_bytes_transferred: u64,
 }
 
@@ -67,14 +67,12 @@ impl UploadObjectsOutputBuilder {
     ///
     /// To override the contents of this collection use [`set_failed_transfers`](Self::set_failed_transfers)
     pub fn failed_transfers(mut self, input: FailedUploadTransfer) -> Self {
-        self.failed_transfers
-            .get_or_insert_with(Vec::new)
-            .push(input);
+        self.failed_transfers.push(input);
         self
     }
 
-    /// The list of any failed uploads
-    pub fn set_failed_transfers(mut self, input: Option<Vec<FailedUploadTransfer>>) -> Self {
+    /// Set a list of failed uploads
+    pub fn set_failed_transfers(mut self, input: Vec<FailedUploadTransfer>) -> Self {
         self.failed_transfers = input;
         self
     }

diff --git a/aws-s3-transfer-manager/src/operation/upload_objects/worker.rs b/aws-s3-transfer-manager/src/operation/upload_objects/worker.rs
@@ -210,8 +210,7 @@ fn handle_failed_upload(
         // the tasks on error rather than relying on join and then drop.
         FailedTransferPolicy::Abort => Err(err),
         FailedTransferPolicy::Continue => {
-            let mut guard = ctx.state.failed_uploads.lock().unwrap();
-            let mut failures = std::mem::take(&mut *guard).unwrap_or_default();
+            let mut failures = ctx.state.failed_uploads.lock().unwrap();
 
             let failed_transfer = FailedUploadTransfer {
                 input: match object_key {
@@ -231,8 +230,6 @@ fn handle_failed_upload(
 
             failures.push(failed_transfer);
 
-            let _ = std::mem::replace(&mut *guard, Some(failures));
-
             Ok(())
         }
     }