refactor: 🔒 Narrow scope of usage of read and write locks to forest and file storage

Author: ffarall

client/src/tasks/bsp_download_file.rs

@@ -68,21 +68,26 @@ where
             chunk_ids,
             request_id,
             bucket_id,
+            file_key,
             ..
         } = event;
 
-        // Get the file metadata from the file storage.
-        let file_storage_read_lock = self.storage_hub_handler.file_storage.read().await;
-        let file_metadata = file_storage_read_lock
-            .get_metadata(&event.file_key.into())
-            .map_err(|_| anyhow::anyhow!("Failed to get file metadata"))?;
+        let file_key_hash = file_key.into();
 
-        // If the file metadata is not found, return an error.
-        let file_metadata = if let Some(file_metadata) = file_metadata {
-            file_metadata
-        } else {
-            error!(target: LOG_TARGET, "File not found in file storage");
-            return Err(anyhow::anyhow!("File not found in file storage"));
+        // Get the file metadata from the file storage within a short-lived read lock.
+        let file_metadata = {
+            let file_storage_read_lock = self.storage_hub_handler.file_storage.read().await;
+            let file_metadata = file_storage_read_lock
+                .get_metadata(&file_key_hash)
+                .map_err(|_| anyhow::anyhow!("Failed to get file metadata"))?;
+
+            // If the file metadata is not found, return an error.
+            if let Some(file_metadata) = file_metadata {
+                file_metadata
+            } else {
+                error!(target: LOG_TARGET, "File not found in file storage");
+                return Err(anyhow::anyhow!("File not found in file storage"));
+            }
         };
 
         // If we have a bucket ID in the request, check if the file bucket matches the bucket ID in
@@ -92,15 +97,20 @@ where
                 error!(
                     target: LOG_TARGET,
                     "File bucket mismatch for file {:?}: expected {:?}, got {:?}",
-                    event.file_key, file_metadata.bucket_id(), bucket_id
+                    file_key,
+                    file_metadata.bucket_id(),
+                    bucket_id
                 );
                 return Err(anyhow::anyhow!("File bucket mismatch"));
             }
         }
 
-        // Generate the proof for the chunk (which also contains the chunk data itself).
-        let generate_proof_result =
-            file_storage_read_lock.generate_proof(&event.file_key.into(), &chunk_ids);
+        // Generate the proof for the chunk (which also contains the chunk data itself) within a
+        // short-lived read lock, then drop the lock before performing any network I/O.
+        let generate_proof_result = {
+            let file_storage_read_lock = self.storage_hub_handler.file_storage.read().await;
+            file_storage_read_lock.generate_proof(&file_key_hash, &chunk_ids)
+        };
 
         match generate_proof_result {
             Ok(file_key_proof) => {
@@ -118,7 +128,7 @@ where
 
         Ok(format!(
             "Handled RemoteDownloadRequest [{:?}] for file [{:x}]",
-            request_id, event.file_key
+            request_id, file_key
         ))
     }
 }

client/src/tasks/bsp_upload_file.rs

@@ -341,6 +341,7 @@ where
         let read_file_storage = self.storage_hub_handler.file_storage.read().await;
         let mut file_keys_and_proofs = Vec::new();
         let mut file_metadatas = HashMap::new();
+        let mut requests_to_retry = Vec::new();
         for (confirm_storing_request, chunks_to_prove) in
             confirm_storing_requests_with_chunks_to_prove.into_iter()
         {
@@ -365,17 +366,22 @@ where
                         error!(target: LOG_TARGET, "Failed to generate proof or get metadatas for file {:?}.\nMax try count exceeded! Dropping request!", confirm_storing_request.file_key);
                     } else {
                         error!(target: LOG_TARGET, "Failed to generate proof or get metadatas for file {:?}.\nEnqueuing file key again! (retry {}/{})", confirm_storing_request.file_key, confirm_storing_request.try_count, self.config.max_try_count);
-                        self.storage_hub_handler
-                            .blockchain
-                            .queue_confirm_bsp_request(confirm_storing_request)
-                            .await?;
+                        requests_to_retry.push(confirm_storing_request);
                     }
                 }
             }
         }
         // Release the file storage read lock as soon as possible.
         drop(read_file_storage);
 
+        // Re-enqueue any requests that we could not process, now that the file storage lock is dropped.
+        for confirm_storing_request in requests_to_retry {
+            self.storage_hub_handler
+                .blockchain
+                .queue_confirm_bsp_request(confirm_storing_request)
+                .await?;
+        }
+
         if file_keys_and_proofs.is_empty() {
             error!(target: LOG_TARGET, "Failed to generate proofs for ALL the requested files.\n");
             return Err(anyhow!(

client/src/tasks/msp_upload_file.rs

@@ -305,10 +305,11 @@ where
 
         let mut file_key_responses = HashMap::new();
 
-        let read_file_storage = self.storage_hub_handler.file_storage.read().await;
-
         for respond in &event.data.respond_storing_requests {
             info!(target: LOG_TARGET, "Processing respond storing request.");
+
+            let read_file_storage = self.storage_hub_handler.file_storage.read().await;
+
             let bucket_id = match read_file_storage.get_metadata(&respond.file_key) {
                 Ok(Some(metadata)) => H256::from_slice(metadata.bucket_id().as_ref()),
                 Ok(None) => {
@@ -364,8 +365,6 @@ where
             }
         }
 
-        drop(read_file_storage);
-
         let mut storage_request_msp_response = Vec::new();
 
         for (bucket_id, (accept, reject)) in file_key_responses.iter_mut() {
@@ -577,11 +576,13 @@ where
             .forest_storage_handler
             .get_or_create(&ForestStorageKey::from(event.bucket_id.as_ref().to_vec()))
             .await;
-        let read_fs = fs.read().await;
 
         // If we do not have the file already in forest storage, we must take into account the
         // available storage capacity.
-        let file_in_forest_storage = read_fs.contains_file_key(&file_key.into())?;
+        let file_in_forest_storage = {
+            let read_fs = fs.read().await;
+            read_fs.contains_file_key(&file_key.into())?
+        };
         if !file_in_forest_storage {
             info!(target: LOG_TARGET, "File key [{:x}] not found in forest storage. Checking available storage capacity.", file_key);