fix(storage): forward on_piece_completed through the storage middlewares

crates/librqbit/src/storage/middleware/slow.rs

@@ -12,6 +12,7 @@ use std::{
     time::Duration,
 };
 
+use librqbit_core::lengths::ValidPieceIndex;
 use parking_lot::Mutex;
 
 use crate::{
@@ -128,4 +129,24 @@ impl<U: TorrentStorage> TorrentStorage for SlowStorage<U> {
     ) -> anyhow::Result<()> {
         self.underlying.init(shared, metadata)
     }
+
+    fn on_piece_completed(&self, piece_index: ValidPieceIndex) -> anyhow::Result<()> {
+        self.underlying.on_piece_completed(piece_index)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::storage::test_util::{Probe, assert_forwards_defaults};
+
+    #[test]
+    fn test_the_defaulted_methods_reach_the_underlying_storage() {
+        let storage = SlowStorage {
+            underlying: Probe::default(),
+            pwrite_all_bufread: Mutex::new(Box::new(std::iter::empty())),
+            pread_exact_bufread: Mutex::new(Box::new(std::iter::empty())),
+        };
+        assert_forwards_defaults(&storage, &storage.underlying);
+    }
 }

crates/librqbit/src/storage/middleware/timing.rs

@@ -2,6 +2,8 @@
 A storage middleware that logs the time underlying storage operations took.
 */
 
+use librqbit_core::lengths::ValidPieceIndex;
+
 use crate::{
     ManagedTorrentShared,
     storage::{StorageFactory, StorageFactoryExt, TorrentStorage},
@@ -116,4 +118,23 @@ impl<U: TorrentStorage> TorrentStorage for TimingStorage<U> {
     ) -> anyhow::Result<()> {
         self.underlying.init(shared, metadata)
     }
+
+    fn on_piece_completed(&self, piece_index: ValidPieceIndex) -> anyhow::Result<()> {
+        self.underlying.on_piece_completed(piece_index)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::storage::test_util::{Probe, assert_forwards_defaults};
+
+    #[test]
+    fn test_the_defaulted_methods_reach_the_underlying_storage() {
+        let storage = TimingStorage {
+            name: "test".to_owned(),
+            underlying: Probe::default(),
+        };
+        assert_forwards_defaults(&storage, &storage.underlying);
+    }
 }

crates/librqbit/src/storage/middleware/write_through_cache.rs

@@ -137,4 +137,33 @@ impl<U: TorrentStorage> TorrentStorage for WriteThroughCacheStorage<U> {
     ) -> anyhow::Result<()> {
         self.underlying.init(shared, metadata)
     }
+
+    fn on_piece_completed(&self, piece_index: ValidPieceIndex) -> anyhow::Result<()> {
+        self.underlying.on_piece_completed(piece_index)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::storage::test_util::{
+        NUM_PIECES, PIECE_LEN, Probe, assert_forwards_defaults, lengths,
+    };
+
+    #[test]
+    fn test_the_defaulted_methods_reach_the_underlying_storage() {
+        let storage = WriteThroughCacheStorage {
+            lru: RwLock::new(LruCache::new(NonZeroUsize::new(1).unwrap())),
+            lengths: lengths(),
+            file_infos: vec![crate::file_info::FileInfo {
+                relative_filename: "test.dat".into(),
+                offset_in_torrent: 0,
+                len: PIECE_LEN as u64 * NUM_PIECES as u64,
+                piece_range: 0..NUM_PIECES,
+                attrs: Default::default(),
+            }],
+            underlying: Probe::default(),
+        };
+        assert_forwards_defaults(&storage, &storage.underlying);
+    }
 }

crates/librqbit/src/storage/mod.rs

@@ -30,6 +30,9 @@ pub mod examples;
 #[cfg(feature = "storage_middleware")]
 pub mod middleware;
 
+#[cfg(test)]
+pub(crate) mod test_util;
+
 use std::{
     any::{Any, TypeId},
     io::IoSlice,
@@ -222,7 +225,9 @@ mod tests {
     use std::any::TypeId;
 
     use super::{
-        BoxStorageFactory, StorageFactory, StorageFactoryExt, filesystem::FilesystemStorageFactory,
+        BoxStorageFactory, StorageFactory, StorageFactoryExt,
+        filesystem::FilesystemStorageFactory,
+        test_util::{Probe, assert_forwards_defaults},
     };
     use crate::torrent_state::{ManagedTorrentShared, TorrentMetadata};
 
@@ -279,4 +284,12 @@ mod tests {
             .is_type_id(TypeId::of::<Middleware<FilesystemStorageFactory>>())
         );
     }
+
+    // What a torrent holds is a Box<dyn TorrentStorage>, so a method this impl doesn't
+    // forward is one no storage in rqbit ever gets asked.
+    #[test]
+    fn test_the_defaulted_methods_survive_boxing() {
+        let boxed: Box<Probe> = Box::default();
+        assert_forwards_defaults(&boxed, &boxed);
+    }
 }

crates/librqbit/src/storage/test_util.rs

@@ -0,0 +1,84 @@
+// A storage that records what reached it, to check the wrappers around one against.
+//
+// A middleware is transparent: anything it has no opinion about has to reach what it
+// wraps. The methods that make that easy to get wrong are the ones [`TorrentStorage`]
+// gives a default, because forgetting one still compiles and then answers something
+// plausible of its own instead of asking the storage underneath.
+
+use std::path::Path;
+
+use librqbit_core::{
+    constants::CHUNK_SIZE,
+    lengths::{Lengths, ValidPieceIndex},
+};
+use parking_lot::Mutex;
+
+use crate::{ManagedTorrentShared, TorrentMetadata, storage::TorrentStorage};
+
+pub(crate) const PIECE_LEN: u32 = CHUNK_SIZE * 2;
+pub(crate) const NUM_PIECES: u32 = 2;
+
+pub(crate) fn lengths() -> Lengths {
+    Lengths::new(PIECE_LEN as u64 * NUM_PIECES as u64, PIECE_LEN).unwrap()
+}
+
+pub(crate) fn piece(id: u32) -> ValidPieceIndex {
+    lengths().validate_piece_index(id).unwrap()
+}
+
+#[derive(Default)]
+pub(crate) struct Probe {
+    /// The pieces on_piece_completed() was called for, in order.
+    pub completed: Mutex<Vec<u32>>,
+}
+
+impl TorrentStorage for Probe {
+    fn init(
+        &mut self,
+        _shared: &ManagedTorrentShared,
+        _metadata: &TorrentMetadata,
+    ) -> anyhow::Result<()> {
+        Ok(())
+    }
+
+    fn pread_exact(&self, _file_id: usize, _offset: u64, _buf: &mut [u8]) -> anyhow::Result<()> {
+        Ok(())
+    }
+
+    fn pwrite_all(&self, _file_id: usize, _offset: u64, _buf: &[u8]) -> anyhow::Result<()> {
+        Ok(())
+    }
+
+    fn remove_file(&self, _file_id: usize, _filename: &Path) -> anyhow::Result<()> {
+        Ok(())
+    }
+
+    fn remove_directory_if_empty(&self, _path: &Path) -> anyhow::Result<()> {
+        Ok(())
+    }
+
+    fn ensure_file_length(&self, _file_id: usize, _length: u64) -> anyhow::Result<()> {
+        Ok(())
+    }
+
+    fn take(&self) -> anyhow::Result<Box<dyn TorrentStorage>> {
+        anyhow::bail!("not used")
+    }
+
+    fn on_piece_completed(&self, piece_index: ValidPieceIndex) -> anyhow::Result<()> {
+        self.completed.lock().push(piece_index.get());
+        Ok(())
+    }
+}
+
+// Everything a wrapper has to pass through to the storage it wraps and can't decide on
+// its own. Called from each middleware's own tests, where its private fields are.
+pub(crate) fn assert_forwards_defaults<S: TorrentStorage>(storage: &S, probe: &Probe) {
+    storage.on_piece_completed(piece(1)).unwrap();
+    assert_eq!(
+        *probe.completed.lock(),
+        vec![1],
+        "on_piece_completed() didn't reach the storage: a store that makes a piece \
+         visible there never gets told"
+    );
+}