From 2f90166a69300f413c5f96e34dd74595683bd1ce Mon Sep 17 00:00:00 2001 From: Roland Sherwin Date: Thu, 5 Dec 2024 03:15:50 +0530 Subject: [PATCH] fix(bootstrap): use atomic write crate and remove locks --- Cargo.lock | 34 ++++++---- ant-bootstrap/Cargo.toml | 7 +- ant-bootstrap/src/cache_store.rs | 107 ++++++------------------------- ant-bootstrap/src/error.rs | 2 - 4 files changed, 46 insertions(+), 104 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f5eb4ca627..f9324659bb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -728,10 +728,10 @@ version = "0.1.0" dependencies = [ "ant-logging", "ant-protocol", + "atomic-write-file", "chrono", "clap", "dirs-next", - "fs2", "futures", "libp2p 0.54.1 (registry+https://github.com/rust-lang/crates.io-index)", "reqwest 0.12.9", @@ -1476,6 +1476,16 @@ dependencies = [ "pin-project-lite", ] +[[package]] +name = "atomic-write-file" +version = "0.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23e32862ecc63d580f4a5e1436a685f51e0629caeb7a7933e4f017d5e2099e13" +dependencies = [ + "nix 0.29.0", + "rand 0.8.5", +] + [[package]] name = "attohttpc" version = "0.24.1" @@ -3558,16 +3568,6 @@ version = "2.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6c2141d6d6c8512188a7891b4b01590a45f6dac67afb4f255c4124dbb86d4eaa" -[[package]] -name = "fs2" -version = "0.4.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9564fc758e15025b46aa6643b1b77d047d1a56a1aea6e01002ac0c7026876213" -dependencies = [ - "libc", - "winapi", -] - [[package]] name = "fs_extra" version = "1.3.0" @@ -6489,6 +6489,18 @@ dependencies = [ "libc", ] +[[package]] +name = "nix" +version = "0.29.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "71e2746dc3a24dd78b3cfcb7be93368c6de9963d30f43a6a73998a9cf4b17b46" +dependencies = [ + "bitflags 2.6.0", + "cfg-if", + "cfg_aliases", + "libc", +] + [[package]] name = "node-launchpad" version = "0.4.5" diff --git a/ant-bootstrap/Cargo.toml b/ant-bootstrap/Cargo.toml index cfe61bd7f5..1e292cd64d 100644 --- a/ant-bootstrap/Cargo.toml +++ b/ant-bootstrap/Cargo.toml @@ -15,6 +15,7 @@ local = [] [dependencies] ant-logging = { path = "../ant-logging", version = "0.2.40" } ant-protocol = { version = "0.17.15", path = "../ant-protocol" } +atomic-write-file = "0.2.2" chrono = { version = "0.4", features = ["serde"] } clap = { version = "4.2.1", features = ["derive", "env"] } dirs-next = "~2.0.0" @@ -25,20 +26,16 @@ reqwest = { version = "0.12.2", default-features = false, features = [ ] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" -tempfile = "3.8.1" thiserror = "1.0" tokio = { version = "1.0", features = ["time"] } tracing = "0.1" url = "2.4.0" -# fs2 fails to compile on wasm32 target -[target.'cfg(not(target_arch = "wasm32"))'.dependencies] -fs2 = "0.4.3" - [dev-dependencies] wiremock = "0.5" tokio = { version = "1.0", features = ["full", "test-util"] } tracing-subscriber = { version = "0.3", features = ["env-filter"] } +tempfile = "3.8.1" [target.'cfg(target_arch = "wasm32")'.dependencies] wasmtimer = "0.2.0" \ No newline at end of file diff --git a/ant-bootstrap/src/cache_store.rs b/ant-bootstrap/src/cache_store.rs index facd71490a..b9137d079d 100644 --- a/ant-bootstrap/src/cache_store.rs +++ b/ant-bootstrap/src/cache_store.rs @@ -10,18 +10,16 @@ use crate::{ craft_valid_multiaddr, initial_peers::PeersArgs, multiaddr_get_peer_id, BootstrapAddr, BootstrapAddresses, BootstrapCacheConfig, Error, Result, }; -#[cfg(not(target_arch = "wasm32"))] -use fs2::FileExt; -use libp2p::multiaddr::Protocol; -use libp2p::{Multiaddr, PeerId}; +use atomic_write_file::AtomicWriteFile; +use libp2p::{multiaddr::Protocol, Multiaddr, PeerId}; use serde::{Deserialize, Serialize}; -use std::collections::hash_map::Entry; -use std::collections::HashMap; -use std::fs::{self, File, OpenOptions}; -use std::io::Read; -use std::path::PathBuf; -use std::time::{Duration, SystemTime}; -use tempfile::NamedTempFile; +use std::{ + collections::{hash_map::Entry, HashMap}, + fs::{self, OpenOptions}, + io::{Read, Write}, + path::PathBuf, + time::{Duration, SystemTime}, +}; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct CacheData { @@ -213,11 +211,6 @@ impl BootstrapCacheStore { .open(&cfg.cache_file_path) .inspect_err(|err| warn!("Failed to open cache file: {err}",))?; - // Acquire shared lock for reading - Self::acquire_shared_lock(&file).inspect_err(|err| { - warn!("Failed to acquire shared lock: {err}"); - })?; - // Read the file contents let mut contents = String::new(); file.read_to_string(&mut contents).inspect_err(|err| { @@ -384,87 +377,29 @@ impl BootstrapCacheStore { }) } - /// Acquire a shared lock on the cache file. - #[cfg(target_arch = "wasm32")] - fn acquire_shared_lock(_file: &File) -> Result<()> { - Ok(()) - } - - /// Acquire a shared lock on the cache file. - /// This is a no-op on WASM. - #[cfg(not(target_arch = "wasm32"))] - fn acquire_shared_lock(file: &File) -> Result<()> { - let file = file.try_clone()?; - file.try_lock_shared()?; - - Ok(()) - } - - /// Acquire an exclusive lock on the cache file. - /// This is a no-op on WASM. - #[cfg(target_arch = "wasm32")] - async fn acquire_exclusive_lock(_file: &File) -> Result<()> { - Ok(()) - } - - /// Acquire an exclusive lock on the cache file. - #[cfg(not(target_arch = "wasm32"))] - async fn acquire_exclusive_lock(file: &File) -> Result<()> { - let mut backoff = Duration::from_millis(10); - let max_attempts = 5; - let mut attempts = 0; - - loop { - match file.try_lock_exclusive() { - Ok(_) => return Ok(()), - Err(_) if attempts >= max_attempts => { - return Err(Error::LockError); - } - Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => { - attempts += 1; - #[cfg(not(target_arch = "wasm32"))] - tokio::time::sleep(backoff).await; - #[cfg(target_arch = "wasm32")] - wasmtimer::tokio::sleep(backoff).await; - backoff *= 2; - } - Err(_) => return Err(Error::LockError), - } - } - } - async fn atomic_write(&self) -> Result<()> { - info!("Writing cache to disk: {:?}", self.cache_path); + debug!("Writing cache to disk: {:?}", self.cache_path); // Create parent directory if it doesn't exist if let Some(parent) = self.cache_path.parent() { fs::create_dir_all(parent)?; } - // Create a temporary file in the same directory as the cache file - let temp_dir = std::env::temp_dir(); - let temp_file = NamedTempFile::new_in(&temp_dir)?; - - // Write data to temporary file - serde_json::to_writer_pretty(&temp_file, &self.data)?; - - // Open the target file with proper permissions - let file = OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .open(&self.cache_path)?; + let mut file = AtomicWriteFile::options() + .open(&self.cache_path) + .inspect_err(|err| { + error!("Failed to open cache file using AtomicWriteFile: {err}"); + })?; - // Acquire exclusive lock - Self::acquire_exclusive_lock(&file).await?; - - // Perform atomic rename - temp_file.persist(&self.cache_path).inspect_err(|err| { - error!("Failed to persist file with err: {err:?}"); + let data = serde_json::to_string_pretty(&self.data).inspect_err(|err| { + error!("Failed to serialize cache data: {err}"); + })?; + writeln!(file, "{data}")?; + file.commit().inspect_err(|err| { + error!("Failed to commit atomic write: {err}"); })?; info!("Cache written to disk: {:?}", self.cache_path); - // Lock will be automatically released when file is dropped Ok(()) } } diff --git a/ant-bootstrap/src/error.rs b/ant-bootstrap/src/error.rs index a8cb8e1cc8..77002702e5 100644 --- a/ant-bootstrap/src/error.rs +++ b/ant-bootstrap/src/error.rs @@ -26,8 +26,6 @@ pub enum Error { Json(#[from] serde_json::Error), #[error("HTTP error: {0}")] Http(#[from] reqwest::Error), - #[error("Persist error: {0}")] - Persist(#[from] tempfile::PersistError), #[error("Lock error")] LockError, }