From 81c294356adb040a15fc7cce067a00a044387813 Mon Sep 17 00:00:00 2001 From: "Ericson \"Fogo\" Soares" Date: Thu, 27 Jul 2023 20:18:45 -0300 Subject: [PATCH] [ENG-794] Library deletion hangs (#1137) Solving deadlock on library deletion Also improving some minor stuff on thumbnails remover actor --- core/src/api/libraries.rs | 4 +++- core/src/library/manager.rs | 23 +++++++++++------- core/src/object/thumbnail_remover.rs | 35 ++++++++++++++++++++++++---- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/core/src/api/libraries.rs b/core/src/api/libraries.rs index fc1bbc681..3bc121cdc 100644 --- a/core/src/api/libraries.rs +++ b/core/src/api/libraries.rs @@ -116,7 +116,9 @@ pub(crate) fn mount() -> AlphaRouter { }) .procedure( "delete", - R.mutation(|ctx, id: Uuid| async move { Ok(ctx.library_manager.delete(id).await?) }), + R.mutation(|ctx, id: Uuid| async move { + ctx.library_manager.delete(id).await.map_err(Into::into) + }), ) // .yolo_merge("peer.guest.", peer_guest_router()) // .yolo_merge("peer.host.", peer_host_router()) diff --git a/core/src/library/manager.rs b/core/src/library/manager.rs index baac310d5..f243c4fc4 100644 --- a/core/src/library/manager.rs +++ b/core/src/library/manager.rs @@ -315,15 +315,18 @@ impl LibraryManager { } pub async fn delete(&self, id: Uuid) -> Result<(), LibraryManagerError> { - let libraries = self.libraries.read().await; + let mut libraries_write_guard = self.libraries.write().await; - let library = libraries + // As we're holding a write lock here, we know that our index can't change before removal. + let library_idx = libraries_write_guard .iter() - .find(|l| l.id == id) + .position(|l| l.id == id) .ok_or(LibraryManagerError::LibraryNotFound)?; - let db_path = self.libraries_dir.join(format!("{}.db", library.id)); - let sd_lib_path = self.libraries_dir.join(format!("{}.sdlibrary", library.id)); + let library_id = libraries_write_guard[library_idx].id; + + let db_path = self.libraries_dir.join(format!("{}.db", library_id)); + let sd_lib_path = self.libraries_dir.join(format!("{}.sdlibrary", library_id)); try_join!( async { @@ -338,10 +341,14 @@ impl LibraryManager { }, )?; - invalidate_query!(library, "library.list"); - self.thumbnail_remover.remove_library(id).await; - self.libraries.write().await.retain(|l| l.id != id); + + // We only remove here after files deletion + let library = libraries_write_guard.remove(library_idx); + + info!("Removed Library "); + + invalidate_query!(library, "library.list"); Ok(()) } diff --git a/core/src/object/thumbnail_remover.rs b/core/src/object/thumbnail_remover.rs index 830a95406..87a87e55f 100644 --- a/core/src/object/thumbnail_remover.rs +++ b/core/src/object/thumbnail_remover.rs @@ -19,13 +19,14 @@ use futures_concurrency::{future::TryJoin, stream::Merge}; use thiserror::Error; use tokio::{ fs, io, - time::{interval, MissedTickBehavior}, + time::{interval_at, Instant, MissedTickBehavior}, }; use tokio_stream::{wrappers::IntervalStream, StreamExt}; use tokio_util::sync::{CancellationToken, DropGuard}; use tracing::{debug, error, trace}; use uuid::Uuid; +const THIRTY_SECS: Duration = Duration::from_secs(30); const HALF_HOUR: Duration = Duration::from_secs(30 * 60); #[derive(Error, Debug)] @@ -65,6 +66,7 @@ impl ThumbnailRemoverActorProxy { } } +#[derive(Debug)] enum DatabaseMessage { Add(Uuid, Arc), Remove(Uuid), @@ -154,12 +156,13 @@ impl ThumbnailRemoverActor { non_indexed_thumbnails_cas_ids_rx: chan::Receiver, cancel_token: CancellationToken, ) { - let mut check_interval = interval(HALF_HOUR); + let mut check_interval = interval_at(Instant::now() + THIRTY_SECS, HALF_HOUR); check_interval.set_missed_tick_behavior(MissedTickBehavior::Skip); let mut databases = HashMap::new(); let mut non_indexed_thumbnails_cas_ids = HashSet::new(); + #[derive(Debug)] enum StreamMessage { Run, ToDelete(Vec), @@ -196,8 +199,12 @@ impl ThumbnailRemoverActor { } } StreamMessage::ToDelete(cas_ids) => { - if let Err(e) = Self::remove_by_cas_ids(&thumbnails_directory, cas_ids).await { - error!("Got an error when trying to remove thumbnails: {e:#?}"); + if !cas_ids.is_empty() { + if let Err(e) = + Self::remove_by_cas_ids(&thumbnails_directory, cas_ids).await + { + error!("Got an error when trying to remove thumbnails: {e:#?}"); + } } } @@ -317,6 +324,10 @@ impl ThumbnailRemoverActor { } if thumbnails_paths_by_cas_id.is_empty() { + trace!( + "Removing empty thumbnails sharding directory: {}", + entry_path.display() + ); fs::remove_dir(&entry_path) .await .map_err(|e| FileIOError::from((entry_path, e)))?; @@ -324,6 +335,8 @@ impl ThumbnailRemoverActor { continue; } + let thumbs_found = thumbnails_paths_by_cas_id.len(); + let mut thumbs_in_db_futs = databases .iter() .map(|db| { @@ -348,6 +361,8 @@ impl ThumbnailRemoverActor { thumbnails_paths_by_cas_id .retain(|cas_id, _| !non_indexed_thumbnails_cas_ids.contains(cas_id)); + let thumbs_to_remove = thumbnails_paths_by_cas_id.len(); + thumbnails_paths_by_cas_id .into_values() .map(|path| async move { @@ -359,6 +374,18 @@ impl ThumbnailRemoverActor { .collect::>() .try_join() .await?; + + if thumbs_to_remove == thumbs_found { + // if we removed all the thumnails we foumd, it means that the directory is empty + // and can be removed... + trace!( + "Removing empty thumbnails sharding directory: {}", + entry_path.display() + ); + fs::remove_dir(&entry_path) + .await + .map_err(|e| FileIOError::from((entry_path, e)))?; + } } Ok(())