From: Elias Rohrer <dev@tnull.de>
Date: Wed, 23 Aug 2023 11:54:37 +0000 (+0200)
Subject: f Cleanup lock handling
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=d660c1222ab22588174a3a4b91742e822767c17b;p=rust-lightning

f Cleanup lock handling
---

diff --git a/lightning-persister/src/fs_store.rs b/lightning-persister/src/fs_store.rs
index 2cb273f26..6cf54e2e9 100644
--- a/lightning-persister/src/fs_store.rs
+++ b/lightning-persister/src/fs_store.rs
@@ -70,8 +70,10 @@ impl KVStore for FilesystemStore {
 		dest_file_path.push(namespace);
 		dest_file_path.push(key);
 
-		let mut outer_lock = self.locks.lock().unwrap();
-		let inner_lock_ref = Arc::clone(&outer_lock.entry(dest_file_path.clone()).or_default());
+		let inner_lock_ref = {
+			let mut outer_lock = self.locks.lock().unwrap();
+			Arc::clone(&outer_lock.entry(dest_file_path.clone()).or_default())
+		};
 		let _guard = inner_lock_ref.read().unwrap();
 
 		let mut buf = Vec::new();
@@ -103,10 +105,6 @@ impl KVStore for FilesystemStore {
 		dest_file_path.push(namespace);
 		dest_file_path.push(key);
 
-		let mut outer_lock = self.locks.lock().unwrap();
-		let inner_lock_ref = Arc::clone(&outer_lock.entry(dest_file_path.clone()).or_default());
-		let _guard = inner_lock_ref.write().unwrap();
-
 		let parent_directory = dest_file_path
 			.parent()
 			.ok_or_else(|| {
@@ -132,6 +130,12 @@ impl KVStore for FilesystemStore {
 			tmp_file.sync_all()?;
 		}
 
+		let inner_lock_ref = {
+			let mut outer_lock = self.locks.lock().unwrap();
+			Arc::clone(&outer_lock.entry(dest_file_path.clone()).or_default())
+		};
+		let _guard = inner_lock_ref.write().unwrap();
+
 		#[cfg(not(target_os = "windows"))]
 		{
 			fs::rename(&tmp_file_path, &dest_file_path)?;
@@ -186,51 +190,47 @@ impl KVStore for FilesystemStore {
 		dest_file_path.push(namespace);
 		dest_file_path.push(key);
 
-		let mut outer_lock = self.locks.lock().unwrap();
-		let inner_lock_ref = Arc::clone(&outer_lock.entry(dest_file_path.clone()).or_default());
-
-		let _guard = inner_lock_ref.write().unwrap();
-
 		if !dest_file_path.is_file() {
 			return Ok(());
 		}
 
-		fs::remove_file(&dest_file_path)?;
-		#[cfg(not(target_os = "windows"))]
 		{
-			let parent_directory = dest_file_path.parent().ok_or_else(|| {
-				let msg =
-					format!("Could not retrieve parent directory of {}.", dest_file_path.display());
-				std::io::Error::new(std::io::ErrorKind::InvalidInput, msg)
-			})?;
-			let dir_file = fs::OpenOptions::new().read(true).open(parent_directory)?;
-			// The above call to `fs::remove_file` corresponds to POSIX `unlink`, whose changes
-			// to the inode might get cached (and hence possibly lost on crash), depending on
-			// the target platform and file system.
-			//
-			// In order to assert we permanently removed the file in question we therefore
-			// call `fsync` on the parent directory on platforms that support it,
-			dir_file.sync_all()?;
-		}
+			let inner_lock_ref = {
+				let mut outer_lock = self.locks.lock().unwrap();
+				Arc::clone(&outer_lock.entry(dest_file_path.clone()).or_default())
+			};
+			let _guard = inner_lock_ref.write().unwrap();
+
+			fs::remove_file(&dest_file_path)?;
+			#[cfg(not(target_os = "windows"))]
+			{
+				let parent_directory = dest_file_path.parent().ok_or_else(|| {
+					let msg =
+						format!("Could not retrieve parent directory of {}.", dest_file_path.display());
+					std::io::Error::new(std::io::ErrorKind::InvalidInput, msg)
+				})?;
+				let dir_file = fs::OpenOptions::new().read(true).open(parent_directory)?;
+				// The above call to `fs::remove_file` corresponds to POSIX `unlink`, whose changes
+				// to the inode might get cached (and hence possibly lost on crash), depending on
+				// the target platform and file system.
+				//
+				// In order to assert we permanently removed the file in question we therefore
+				// call `fsync` on the parent directory on platforms that support it,
+				dir_file.sync_all()?;
+			}
 
-		if dest_file_path.is_file() {
-			return Err(std::io::Error::new(std::io::ErrorKind::Other, "Removing key failed"));
+			if dest_file_path.is_file() {
+				return Err(std::io::Error::new(std::io::ErrorKind::Other, "Removing key failed"));
+			}
 		}
 
-		if Arc::strong_count(&inner_lock_ref) == 2 {
-			// It's safe to remove the lock entry if we're the only one left holding a strong
-			// reference. Checking this is necessary to ensure we continue to distribute references to the
-			// same lock as long as some Readers are around. However, we still want to
-			// clean up the table when possible.
-			//
-			// Note that this by itself is still leaky as lock entries will remain when more Readers/Writers are
-			// around, but is preferable to doing nothing *or* something overly complex such as
-			// implementing yet another RAII structure just for this pupose.
-			outer_lock.remove(&dest_file_path);
-		}
+		{
+			// Retake outer lock for the cleanup.
+			let mut outer_lock = self.locks.lock().unwrap();
 
-		// Garbage collect all lock entries that are not referenced anymore.
-		outer_lock.retain(|_, v| Arc::strong_count(&v) > 1);
+			// Garbage collect all lock entries that are not referenced anymore.
+			outer_lock.retain(|_, v| Arc::strong_count(&v) > 1);
+		}
 
 		Ok(())
 	}