Mercurial > public > mercurial-scm > hg
changeset 52766:549b58b1ce72
rust-inner-revlog: use threadlocals for file handles
In contexts where we access the same revlog from multiple threads
(like in `annotate`), multiple threads would lock the handle, then read from
a different place that they would expect, since the `seek` behavior of the
`InnerRevlog` was written with single threaded access in mind.
author | Rapha?l Gom?s <rgomes@octobus.net> |
---|---|
date | Fri, 24 Jan 2025 13:07:15 -0500 |
parents | 162f4801ad39 |
children | 6183949219b2 |
files | rust/hg-core/src/revlog/file_io.rs rust/hg-core/src/revlog/inner_revlog.rs |
diffstat | 2 files changed, 26 insertions(+), 46 deletions(-) [+] |
line wrap: on
line diff
--- a/rust/hg-core/src/revlog/file_io.rs Fri Jan 24 12:05:23 2025 -0500 +++ b/rust/hg-core/src/revlog/file_io.rs Fri Jan 24 13:07:15 2025 -0500 @@ -1,9 +1,10 @@ //! Helpers for revlog file reading and writing. use std::{ + cell::RefCell, io::{Read, Seek, SeekFrom, Write}, path::{Path, PathBuf}, - sync::{Arc, Mutex, RwLock}, + sync::{Arc, Mutex}, }; use crate::{ @@ -27,10 +28,15 @@ vfs: Box<dyn Vfs>, /// Filename of the open file, relative to the vfs root pub filename: PathBuf, - /// The current read-only handle on the file, if any - pub reading_handle: RwLock<Option<FileHandle>>, - /// The current read-write handle on the file, if any - pub writing_handle: RwLock<Option<FileHandle>>, + /// The current read-only handle on the file, if any. + /// Specific to the current thread, since we don't want seeks to overlap + pub reading_handle: thread_local::ThreadLocal<RefCell<Option<FileHandle>>>, + /// The current read-write handle on the file, if any. + /// Specific to the current thread, since we don't want seeks to overlap, + /// and we can re-use the write handle for reading in certain contexts. + /// Logically, two concurrent writes are impossible because they are only + /// accessible through `&mut self` methods, which take a lock. + pub writing_handle: thread_local::ThreadLocal<RefCell<Option<FileHandle>>>, } impl RandomAccessFile { @@ -40,8 +46,8 @@ Self { vfs, filename, - reading_handle: RwLock::new(None), - writing_handle: RwLock::new(None), + reading_handle: thread_local::ThreadLocal::new(), + writing_handle: thread_local::ThreadLocal::new(), } } @@ -61,9 +67,7 @@ /// `pub` only for hg-cpython #[doc(hidden)] pub fn get_read_handle(&self) -> Result<FileHandle, HgError> { - if let Some(handle) = - &*self.writing_handle.write().expect("lock is poisoned") - { + if let Some(handle) = &*self.writing_handle.get_or_default().borrow() { // Use a file handle being actively used for writes, if available. // There is some danger to doing this because reads will seek the // file. @@ -71,9 +75,7 @@ // before all writes, so we should be safe. return Ok(handle.clone()); } - if let Some(handle) = - &*self.reading_handle.read().expect("lock is poisoned") - { + if let Some(handle) = &*self.reading_handle.get_or_default().borrow() { return Ok(handle.clone()); } // early returns done to work around borrowck being overzealous @@ -84,7 +86,7 @@ false, false, )?; - *self.reading_handle.write().expect("lock is poisoned") = + *self.reading_handle.get_or_default().borrow_mut() = Some(new_handle.clone()); Ok(new_handle) } @@ -92,23 +94,13 @@ /// `pub` only for hg-cpython #[doc(hidden)] pub fn exit_reading_context(&self) { - self.reading_handle - .write() - .expect("lock is poisoned") - .take(); + self.reading_handle.get().map(|h| h.take()); } // Returns whether this file currently open pub fn is_open(&self) -> bool { - self.reading_handle - .read() - .expect("lock is poisoned") - .is_some() - || self - .writing_handle - .read() - .expect("lock is poisoned") - .is_some() + self.reading_handle.get_or_default().borrow().is_some() + || self.writing_handle.get_or_default().borrow().is_some() } }
--- a/rust/hg-core/src/revlog/inner_revlog.rs Fri Jan 24 12:05:23 2025 -0500 +++ b/rust/hg-core/src/revlog/inner_revlog.rs Fri Jan 24 13:07:15 2025 -0500 @@ -829,16 +829,8 @@ #[doc(hidden)] pub fn exit_writing_context(&mut self) { self.writing_handles.take(); - self.segment_file - .writing_handle - .write() - .expect("lock is poisoned") - .take(); - self.segment_file - .reading_handle - .write() - .expect("lock is poisoned") - .take(); + self.segment_file.writing_handle.get().map(|h| h.take()); + self.segment_file.reading_handle.get().map(|h| h.take()); } /// `pub` only for use in hg-cpython @@ -904,8 +896,8 @@ *self .segment_file .reading_handle - .write() - .expect("lock is poisoned") = if self.is_inline() { + .get_or_default() + .borrow_mut() = if self.is_inline() { Some(index_handle) } else { data_handle @@ -985,11 +977,7 @@ if let Some(handles) = &mut self.writing_handles { handles.index_handle.flush()?; self.writing_handles.take(); - self.segment_file - .writing_handle - .write() - .expect("lock is poisoned") - .take(); + self.segment_file.writing_handle.get().map(|h| h.take()); } let mut new_data_file_handle = self.vfs.create(&self.data_file, true)?; @@ -1058,8 +1046,8 @@ *self .segment_file .writing_handle - .write() - .expect("lock is poisoned") = new_data_handle; + .get_or_default() + .borrow_mut() = new_data_handle; } Ok(self.index_file.to_owned())