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())