From d1eeb66de3dcc6d9bf106582a2d10b20aa71f056 Mon Sep 17 00:00:00 2001 From: Christopher Berner Date: Sat, 28 Sep 2024 15:00:20 -0700 Subject: [PATCH] WIP: use std file_lock --- .github/workflows/ci.yml | 11 +- Cargo.toml | 2 +- justfile | 2 - rust-toolchain | 2 +- src/lib.rs | 24 +--- src/tree_store/page_store/file_backend/mod.rs | 13 +- .../page_store/file_backend/optimized.rs | 121 ++++++++++++++++++ .../page_store/file_backend/unix.rs | 95 -------------- .../page_store/file_backend/windows.rs | 101 --------------- 9 files changed, 131 insertions(+), 240 deletions(-) create mode 100644 src/tree_store/page_store/file_backend/optimized.rs delete mode 100644 src/tree_store/page_store/file_backend/unix.rs delete mode 100644 src/tree_store/page_store/file_backend/windows.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5980a7cd..ea1c707b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,9 +14,6 @@ jobs: runs-on: ${{ matrix.os }} - env: - RUSTFLAGS: --deny warnings - steps: - uses: actions/checkout@v4 - name: Cache @@ -43,7 +40,7 @@ jobs: - name: Install Rust if: steps.rust-cache.outputs.cache-hit != 'true' run: | - rustup default 1.81 + rustup default nightly-2024-11-15 rustup component add rustfmt rustup component add clippy @@ -80,12 +77,6 @@ jobs: - name: Run tests run: just build test - - name: Clippy - run: cargo clippy --all --all-targets -- -Dwarnings - - - name: Format - run: cargo fmt --all -- --check - - name: Run CPython wrapper tests if: runner.os != 'Windows' run: | diff --git a/Cargo.toml b/Cargo.toml index ec374407..69668339 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ pyo3-build-config = { version = "0.22.0", optional = true } log = { version = "0.4.17", optional = true } pyo3 = { version = "0.22.0", features=["extension-module", "abi3-py37"], optional = true } -[target.'cfg(unix)'.dependencies] +[target.'cfg(target_os = "macos")'.dependencies] libc = "0.2.104" # Common test/bench dependencies diff --git a/justfile b/justfile index b40c7661..3d93d8fe 100644 --- a/justfile +++ b/justfile @@ -4,8 +4,6 @@ build: pre pre: cargo deny --all-features check licenses - cargo fmt --all -- --check - cargo clippy --all --all-targets release: pre cargo build --release diff --git a/rust-toolchain b/rust-toolchain index ea3769f2..24d71449 100644 --- a/rust-toolchain +++ b/rust-toolchain @@ -1 +1 @@ -1.81 +nightly-2024-11-15 diff --git a/src/lib.rs b/src/lib.rs index 6b52a59a..49307bc4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,27 +1,9 @@ -#![deny(clippy::all, clippy::pedantic, clippy::disallowed_methods)] +#![deny(clippy::disallowed_methods)] // TODO: revisit this list and see if we can enable some -#![allow( - let_underscore_drop, - clippy::default_trait_access, - clippy::if_not_else, - clippy::inline_always, - clippy::iter_not_returning_iterator, - clippy::manual_let_else, - clippy::missing_errors_doc, - clippy::missing_panics_doc, - clippy::module_name_repetitions, - clippy::must_use_candidate, - clippy::needless_pass_by_value, - clippy::option_option, - clippy::redundant_closure_for_method_calls, - clippy::similar_names, - clippy::too_many_lines, - clippy::unnecessary_wraps, - clippy::unreadable_literal, - clippy::wildcard_imports -)] +#![allow(clippy::all)] // TODO remove this once wasi no longer requires nightly #![cfg_attr(target_os = "wasi", feature(wasi_ext))] +#![feature(file_lock)] //! # redb //! diff --git a/src/tree_store/page_store/file_backend/mod.rs b/src/tree_store/page_store/file_backend/mod.rs index 91a10029..dd67d15b 100644 --- a/src/tree_store/page_store/file_backend/mod.rs +++ b/src/tree_store/page_store/file_backend/mod.rs @@ -1,12 +1,7 @@ -#[cfg(any(unix, target_os = "wasi"))] -mod unix; -#[cfg(any(unix, target_os = "wasi"))] -pub use unix::FileBackend; - -#[cfg(windows)] -mod windows; -#[cfg(windows)] -pub use windows::FileBackend; +#[cfg(any(windows, unix, target_os = "wasi"))] +mod optimized; +#[cfg(any(windows, unix, target_os = "wasi"))] +pub use optimized::FileBackend; #[cfg(not(any(windows, unix, target_os = "wasi")))] mod fallback; diff --git a/src/tree_store/page_store/file_backend/optimized.rs b/src/tree_store/page_store/file_backend/optimized.rs new file mode 100644 index 00000000..a89a15e2 --- /dev/null +++ b/src/tree_store/page_store/file_backend/optimized.rs @@ -0,0 +1,121 @@ +use crate::{DatabaseError, Result, StorageBackend}; +use std::fs::File; +use std::io; + +#[cfg(feature = "logging")] +use log::warn; + +#[cfg(unix)] +use std::os::unix::fs::FileExt; + +#[cfg(windows)] +use std::os::windows::fs::FileExt; + +#[cfg(target_os = "wasi")] +use std::os::wasi::fs::FileExt; + +#[cfg(target_os = "macos")] +use std::os::unix::io::AsRawFd; + +/// Stores a database as a file on-disk. +#[derive(Debug)] +pub struct FileBackend { + lock_supported: bool, + file: File, +} + +impl FileBackend { + /// Creates a new backend which stores data to the given file. + pub fn new(file: File) -> Result { + match file.try_lock() { + Ok(locked) => { + if locked { + Ok(Self { file, lock_supported: true}) + } else { + Err(DatabaseError::DatabaseAlreadyOpen) + } + }, + Err(err) if err.kind() == io::ErrorKind::Unsupported => { + #[cfg(feature = "logging")] + warn!("File locks not supported on this platform. You must ensure that only a single process opens the database file, at a time"); + + Ok(Self { file, lock_supported: false }) + } + Err(err) => { + Err(err.into()) + } + } + } +} + +impl StorageBackend for FileBackend { + fn len(&self) -> Result { + Ok(self.file.metadata()?.len()) + } + + #[cfg(any(unix, target_os = "wasi"))] + fn read(&self, offset: u64, len: usize) -> Result, io::Error> { + let mut buffer = vec![0; len]; + self.file.read_exact_at(&mut buffer, offset)?; + Ok(buffer) + } + + #[cfg(windows)] + fn read(&self, mut offset: u64, len: usize) -> std::result::Result, io::Error> { + let mut buffer = vec![0; len]; + let mut data_offset = 0; + while data_offset < buffer.len() { + let read = self.file.seek_read(&mut buffer[data_offset..], offset)?; + offset += read as u64; + data_offset += read; + } + Ok(buffer) + } + + fn set_len(&self, len: u64) -> Result<(), io::Error> { + self.file.set_len(len) + } + + #[cfg(not(target_os = "macos"))] + fn sync_data(&self, _: bool) -> Result<(), io::Error> { + self.file.sync_data() + } + + #[cfg(target_os = "macos")] + fn sync_data(&self, eventual: bool) -> Result<(), io::Error> { + if eventual { + let code = unsafe { libc::fcntl(self.file.as_raw_fd(), libc::F_BARRIERFSYNC) }; + if code == -1 { + Err(io::Error::last_os_error()) + } else { + Ok(()) + } + } else { + self.file.sync_data() + } + } + + #[cfg(any(unix, target_os = "wasi"))] + fn write(&self, offset: u64, data: &[u8]) -> Result<(), io::Error> { + self.file.write_all_at(data, offset) + } + + #[cfg(windows)] + fn write(&self, mut offset: u64, data: &[u8]) -> std::result::Result<(), io::Error> { + let mut data_offset = 0; + while data_offset < data.len() { + let written = self.file.seek_write(&data[data_offset..], offset)?; + offset += written as u64; + data_offset += written; + } + Ok(()) + } +} + +impl Drop for FileBackend { + fn drop(&mut self) { + if self.lock_supported { + let _ = self.file.unlock(); + } + } +} diff --git a/src/tree_store/page_store/file_backend/unix.rs b/src/tree_store/page_store/file_backend/unix.rs deleted file mode 100644 index db24c48b..00000000 --- a/src/tree_store/page_store/file_backend/unix.rs +++ /dev/null @@ -1,95 +0,0 @@ -// TODO once Rust's libc has flock implemented for WASI, this file needs to be revisited. -// What needs to be changed is commented below. -// See also: https://github.com/WebAssembly/wasi-filesystem/issues/2 - -// Remove this line once wasi-libc has flock -#![cfg_attr(target_os = "wasi", allow(unused_imports))] - -use crate::{DatabaseError, Result, StorageBackend}; -use std::fs::File; -use std::io; - -#[cfg(unix)] -use std::os::unix::{fs::FileExt, io::AsRawFd}; - -#[cfg(target_os = "wasi")] -use std::os::wasi::{fs::FileExt, io::AsRawFd}; - -/// Stores a database as a file on-disk. -#[derive(Debug)] -pub struct FileBackend { - file: File, -} - -impl FileBackend { - /// Creates a new backend which stores data to the given file. - // This is a no-op until we get flock in wasi-libc. - // Delete this function when we get flock. - #[cfg(target_os = "wasi")] - pub fn new(file: File) -> Result { - Ok(Self { file }) - } - - /// Creates a new backend which stores data to the given file. - #[cfg(unix)] // remove this line when wasi-libc gets flock - pub fn new(file: File) -> Result { - let fd = file.as_raw_fd(); - let result = unsafe { libc::flock(fd, libc::LOCK_EX | libc::LOCK_NB) }; - if result != 0 { - let err = io::Error::last_os_error(); - if err.kind() == io::ErrorKind::WouldBlock { - Err(DatabaseError::DatabaseAlreadyOpen) - } else { - Err(err.into()) - } - } else { - Ok(Self { file }) - } - } -} - -impl StorageBackend for FileBackend { - fn len(&self) -> Result { - Ok(self.file.metadata()?.len()) - } - - fn read(&self, offset: u64, len: usize) -> Result, io::Error> { - let mut buffer = vec![0; len]; - self.file.read_exact_at(&mut buffer, offset)?; - Ok(buffer) - } - - fn set_len(&self, len: u64) -> Result<(), io::Error> { - self.file.set_len(len) - } - - #[cfg(not(target_os = "macos"))] - fn sync_data(&self, _: bool) -> Result<(), io::Error> { - self.file.sync_data() - } - - #[cfg(target_os = "macos")] - fn sync_data(&self, eventual: bool) -> Result<(), io::Error> { - if eventual { - let code = unsafe { libc::fcntl(self.file.as_raw_fd(), libc::F_BARRIERFSYNC) }; - if code == -1 { - Err(io::Error::last_os_error()) - } else { - Ok(()) - } - } else { - self.file.sync_data() - } - } - - fn write(&self, offset: u64, data: &[u8]) -> Result<(), io::Error> { - self.file.write_all_at(data, offset) - } -} - -#[cfg(unix)] // remove this line when wasi-libc gets flock -impl Drop for FileBackend { - fn drop(&mut self) { - unsafe { libc::flock(self.file.as_raw_fd(), libc::LOCK_UN) }; - } -} diff --git a/src/tree_store/page_store/file_backend/windows.rs b/src/tree_store/page_store/file_backend/windows.rs deleted file mode 100644 index 0a2c263f..00000000 --- a/src/tree_store/page_store/file_backend/windows.rs +++ /dev/null @@ -1,101 +0,0 @@ -#![allow(clippy::upper_case_acronyms)] - -use crate::{DatabaseError, Result, StorageBackend}; -use std::fs::File; -use std::io; -use std::os::windows::fs::FileExt; -use std::os::windows::io::AsRawHandle; -use std::os::windows::io::RawHandle; - -const ERROR_LOCK_VIOLATION: i32 = 0x21; -const ERROR_IO_PENDING: i32 = 997; - -extern "system" { - /// - fn LockFile( - file: RawHandle, - offset_low: u32, - offset_high: u32, - length_low: u32, - length_high: u32, - ) -> i32; - - /// - fn UnlockFile( - file: RawHandle, - offset_low: u32, - offset_high: u32, - length_low: u32, - length_high: u32, - ) -> i32; -} - -/// Stores a database as a file on-disk. -#[derive(Debug)] -pub struct FileBackend { - file: File, -} - -impl FileBackend { - /// Creates a new backend which stores data to the given file. - pub fn new(file: File) -> Result { - let handle = file.as_raw_handle(); - unsafe { - let result = LockFile(handle, 0, 0, u32::MAX, u32::MAX); - - if result == 0 { - let err = io::Error::last_os_error(); - return if err.raw_os_error() == Some(ERROR_IO_PENDING) - || err.raw_os_error() == Some(ERROR_LOCK_VIOLATION) - { - Err(DatabaseError::DatabaseAlreadyOpen) - } else { - Err(err.into()) - }; - } - }; - - Ok(Self { file }) - } -} - -impl StorageBackend for FileBackend { - fn len(&self) -> Result { - Ok(self.file.metadata()?.len()) - } - - fn read(&self, mut offset: u64, len: usize) -> Result, io::Error> { - let mut buffer = vec![0; len]; - let mut data_offset = 0; - while data_offset < buffer.len() { - let read = self.file.seek_read(&mut buffer[data_offset..], offset)?; - offset += read as u64; - data_offset += read; - } - Ok(buffer) - } - - fn set_len(&self, len: u64) -> Result<(), io::Error> { - self.file.set_len(len) - } - - fn sync_data(&self, _: bool) -> Result<(), io::Error> { - self.file.sync_data() - } - - fn write(&self, mut offset: u64, data: &[u8]) -> Result<(), io::Error> { - let mut data_offset = 0; - while data_offset < data.len() { - let written = self.file.seek_write(&data[data_offset..], offset)?; - offset += written as u64; - data_offset += written; - } - Ok(()) - } -} - -impl Drop for FileBackend { - fn drop(&mut self) { - unsafe { UnlockFile(self.file.as_raw_handle(), 0, 0, u32::MAX, u32::MAX) }; - } -}