From 5610320fca24381ed293580aa795f51915de4c94 Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Sun, 26 Nov 2023 22:48:09 +0100 Subject: [PATCH 1/2] Make CompoundFile implement Send + Sync. This was done by refactoring code to use Arc instead of Rc. --- src/internal/entry.rs | 18 ++++++++---------- src/internal/stream.rs | 28 ++++++++++++++++------------ src/lib.rs | 21 ++++++++++----------- 3 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/internal/entry.rs b/src/internal/entry.rs index 5475cfb..14637cf 100644 --- a/src/internal/entry.rs +++ b/src/internal/entry.rs @@ -1,8 +1,7 @@ use crate::internal::{consts, DirEntry, MiniAllocator, ObjType, Timestamp}; -use std::cell::RefCell; use std::fmt; use std::path::{Path, PathBuf}; -use std::rc::Rc; +use std::sync::{Arc, RwLock}; use std::time::SystemTime; use uuid::Uuid; @@ -125,14 +124,14 @@ pub struct Entries<'a, F: 'a> { // a reference to the Rc. That would allow e.g. opening streams during // iteration. But we'd need to think about how the iterator should behave // if the CFB tree structure is modified during iteration. - minialloc: &'a Rc>>, + minialloc: &'a Arc>>, stack: Vec<(PathBuf, u32, bool)>, } impl<'a, F> Entries<'a, F> { pub(crate) fn new( order: EntriesOrder, - minialloc: &'a Rc>>, + minialloc: &'a Arc>>, parent_path: PathBuf, start: u32, ) -> Entries<'a, F> { @@ -149,7 +148,7 @@ impl<'a, F> Entries<'a, F> { } fn stack_left_spine(&mut self, parent_path: &Path, mut current_id: u32) { - let minialloc = self.minialloc.borrow(); + let minialloc = self.minialloc.read().unwrap(); while current_id != consts::NO_STREAM { self.stack.push((parent_path.to_path_buf(), current_id, true)); current_id = minialloc.dir_entry(current_id).left_sibling; @@ -162,7 +161,7 @@ impl<'a, F> Iterator for Entries<'a, F> { fn next(&mut self) -> Option { if let Some((parent, stream_id, visit_siblings)) = self.stack.pop() { - let minialloc = self.minialloc.borrow(); + let minialloc = self.minialloc.read().unwrap(); let dir_entry = minialloc.dir_entry(stream_id); let path = join_path(&parent, dir_entry); if visit_siblings { @@ -201,9 +200,8 @@ mod tests { Allocator, DirEntry, Directory, MiniAllocator, ObjType, Sectors, Timestamp, Validation, Version, }; - use std::cell::RefCell; use std::path::{Path, PathBuf}; - use std::rc::Rc; + use std::sync::{Arc, RwLock}; fn make_entry( name: &str, @@ -219,7 +217,7 @@ mod tests { dir_entry } - fn make_minialloc() -> Rc>> { + fn make_minialloc() -> Arc>> { // Root contains: 3 contains: // 5 8 // / \ / \ @@ -261,7 +259,7 @@ mod tests { Validation::Strict, ) .unwrap(); - Rc::new(RefCell::new(minialloc)) + Arc::new(RwLock::new(minialloc)) } fn paths_for_entries(entries: &[Entry]) -> Vec<&Path> { diff --git a/src/internal/stream.rs b/src/internal/stream.rs index bf2bdde..2e79151 100644 --- a/src/internal/stream.rs +++ b/src/internal/stream.rs @@ -1,7 +1,6 @@ use crate::internal::{consts, MiniAllocator, ObjType, SectorInit}; -use std::cell::RefCell; use std::io::{self, BufRead, Read, Seek, SeekFrom, Write}; -use std::rc::{Rc, Weak}; +use std::sync::{Arc, RwLock, Weak}; //===========================================================================// @@ -11,7 +10,7 @@ const BUFFER_SIZE: usize = 8192; /// A stream entry in a compound file, much like a filesystem file. pub struct Stream { - minialloc: Weak>>, + minialloc: Weak>>, stream_id: u32, total_len: u64, buffer: Box<[u8; BUFFER_SIZE]>, @@ -23,12 +22,13 @@ pub struct Stream { impl Stream { pub(crate) fn new( - minialloc: &Rc>>, + minialloc: &Arc>>, stream_id: u32, ) -> Stream { - let total_len = minialloc.borrow().dir_entry(stream_id).stream_len; + let total_len = + minialloc.read().unwrap().dir_entry(stream_id).stream_len; Stream { - minialloc: Rc::downgrade(minialloc), + minialloc: Arc::downgrade(minialloc), stream_id, total_len, buffer: Box::new([0; BUFFER_SIZE]), @@ -39,7 +39,7 @@ impl Stream { } } - fn minialloc(&self) -> io::Result>>> { + fn minialloc(&self) -> io::Result>>> { self.minialloc.upgrade().ok_or_else(|| { io::Error::new(io::ErrorKind::Other, "CompoundFile was dropped") }) @@ -83,7 +83,11 @@ impl Stream { let new_position = self.current_position().min(size); self.flush_changes()?; let minialloc = self.minialloc()?; - resize_stream(&mut minialloc.borrow_mut(), self.stream_id, size)?; + resize_stream( + &mut minialloc.write().unwrap(), + self.stream_id, + size, + )?; self.total_len = size; self.buf_offset_from_start = new_position; self.buf_pos = 0; @@ -110,7 +114,7 @@ impl BufRead for Stream { self.buf_pos = 0; let minialloc = self.minialloc()?; self.buf_cap = read_data_from_stream( - &mut minialloc.borrow_mut(), + &mut minialloc.write().unwrap(), self.stream_id, self.buf_offset_from_start, &mut self.buffer[..], @@ -234,7 +238,7 @@ impl Write for Stream { fn flush(&mut self) -> io::Result<()> { self.flush_changes()?; let minialloc = self.minialloc()?; - minialloc.borrow_mut().flush()?; + minialloc.write().unwrap().flush()?; Ok(()) } } @@ -257,13 +261,13 @@ impl Flusher for FlushBuffer { fn flush_changes(&self, stream: &mut Stream) -> io::Result<()> { let minialloc = stream.minialloc()?; write_data_to_stream( - &mut minialloc.borrow_mut(), + &mut minialloc.write().unwrap(), stream.stream_id, stream.buf_offset_from_start, &stream.buffer[..stream.buf_cap], )?; debug_assert_eq!( - minialloc.borrow().dir_entry(stream.stream_id).stream_len, + minialloc.read().unwrap().dir_entry(stream.stream_id).stream_len, stream.total_len ); Ok(()) diff --git a/src/lib.rs b/src/lib.rs index 58a7fc8..6ac69bd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -45,13 +45,12 @@ #![warn(missing_docs)] -use std::cell::{Ref, RefCell, RefMut}; use std::fmt; use std::fs; use std::io::{self, Read, Seek, SeekFrom, Write}; use std::mem::size_of; use std::path::{Path, PathBuf}; -use std::rc::Rc; +use std::sync::{Arc, RwLock, RwLockReadGuard, RwLockWriteGuard}; use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt}; use fnv::FnvHashSet; @@ -108,16 +107,16 @@ fn create_with_path(path: &Path) -> io::Result> { /// [`File`](https://doc.rust-lang.org/std/fs/struct.File.html) or /// [`Cursor`](https://doc.rust-lang.org/std/io/struct.Cursor.html)). pub struct CompoundFile { - minialloc: Rc>>, + minialloc: Arc>>, } impl CompoundFile { - fn minialloc(&self) -> Ref> { - self.minialloc.borrow() + fn minialloc(&self) -> RwLockReadGuard> { + self.minialloc.read().unwrap() } - fn minialloc_mut(&mut self) -> RefMut> { - self.minialloc.borrow_mut() + fn minialloc_mut(&mut self) -> RwLockWriteGuard> { + self.minialloc.write().unwrap() } /// Returns the CFB format version used for this compound file. @@ -291,8 +290,8 @@ impl CompoundFile { // We only ever retain Weak copies of the CompoundFile's minialloc Rc // (e.g. in Stream structs), so the Rc::try_unwrap() should always // succeed. - match Rc::try_unwrap(self.minialloc) { - Ok(ref_cell) => ref_cell.into_inner().into_inner(), + match Arc::try_unwrap(self.minialloc) { + Ok(ref_cell) => ref_cell.into_inner().unwrap().into_inner(), Err(_) => unreachable!(), } } @@ -557,7 +556,7 @@ impl CompoundFile { validation, )?; - Ok(CompoundFile { minialloc: Rc::new(RefCell::new(minialloc)) }) + Ok(CompoundFile { minialloc: Arc::new(RwLock::new(minialloc)) }) } } @@ -635,7 +634,7 @@ impl CompoundFile { consts::END_OF_CHAIN, Validation::Strict, )?; - Ok(CompoundFile { minialloc: Rc::new(RefCell::new(minialloc)) }) + Ok(CompoundFile { minialloc: Arc::new(RwLock::new(minialloc)) }) } /// Creates a new, empty storage object (i.e. "directory") at the provided From b6d8359840dabea6d4a2f7646f8745b8b98a986c Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Sun, 26 Nov 2023 22:52:31 +0100 Subject: [PATCH 2/2] Add tests for asserting Send + Sync in CompoundFile --- tests/basic.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/basic.rs b/tests/basic.rs index 75434bf..24fd5d9 100644 --- a/tests/basic.rs +++ b/tests/basic.rs @@ -769,3 +769,18 @@ fn drop_compound_file_with_stream_open() -> io::Result<()> { } //===========================================================================// +// Tests for asserting Send + Sync: + +#[test] +fn test_compound_file_send() { + fn assert_send() {} + assert_send::>(); +} + +#[test] +fn test_compound_file_sync() { + fn assert_sync() {} + assert_sync::>(); +} + +//===========================================================================//