Merge pull request #145 from ikatson/delete-folders

More robust deletion of files and folders (fixes #140)

Makefile

@@ -17,7 +17,7 @@ webui-build: webui-deps
 
 @PHONY: devserver
 devserver:
-	echo -n '' > /tmp/rqbit-log && cargo run --release -- \
+	echo -n '' > /tmp/rqbit-log && cargo run -- \
 		--log-file /tmp/rqbit-log \
 		--log-file-rust-log=debug,librqbit=trace \
 		server start /tmp/scratch/

crates/librqbit/examples/custom_storage.rs

@@ -19,7 +19,7 @@ struct CustomStorage {
 impl StorageFactory for CustomStorageFactory {
     type Storage = CustomStorage;
 
-    fn init_storage(&self, _info: &librqbit::ManagedTorrentInfo) -> anyhow::Result<Self::Storage> {
+    fn create(&self, _info: &librqbit::ManagedTorrentInfo) -> anyhow::Result<Self::Storage> {
         Ok(CustomStorage::default())
     }
 
@@ -48,6 +48,14 @@ impl TorrentStorage for CustomStorage {
     fn take(&self) -> anyhow::Result<Box<dyn TorrentStorage>> {
         anyhow::bail!("not implemented")
     }
+
+    fn remove_directory_if_empty(&self, _path: &std::path::Path) -> anyhow::Result<()> {
+        anyhow::bail!("not implemented")
+    }
+
+    fn init(&mut self, _meta: &librqbit::ManagedTorrentInfo) -> anyhow::Result<()> {
+        anyhow::bail!("not implemented")
+    }
 }
 
 #[tokio::main]

crates/librqbit/src/session.rs

@@ -4,7 +4,7 @@ use std::{
     collections::{HashMap, HashSet},
     io::{BufReader, BufWriter, Read},
     net::SocketAddr,
-    path::PathBuf,
+    path::{Path, PathBuf},
     str::FromStr,
     sync::Arc,
     time::Duration,
@@ -16,11 +16,14 @@ use crate::{
     peer_connection::PeerConnectionOptions,
     read_buf::ReadBuf,
     spawn_utils::BlockingSpawner,
-    storage::{filesystem::FilesystemStorageFactory, BoxStorageFactory, StorageFactoryExt},
+    storage::{
+        filesystem::FilesystemStorageFactory, BoxStorageFactory, StorageFactoryExt, TorrentStorage,
+    },
     torrent_state::{
         ManagedTorrentBuilder, ManagedTorrentHandle, ManagedTorrentState, TorrentStateLive,
     },
     type_aliases::{DiskWorkQueueSender, PeerStream},
+    ManagedTorrentInfo,
 };
 use anyhow::{bail, Context};
 use bencode::{bencode_serialize_to_writer, BencodeDeserializer};
@@ -1136,31 +1139,48 @@ impl Session {
             .remove(&id)
             .with_context(|| format!("torrent with id {} did not exist", id))?;
 
-        let paused = removed
-            .with_state_mut(|s| {
-                let paused = match s.take() {
-                    ManagedTorrentState::Paused(p) => p,
-                    ManagedTorrentState::Live(l) => l.pause()?,
-                    _ => return Ok(None),
-                };
-                Ok::<_, anyhow::Error>(Some(paused))
+        if let Err(e) = removed.pause() {
+            debug!("error pausing torrent before deletion: {e:?}")
+        }
+
+        let storage = removed
+            .with_state_mut(|s| match s.take() {
+                ManagedTorrentState::Initializing(p) => p.files.take().ok(),
+                ManagedTorrentState::Paused(p) => Some(p.files),
+                ManagedTorrentState::Live(l) => l
+                    .pause()
+                    // inspect_err not available in 1.75
+                    .map_err(|e| {
+                        warn!("error pausing torrent: {e:?}");
+                        e
+                    })
+                    .ok()
+                    .map(|p| p.files),
+                _ => None,
             })
-            .context("error pausing torrent");
+            .map(Ok)
+            .unwrap_or_else(|| removed.storage_factory.create(removed.info()));
 
-        match (paused, delete_files) {
+        match (storage, delete_files) {
             (Err(e), true) => return Err(e).context("torrent deleted, but could not delete files"),
-            (Err(e), false) => {
-                warn!(error=?e, "error deleting torrent cleanly");
-            }
-            (Ok(Some(paused)), true) => {
-                for (id, fi) in removed.info().file_infos.iter().enumerate() {
-                    if let Err(e) = paused.files.remove_file(id, &fi.relative_filename) {
-                        warn!(?fi.relative_filename, error=?e, "could not delete file");
+            (Ok(storage), true) => {
+                debug!("will delete files");
+                remove_files_and_dirs(removed.info(), &storage);
+                if removed.info().options.output_folder != self.output_folder {
+                    if let Err(e) = storage.remove_directory_if_empty(Path::new("")) {
+                        warn!(
+                            "error removing {:?}: {e:?}",
+                            removed.info().options.output_folder
+                        )
                     }
                 }
             }
-            _ => {}
+            (_, false) => {
+                debug!("not deleting files")
+            }
         };
+
+        info!(id, "deleted torrent");
         Ok(())
     }
 
@@ -1220,6 +1240,37 @@ impl Session {
     }
 }
 
+fn remove_files_and_dirs(info: &ManagedTorrentInfo, files: &dyn TorrentStorage) {
+    let mut all_dirs = HashSet::new();
+    for (id, fi) in info.file_infos.iter().enumerate() {
+        let mut fname = &*fi.relative_filename;
+        if let Err(e) = files.remove_file(id, fname) {
+            warn!(?fi.relative_filename, error=?e, "could not delete file");
+        } else {
+            debug!(?fi.relative_filename, "deleted the file")
+        }
+        while let Some(parent) = fname.parent() {
+            if parent != Path::new("") {
+                all_dirs.insert(parent);
+            }
+            fname = parent;
+        }
+    }
+
+    let all_dirs = {
+        let mut v = all_dirs.into_iter().collect::<Vec<_>>();
+        v.sort_unstable_by_key(|p| std::cmp::Reverse(p.as_os_str().len()));
+        v
+    };
+    for dir in all_dirs {
+        if let Err(e) = files.remove_directory_if_empty(dir) {
+            warn!("error removing {dir:?}: {e:?}");
+        } else {
+            debug!("removed {dir:?}")
+        }
+    }
+}
+
 // Ad adapter for converting stats into the format that tracker_comms accepts.
 struct PeerRxTorrentInfo {
     info_hash: Id20,

crates/librqbit/src/storage/examples/inmemory.rs

@@ -25,7 +25,7 @@ pub struct InMemoryExampleStorageFactory {}
 impl StorageFactory for InMemoryExampleStorageFactory {
     type Storage = InMemoryExampleStorage;
 
-    fn init_storage(
+    fn create(
         &self,
         info: &crate::torrent_state::ManagedTorrentInfo,
     ) -> anyhow::Result<InMemoryExampleStorage> {
@@ -110,4 +110,12 @@ impl TorrentStorage for InMemoryExampleStorage {
             file_infos: self.file_infos.clone(),
         }))
     }
+
+    fn init(&mut self, _meta: &crate::ManagedTorrentInfo) -> anyhow::Result<()> {
+        Ok(())
+    }
+
+    fn remove_directory_if_empty(&self, _path: &Path) -> anyhow::Result<()> {
+        Ok(())
+    }
 }

crates/librqbit/src/storage/examples/mmap.rs

@@ -18,7 +18,7 @@ pub struct MmapStorage {
 impl StorageFactory for MmapStorageFactory {
     type Storage = MmapStorage;
 
-    fn init_storage(&self, info: &ManagedTorrentInfo) -> anyhow::Result<Self::Storage> {
+    fn create(&self, info: &ManagedTorrentInfo) -> anyhow::Result<Self::Storage> {
         Ok(MmapStorage {
             mmap: RwLock::new(
                 MmapOptions::new()
@@ -62,4 +62,12 @@ impl TorrentStorage for MmapStorage {
     fn take(&self) -> anyhow::Result<Box<dyn TorrentStorage>> {
         anyhow::bail!("not implemented")
     }
+
+    fn init(&mut self, _meta: &ManagedTorrentInfo) -> anyhow::Result<()> {
+        Ok(())
+    }
+
+    fn remove_directory_if_empty(&self, _path: &std::path::Path) -> anyhow::Result<()> {
+        Ok(())
+    }
 }

crates/librqbit/src/storage/filesystem/fs.rs

@@ -4,6 +4,7 @@ use std::{
 };
 
 use anyhow::Context;
+use tracing::warn;
 
 use crate::{storage::StorageFactoryExt, torrent_state::ManagedTorrentInfo};
 
@@ -17,45 +18,10 @@ pub struct FilesystemStorageFactory {}
 impl StorageFactory for FilesystemStorageFactory {
     type Storage = FilesystemStorage;
 
-    fn init_storage(&self, meta: &ManagedTorrentInfo) -> anyhow::Result<FilesystemStorage> {
-        let mut files = Vec::<OpenedFile>::new();
-        let output_folder = &meta.options.output_folder;
-        for file_details in meta.info.iter_file_details(&meta.lengths)? {
-            let mut full_path = output_folder.clone();
-            let relative_path = file_details
-                .filename
-                .to_pathbuf()
-                .context("error converting file to path")?;
-            full_path.push(relative_path);
-
-            std::fs::create_dir_all(full_path.parent().context("bug: no parent")?)?;
-            let file = if meta.options.allow_overwrite {
-                OpenOptions::new()
-                    .create(true)
-                    .truncate(false)
-                    .read(true)
-                    .write(true)
-                    .open(&full_path)
-                    .with_context(|| format!("error opening {full_path:?} in read/write mode"))?
-            } else {
-                // create_new does not seem to work with read(true), so calling this twice.
-                OpenOptions::new()
-                    .create_new(true)
-                    .write(true)
-                    .open(&full_path)
-                    .with_context(|| {
-                        format!(
-                            "error creating a new file (because allow_overwrite = false) {:?}",
-                            &full_path
-                        )
-                    })?;
-                OpenOptions::new().read(true).write(true).open(&full_path)?
-            };
-            files.push(OpenedFile::new(file));
-        }
+    fn create(&self, meta: &ManagedTorrentInfo) -> anyhow::Result<FilesystemStorage> {
         Ok(FilesystemStorage {
-            output_folder: output_folder.clone(),
-            opened_files: files,
+            output_folder: meta.options.output_folder.clone(),
+            opened_files: Default::default(),
         })
     }
 
@@ -142,4 +108,57 @@ impl TorrentStorage for FilesystemStorage {
             output_folder: self.output_folder.clone(),
         }))
     }
+
+    fn remove_directory_if_empty(&self, path: &Path) -> anyhow::Result<()> {
+        let path = self.output_folder.join(path);
+        if !path.is_dir() {
+            anyhow::bail!("cannot remove dir: {path:?} is not a directory")
+        }
+        if std::fs::read_dir(&path)?.count() == 0 {
+            std::fs::remove_dir(&path).with_context(|| format!("error removing {path:?}"))
+        } else {
+            warn!("did not remove {path:?} as it was not empty");
+            Ok(())
+        }
+    }
+
+    fn init(&mut self, meta: &ManagedTorrentInfo) -> anyhow::Result<()> {
+        let mut files = Vec::<OpenedFile>::new();
+        for file_details in meta.info.iter_file_details(&meta.lengths)? {
+            let mut full_path = self.output_folder.clone();
+            let relative_path = file_details
+                .filename
+                .to_pathbuf()
+                .context("error converting file to path")?;
+            full_path.push(relative_path);
+
+            std::fs::create_dir_all(full_path.parent().context("bug: no parent")?)?;
+            let file = if meta.options.allow_overwrite {
+                OpenOptions::new()
+                    .create(true)
+                    .truncate(false)
+                    .read(true)
+                    .write(true)
+                    .open(&full_path)
+                    .with_context(|| format!("error opening {full_path:?} in read/write mode"))?
+            } else {
+                // create_new does not seem to work with read(true), so calling this twice.
+                OpenOptions::new()
+                    .create_new(true)
+                    .write(true)
+                    .open(&full_path)
+                    .with_context(|| {
+                        format!(
+                            "error creating a new file (because allow_overwrite = false) {:?}",
+                            &full_path
+                        )
+                    })?;
+                OpenOptions::new().read(true).write(true).open(&full_path)?
+            };
+            files.push(OpenedFile::new(file));
+        }
+
+        self.opened_files = files;
+        Ok(())
+    }
 }

crates/librqbit/src/storage/filesystem/mmap.rs

@@ -22,19 +22,11 @@ fn dummy_mmap() -> anyhow::Result<MmapMut> {
 impl StorageFactory for MmapFilesystemStorageFactory {
     type Storage = MmapFilesystemStorage;
 
-    fn init_storage(&self, meta: &ManagedTorrentInfo) -> anyhow::Result<Self::Storage> {
-        let fs_storage = FilesystemStorageFactory::default().init_storage(meta)?;
-        let mut mmaps = Vec::new();
-        for (idx, file) in fs_storage.opened_files.iter().enumerate() {
-            let fg = file.file.write();
-            fg.set_len(meta.file_infos[idx].len)
-                .context("mmap storage: error setting length")?;
-            let mmap = unsafe { MmapOptions::new().map_mut(&*fg) }.context("error mapping file")?;
-            mmaps.push(RwLock::new(mmap));
-        }
+    fn create(&self, meta: &ManagedTorrentInfo) -> anyhow::Result<Self::Storage> {
+        let fs_storage = FilesystemStorageFactory::default().create(meta)?;
 
         Ok(MmapFilesystemStorage {
-            opened_mmaps: mmaps,
+            opened_mmaps: Vec::new(),
             fs: fs_storage,
         })
     }
@@ -82,6 +74,10 @@ impl TorrentStorage for MmapFilesystemStorage {
         self.fs.remove_file(file_id, filename)
     }
 
+    fn remove_directory_if_empty(&self, path: &Path) -> anyhow::Result<()> {
+        self.fs.remove_directory_if_empty(path)
+    }
+
     fn ensure_file_length(&self, file_id: usize, len: u64) -> anyhow::Result<()> {
         self.fs.ensure_file_length(file_id, len)
     }
@@ -100,4 +96,19 @@ impl TorrentStorage for MmapFilesystemStorage {
             fs: self.fs.take_fs()?,
         }))
     }
+
+    fn init(&mut self, meta: &ManagedTorrentInfo) -> anyhow::Result<()> {
+        self.fs.init(meta)?;
+        let mut mmaps = Vec::new();
+        for (idx, file) in self.fs.opened_files.iter().enumerate() {
+            let fg = file.file.write();
+            fg.set_len(meta.file_infos[idx].len)
+                .context("mmap storage: error setting length")?;
+            let mmap = unsafe { MmapOptions::new().map_mut(&*fg) }.context("error mapping file")?;
+            mmaps.push(RwLock::new(mmap));
+        }
+
+        self.opened_mmaps = mmaps;
+        Ok(())
+    }
 }