From 2670938df5f6a3ed155b793e301ea0ab64b8cec1 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 26 Feb 2023 15:55:36 +0100 Subject: [PATCH 1/3] upgrade `tempfile` to `3.4` --- Cargo.lock | 18 ++++-------------- gix-tempfile/Cargo.toml | 2 +- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8c6f729c51c..35d331bfab3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3486,15 +3486,6 @@ version = "0.6.28" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "456c603be3e8d448b072f410900c09faf164fbce2d480456f50eea6e25f9c848" -[[package]] -name = "remove_dir_all" -version = "0.5.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3acd125665422973a33ac9d3dd2df85edad0f4ae9b00dafb1a05e43a9f5ef8e7" -dependencies = [ - "winapi", -] - [[package]] name = "reqwest" version = "0.11.14" @@ -3930,16 +3921,15 @@ dependencies = [ [[package]] name = "tempfile" -version = "3.3.0" +version = "3.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5cdb1ef4eaeeaddc8fbd371e5017057064af0911902ef36b39801f67cc6d79e4" +checksum = "af18f7ae1acd354b992402e9ec5864359d693cd8a79dcbef59f76891701c1e95" dependencies = [ "cfg-if", "fastrand", - "libc", "redox_syscall", - "remove_dir_all", - "winapi", + "rustix", + "windows-sys 0.42.0", ] [[package]] diff --git a/gix-tempfile/Cargo.toml b/gix-tempfile/Cargo.toml index 7b9f6c2e8dd..6dcb36237fb 100644 --- a/gix-tempfile/Cargo.toml +++ b/gix-tempfile/Cargo.toml @@ -18,7 +18,7 @@ dashmap = "5.1.0" once_cell = { version = "1.8.0", default-features = false, features = ["race", "std"] } signal-hook = { version = "0.3.9", default-features = false } signal-hook-registry = "1.4.0" -tempfile = "3.2.0" +tempfile = "3.4.0" [target.'cfg(not(windows))'.dependencies] libc = { version = "0.2.98", default-features = false } From 441f5ac4dd2f0636ec07065f8095e8bae5ce6985 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 26 Feb 2023 16:39:25 +0100 Subject: [PATCH 2/3] feat!: gate all signal handling behind the `signals` feature toggle. (#339) This change also consolidates all signal handling into its own module called `signal` to provide reusable handlers and as well as well as signal initialization. Note that the functions to cleanup tempfiles don't interact with the signal registry, hence they still can be called without the `signals` feature enabled. Note that this change sneakily fixes a bug that could have caused a `write_all()` on a tempfile that was removed by a signal to enter an infinite loop. --- Cargo.lock | 1 + Makefile | 4 + gix-tempfile/Cargo.toml | 32 +++++- ...delete-tempfiles-on-sigterm-interactive.rs | 13 ++- .../examples/delete-tempfiles-on-sigterm.rs | 19 ++-- .../examples/try-deadlock-on-cleanup.rs | 48 +++++---- gix-tempfile/src/forksafe.rs | 3 +- gix-tempfile/src/handle.rs | 34 +++--- gix-tempfile/src/handler.rs | 83 --------------- gix-tempfile/src/lib.rs | 81 ++++---------- gix-tempfile/src/registry.rs | 45 ++++++++ gix-tempfile/src/signal.rs | 100 ++++++++++++++++++ gix-tempfile/tests/tempfile/mod.rs | 8 +- gix-tempfile/tests/tempfile/registry.rs | 30 ++++++ gix/src/interrupt.rs | 2 +- 15 files changed, 305 insertions(+), 198 deletions(-) delete mode 100644 gix-tempfile/src/handler.rs create mode 100644 gix-tempfile/src/registry.rs create mode 100644 gix-tempfile/src/signal.rs create mode 100644 gix-tempfile/tests/tempfile/registry.rs diff --git a/Cargo.lock b/Cargo.lock index 35d331bfab3..c290bacfd85 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2164,6 +2164,7 @@ name = "gix-tempfile" version = "3.0.2" dependencies = [ "dashmap", + "document-features", "libc", "once_cell", "signal-hook", diff --git a/Makefile b/Makefile index 64abd08db15..6c8123d7783 100644 --- a/Makefile +++ b/Makefile @@ -72,6 +72,8 @@ check: ## Build all code in suitable configurations cd gitoxide-core && if cargo check --all-features 2>/dev/null; then false; else true; fi cd gix-hash && cargo check --all-features \ && cargo check + cd gix-tempfile && cargo check --features signals \ + && cargo check cd gix-object && cargo check --all-features \ && cargo check --features verbose-object-parsing-errors cd gix-index && cargo check --features serde1 @@ -139,6 +141,8 @@ check: ## Build all code in suitable configurations unit-tests: ## run all unit tests cargo test --all + cd gix-tempfile && cargo test --features signals \ + && cargo test cd gix-features && cargo test && cargo test --all-features cd gix-ref/tests && cargo test --all-features cd gix-odb && cargo test && cargo test --all-features diff --git a/gix-tempfile/Cargo.toml b/gix-tempfile/Cargo.toml index 6dcb36237fb..399177c5a03 100644 --- a/gix-tempfile/Cargo.toml +++ b/gix-tempfile/Cargo.toml @@ -9,6 +9,21 @@ edition = "2021" include = ["src/**/*", "LICENSE-*", "README.md", "CHANGELOG.md"] rust-version = "1.64" +[[example]] +name = "delete-tempfiles-on-sigterm" +path = "examples/delete-tempfiles-on-sigterm.rs" +required-features = ["signals"] + +[[example]] +name = "delete-tempfiles-on-sigterm-interactive" +path = "examples/delete-tempfiles-on-sigterm-interactive.rs" +required-features = ["signals"] + +[[example]] +name = "try-deadlock-on-cleanup" +path = "examples/try-deadlock-on-cleanup.rs" +required-features = ["signals"] + [lib] doctest = false test = true @@ -16,9 +31,22 @@ test = true [dependencies] dashmap = "5.1.0" once_cell = { version = "1.8.0", default-features = false, features = ["race", "std"] } -signal-hook = { version = "0.3.9", default-features = false } -signal-hook-registry = "1.4.0" tempfile = "3.4.0" +signal-hook = { version = "0.3.9", default-features = false, optional = true } +signal-hook-registry = { version = "1.4.0", optional = true } + +document-features = { version = "0.2.0", optional = true } + +[features] +default = [] +signals = ["dep:signal-hook", "dep:signal-hook-registry"] + [target.'cfg(not(windows))'.dependencies] libc = { version = "0.2.98", default-features = false } + +[package.metadata.docs.rs] +all-features = true +features = ["document-features"] +rustdoc-args = ["--cfg", "docsrs"] + diff --git a/gix-tempfile/examples/delete-tempfiles-on-sigterm-interactive.rs b/gix-tempfile/examples/delete-tempfiles-on-sigterm-interactive.rs index 8eb93fd6031..8843b66c358 100644 --- a/gix-tempfile/examples/delete-tempfiles-on-sigterm-interactive.rs +++ b/gix-tempfile/examples/delete-tempfiles-on-sigterm-interactive.rs @@ -1,9 +1,14 @@ -use std::path::PathBuf; - -use gix_tempfile::{AutoRemove, ContainingDirectory}; +#[cfg(not(feature = "signals"))] +fn main() { + panic!("The `signals` feature needs to be set to compile this example"); +} +#[cfg(feature = "signals")] fn main() -> std::io::Result<()> { - gix_tempfile::setup(Default::default()); + use gix_tempfile::{AutoRemove, ContainingDirectory}; + use std::path::PathBuf; + + gix_tempfile::signal::setup(Default::default()); let filepath = PathBuf::new().join("writable-tempfile.ext"); let markerpath = PathBuf::new().join("marker.ext"); let _tempfile = gix_tempfile::writable_at(&filepath, ContainingDirectory::Exists, AutoRemove::Tempfile)?; diff --git a/gix-tempfile/examples/delete-tempfiles-on-sigterm.rs b/gix-tempfile/examples/delete-tempfiles-on-sigterm.rs index 72919a28882..4906fe72560 100644 --- a/gix-tempfile/examples/delete-tempfiles-on-sigterm.rs +++ b/gix-tempfile/examples/delete-tempfiles-on-sigterm.rs @@ -1,12 +1,17 @@ -use std::{ - io::{stdout, Write}, - path::PathBuf, -}; - -use gix_tempfile::{AutoRemove, ContainingDirectory}; +#[cfg(not(feature = "signals"))] +fn main() { + panic!("The `signals` feature needs to be set to compile this example"); +} +#[cfg(feature = "signals")] fn main() -> std::io::Result<()> { - gix_tempfile::setup(Default::default()); + use gix_tempfile::{AutoRemove, ContainingDirectory}; + use std::{ + io::{stdout, Write}, + path::PathBuf, + }; + + gix_tempfile::signal::setup(Default::default()); let filepath = PathBuf::new().join("tempfile.ext"); let _tempfile = gix_tempfile::mark_at(&filepath, ContainingDirectory::Exists, AutoRemove::Tempfile)?; assert!(filepath.is_file(), "a tempfile was created"); diff --git a/gix-tempfile/examples/try-deadlock-on-cleanup.rs b/gix-tempfile/examples/try-deadlock-on-cleanup.rs index e0625b2649c..81d99258154 100644 --- a/gix-tempfile/examples/try-deadlock-on-cleanup.rs +++ b/gix-tempfile/examples/try-deadlock-on-cleanup.rs @@ -1,15 +1,21 @@ -use std::{ - path::Path, - sync::{ - atomic::{AtomicUsize, Ordering}, - Arc, - }, - time::Duration, -}; - -use gix_tempfile::{handle::Writable, AutoRemove, ContainingDirectory, Handle}; +#[cfg(not(feature = "signals"))] +fn main() { + panic!("The `signals` feature needs to be set to compile this example"); +} +#[cfg(feature = "signals")] fn main() -> Result<(), Box> { + use std::{ + path::Path, + sync::{ + atomic::{AtomicUsize, Ordering}, + Arc, + }, + time::Duration, + }; + + use gix_tempfile::{handle::Writable, AutoRemove, ContainingDirectory, Handle}; + let secs_to_run: usize = std::env::args() .nth(1) .ok_or("the first argument is the amount of seconds to run")? @@ -19,7 +25,7 @@ fn main() -> Result<(), Box> { let tempfiles_created = Arc::new(AtomicUsize::default()); let tempfiles_registry_locked = Arc::new(AtomicUsize::default()); let signal_raised = Arc::new(AtomicUsize::default()); - gix_tempfile::setup(gix_tempfile::SignalHandlerMode::DeleteTempfilesOnTermination); + gix_tempfile::signal::setup(gix_tempfile::signal::handler::Mode::DeleteTempfilesOnTermination); for tid in 0..suspected_dashmap_block_size { std::thread::spawn({ @@ -83,15 +89,15 @@ fn main() -> Result<(), Box> { signal_hook::low_level::raise(signal_hook::consts::SIGINT)?; signal_raised.fetch_add(1, Ordering::SeqCst); } -} -fn tempfile_for_thread_or_panic(tid: i32, tmp: &Path, count: &AtomicUsize) -> Handle { - let res = gix_tempfile::writable_at( - tmp.join(format!("thread-{tid}")), - ContainingDirectory::Exists, - AutoRemove::Tempfile, - ) - .unwrap(); - count.fetch_add(1, Ordering::SeqCst); - res + fn tempfile_for_thread_or_panic(tid: i32, tmp: &Path, count: &AtomicUsize) -> Handle { + let res = gix_tempfile::writable_at( + tmp.join(format!("thread-{tid}")), + ContainingDirectory::Exists, + AutoRemove::Tempfile, + ) + .unwrap(); + count.fetch_add(1, Ordering::SeqCst); + res + } } diff --git a/gix-tempfile/src/forksafe.rs b/gix-tempfile/src/forksafe.rs index 804db1ed634..1da7b2b2c5a 100644 --- a/gix-tempfile/src/forksafe.rs +++ b/gix-tempfile/src/forksafe.rs @@ -1,4 +1,4 @@ -use std::{io::Write, path::Path}; +use std::path::Path; use tempfile::{NamedTempFile, TempPath}; @@ -88,6 +88,7 @@ impl ForksafeTempfile { } pub fn drop_without_deallocation(self) { + use std::io::Write; let temppath = match self.inner { TempfileOrTemppath::Tempfile(file) => { let (mut file, temppath) = file.into_parts(); diff --git a/gix-tempfile/src/handle.rs b/gix-tempfile/src/handle.rs index df857107439..f20e237ee1a 100644 --- a/gix-tempfile/src/handle.rs +++ b/gix-tempfile/src/handle.rs @@ -3,7 +3,7 @@ use std::{io, path::Path}; use tempfile::{NamedTempFile, TempPath}; -use crate::{AutoRemove, ContainingDirectory, ForksafeTempfile, Handle, NEXT_MAP_INDEX, REGISTER}; +use crate::{AutoRemove, ContainingDirectory, ForksafeTempfile, Handle, NEXT_MAP_INDEX, REGISTRY}; /// Marker to signal the Registration is an open file able to be written to. #[derive(Debug)] @@ -45,7 +45,7 @@ impl Handle<()> { ForksafeTempfile::new(builder.rand_bytes(0).tempfile_in(parent_dir)?, cleanup, mode) }; let id = NEXT_MAP_INDEX.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - expect_none(REGISTER.insert(id, Some(tempfile))); + expect_none(REGISTRY.insert(id, Some(tempfile))); Ok(id) } @@ -57,7 +57,7 @@ impl Handle<()> { ) -> io::Result { let containing_directory = directory.resolve(containing_directory.as_ref())?; let id = NEXT_MAP_INDEX.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - expect_none(REGISTER.insert( + expect_none(REGISTRY.insert( id, Some(ForksafeTempfile::new( NamedTempFile::new_in(containing_directory)?, @@ -86,7 +86,7 @@ impl Handle { /// /// It's a theoretical possibility that the file isn't present anymore if signals interfere, hence the `Option` pub fn take(self) -> Option { - let res = REGISTER.remove(&self.id); + let res = REGISTRY.remove(&self.id); std::mem::forget(self); res.and_then(|(_k, v)| v.map(|v| v.into_temppath())) } @@ -123,7 +123,7 @@ impl Handle { /// /// It's a theoretical possibility that the file isn't present anymore if signals interfere, hence the `Option` pub fn take(self) -> Option { - let res = REGISTER.remove(&self.id); + let res = REGISTRY.remove(&self.id); std::mem::forget(self); res.and_then(|(_k, v)| v.map(|v| v.into_tempfile().expect("correct runtime typing"))) } @@ -134,17 +134,17 @@ impl Handle { /// it right after to perform more updates of this kind in other tempfiles. When all succeed, they can be renamed one after /// another. pub fn close(self) -> std::io::Result> { - match REGISTER.remove(&self.id) { + match REGISTRY.remove(&self.id) { Some((id, Some(t))) => { std::mem::forget(self); - expect_none(REGISTER.insert(id, Some(t.close()))); + expect_none(REGISTRY.insert(id, Some(t.close()))); Ok(Handle:: { id, _marker: Default::default(), }) } None | Some((_, None)) => Err(std::io::Error::new( - std::io::ErrorKind::Interrupted, + std::io::ErrorKind::NotFound, format!("The tempfile with id {} wasn't available anymore", self.id), )), } @@ -162,14 +162,14 @@ impl Handle { /// The caller must assure that the signal handler for cleanup will be followed by an abort call so that /// this code won't run again on a removed instance. An error will occur otherwise. pub fn with_mut(&mut self, once: impl FnOnce(&mut NamedTempFile) -> T) -> std::io::Result { - match REGISTER.remove(&self.id) { + match REGISTRY.remove(&self.id) { Some((id, Some(mut t))) => { let res = once(t.as_mut_tempfile().expect("correct runtime typing")); - expect_none(REGISTER.insert(id, Some(t))); + expect_none(REGISTRY.insert(id, Some(t))); Ok(res) } None | Some((_, None)) => Err(std::io::Error::new( - std::io::ErrorKind::Interrupted, + std::io::ErrorKind::NotFound, format!("The tempfile with id {} wasn't available anymore", self.id), )), } @@ -210,7 +210,7 @@ pub mod persist { use crate::{ handle::{expect_none, Closed, Writable}, - Handle, REGISTER, + Handle, REGISTRY, }; mod error { @@ -247,7 +247,7 @@ pub mod persist { /// Note that it might not exist anymore if an interrupt handler managed to steal it and allowed the program to return to /// its normal flow. pub fn persist(self, path: impl AsRef) -> Result, Error> { - let res = REGISTER.remove(&self.id); + let res = REGISTRY.remove(&self.id); match res.and_then(|(_k, v)| v.map(|v| v.persist(path))) { Some(Ok(Some(file))) => { @@ -259,7 +259,7 @@ pub mod persist { Ok(None) } Some(Err((err, tempfile))) => { - expect_none(REGISTER.insert(self.id, Some(tempfile))); + expect_none(REGISTRY.insert(self.id, Some(tempfile))); Err(Error:: { error: err, handle: self, @@ -274,7 +274,7 @@ pub mod persist { /// Persist this tempfile to replace the file at the given `path` if necessary, in a way that recovers the original instance /// on error. pub fn persist(self, path: impl AsRef) -> Result<(), Error> { - let res = REGISTER.remove(&self.id); + let res = REGISTRY.remove(&self.id); match res.and_then(|(_k, v)| v.map(|v| v.persist(path))) { None | Some(Ok(None)) => { @@ -282,7 +282,7 @@ pub mod persist { Ok(()) } Some(Err((err, tempfile))) => { - expect_none(REGISTER.insert(self.id, Some(tempfile))); + expect_none(REGISTRY.insert(self.id, Some(tempfile))); Err(Error:: { error: err, handle: self, @@ -312,7 +312,7 @@ fn expect_none(v: Option) { impl Drop for Handle { fn drop(&mut self) { - if let Some((_id, Some(tempfile))) = REGISTER.remove(&self.id) { + if let Some((_id, Some(tempfile))) = REGISTRY.remove(&self.id) { tempfile.drop_impl(); } } diff --git a/gix-tempfile/src/handler.rs b/gix-tempfile/src/handler.rs deleted file mode 100644 index 7b17b144ad2..00000000000 --- a/gix-tempfile/src/handler.rs +++ /dev/null @@ -1,83 +0,0 @@ -//! -use std::sync::atomic::Ordering; - -use crate::{SignalHandlerMode, NEXT_MAP_INDEX, REGISTER, SIGNAL_HANDLER_MODE}; - -/// Remove all tempfiles still registered on our global registry. -/// -/// # Safety -/// Note that Mutexes of any kind are not allowed, and so aren't allocation or deallocation of memory. -/// We are using lock-free datastructures and sprinkle in `std::mem::forget` to avoid deallocating. -/// Most importantly, we use `try_lock()` which uses an atomic int only without waiting, making our register safe to use, -/// at the expense of possibly missing a lock file if another thread wants to obtain it or put it back -/// (i.e. mutates the register shard). -pub fn cleanup_tempfiles() { - let current_pid = std::process::id(); - let one_past_last_index = NEXT_MAP_INDEX.load(Ordering::SeqCst); - for idx in 0..one_past_last_index { - if let Some(entry) = REGISTER.try_entry(idx) { - entry.and_modify(|tempfile| { - if tempfile - .as_ref() - .map_or(false, |tf| tf.owning_process_id == current_pid) - { - if let Some(tempfile) = tempfile.take() { - tempfile.drop_without_deallocation(); - } - } - }); - } - } -} - -/// On linux we can handle the actual signal as we know it. -#[cfg(not(windows))] -pub(crate) fn cleanup_tempfiles_nix(sig: &libc::siginfo_t) { - cleanup_tempfiles(); - let restore_original_behaviour = SignalHandlerMode::DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour as usize; - if SIGNAL_HANDLER_MODE.load(std::sync::atomic::Ordering::SeqCst) == restore_original_behaviour { - signal_hook::low_level::emulate_default_handler(sig.si_signo).ok(); - } -} - -/// On windows, assume sig-term and emulate sig-term unconditionally. -#[cfg(windows)] -pub(crate) fn cleanup_tempfiles_windows() { - cleanup_tempfiles(); - let restore_original_behaviour = SignalHandlerMode::DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour as usize; - if SIGNAL_HANDLER_MODE.load(std::sync::atomic::Ordering::SeqCst) == restore_original_behaviour { - signal_hook::low_level::emulate_default_handler(signal_hook::consts::SIGTERM).ok(); - } -} - -#[cfg(test)] -mod tests { - use std::path::Path; - - use crate::{AutoRemove, ContainingDirectory}; - - fn filecount_in(path: impl AsRef) -> usize { - std::fs::read_dir(path).expect("valid dir").count() - } - - #[test] - fn various_termination_signals_remove_tempfiles_unconditionally() -> Result<(), Box> { - crate::setup(Default::default()); - let dir = tempfile::tempdir()?; - for sig in signal_hook::consts::TERM_SIGNALS { - let _tempfile = crate::new(dir.path(), ContainingDirectory::Exists, AutoRemove::Tempfile)?; - assert_eq!( - filecount_in(dir.path()), - 1, - "only one tempfile exists no matter the iteration" - ); - signal_hook::low_level::raise(*sig)?; - assert_eq!( - filecount_in(dir.path()), - 0, - "the signal triggers removal but won't terminate the process (anymore)" - ); - } - Ok(()) - } -} diff --git a/gix-tempfile/src/lib.rs b/gix-tempfile/src/lib.rs index 7a5977fe499..47de6d32eb1 100644 --- a/gix-tempfile/src/lib.rs +++ b/gix-tempfile/src/lib.rs @@ -1,7 +1,7 @@ //! git-style registered tempfiles that are removed upon typical termination signals. //! //! To register signal handlers in a typical application that doesn't have its own, call -//! [`gix_tempfile::setup(Default::default())`][setup()] before creating the first tempfile. +//! [`gix_tempfile::signal::setup(Default::default())`][signal::setup()] before creating the first tempfile. //! //! Signal handlers are powered by [`signal-hook`] to get notified when the application is told to shut down //! to assure tempfiles are deleted. The deletion is filtered by process id to allow forks to have their own @@ -9,11 +9,11 @@ //! //! ### Initial Setup //! -//! As no handlers for `TERMination` are installed, it is required to call [`setup()`] before creating the first tempfile. +//! As no handlers for `TERMination` are installed, it is required to call [`signal::setup()`] before creating the first tempfile. //! This also allows to control how `git-tempfiles` integrates with other handlers under application control. //! //! As a general rule of thumb, use `Default::default()` as argument to emulate the default behaviour and -//! abort the process after cleaning temporary files. Read more about options in [SignalHandlerMode]. +//! abort the process after cleaning temporary files. Read more about options in [signal::handler::Mode]. //! //! # Limitations //! @@ -24,6 +24,13 @@ //! but not others. Any other operation dealing with the tempfile suffers from the same issue. //! //! [signal-hook]: https://docs.rs/signal-hook +//! +//! ## Feature Flags +#![cfg_attr( + feature = "document-features", + cfg_attr(doc, doc = ::document_features::document_features!()) +)] +#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] #![deny(missing_docs, rust_2018_idioms, unsafe_code)] use std::{ @@ -39,7 +46,9 @@ use once_cell::sync::Lazy; mod fs; pub use fs::{create_dir, remove_dir}; -pub mod handler; +#[cfg(feature = "signals")] +/// signal setup and reusable handlers. +pub mod signal; mod forksafe; use forksafe::ForksafeTempfile; @@ -47,22 +56,24 @@ use forksafe::ForksafeTempfile; pub mod handle; use crate::handle::{Closed, Writable}; -static SIGNAL_HANDLER_MODE: AtomicUsize = AtomicUsize::new(SignalHandlerMode::None as usize); +/// +pub mod registry; + static NEXT_MAP_INDEX: AtomicUsize = AtomicUsize::new(0); -static REGISTER: Lazy>> = Lazy::new(|| { - let mode = SIGNAL_HANDLER_MODE.load(std::sync::atomic::Ordering::SeqCst); - if mode != SignalHandlerMode::None as usize { +static REGISTRY: Lazy>> = Lazy::new(|| { + #[cfg(feature = "signals")] + if signal::handler::MODE.load(std::sync::atomic::Ordering::SeqCst) != signal::handler::Mode::None as usize { for sig in signal_hook::consts::TERM_SIGNALS { // SAFETY: handlers are considered unsafe because a lot can go wrong. See `cleanup_tempfiles()` for details on safety. #[allow(unsafe_code)] unsafe { #[cfg(not(windows))] { - signal_hook_registry::register_sigaction(*sig, handler::cleanup_tempfiles_nix) + signal_hook_registry::register_sigaction(*sig, signal::handler::cleanup_tempfiles_nix) } #[cfg(windows)] { - signal_hook::low_level::register(*sig, handler::cleanup_tempfiles_windows) + signal_hook::low_level::register(*sig, signal::handler::cleanup_tempfiles_windows) } } .expect("signals can always be installed"); @@ -71,32 +82,6 @@ static REGISTER: Lazy>> = Lazy::new(|| { DashMap::new() }); -/// Define how our signal handlers act -#[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq)] -pub enum SignalHandlerMode { - /// Do not install a signal handler at all, but have somebody else call our handler directly. - None = 0, - /// Delete all remaining registered tempfiles on termination. - DeleteTempfilesOnTermination = 1, - /// Delete all remaining registered tempfiles on termination and emulate the default handler behaviour. - /// - /// This typically leads to the process being aborted. - DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour = 2, -} - -impl Default for SignalHandlerMode { - /// By default we will emulate the default behaviour and abort the process. - /// - /// While testing, we will not abort the process. - fn default() -> Self { - if cfg!(test) { - SignalHandlerMode::DeleteTempfilesOnTermination - } else { - SignalHandlerMode::DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour - } - } -} - /// A type expressing the ways we can deal with directories containing a tempfile. #[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq)] pub enum ContainingDirectory { @@ -179,27 +164,3 @@ pub fn mark_at( ) -> io::Result> { Handle::::at(path, directory, cleanup) } - -/// Initialize signal handlers and other state to keep track of tempfiles, and **must be called before the first tempfile is created**, -/// allowing to set the `mode` in which signal handlers are installed. -/// -/// Only has an effect the first time it is called. -/// -/// Note that it is possible to not call this function and instead call [handler::cleanup_tempfiles()][crate::handler::cleanup_tempfiles()] -/// from a handler under the applications control. -pub fn setup(mode: SignalHandlerMode) { - SIGNAL_HANDLER_MODE.store(mode as usize, std::sync::atomic::Ordering::SeqCst); - Lazy::force(®ISTER); -} - -/// DO NOT USE - use [`setup()`] instead. -/// -/// Indeed this is merely the old name of `setup()`, which is now a required part of configuring gix-tempfile. -#[deprecated( - since = "2.0.0", - note = "call setup(…) instead, this function will be removed in the next major release" -)] -#[doc(hidden)] -pub fn force_setup(mode: SignalHandlerMode) { - setup(mode) -} diff --git a/gix-tempfile/src/registry.rs b/gix-tempfile/src/registry.rs new file mode 100644 index 00000000000..a6e629c8fdc --- /dev/null +++ b/gix-tempfile/src/registry.rs @@ -0,0 +1,45 @@ +use crate::{NEXT_MAP_INDEX, REGISTRY}; +use std::sync::atomic::Ordering; + +/// Remove all tempfiles still registered on our global registry, and leak their data to be signal-safe. +/// This happens on a best-effort basis with all errors being ignored. +/// +/// # Safety +/// Note that Mutexes of any kind are not allowed, and so aren't allocation or deallocation of memory. +/// We are using lock-free datastructures and sprinkle in `std::mem::forget` to avoid deallocating. +/// Most importantly, we use `try_lock()` which uses an atomic int only without blocking, making our register method safe to use, +/// at the expense of possibly missing a lock file if another thread wants to obtain it or put it back +/// (i.e. mutates the registry shard). +pub fn cleanup_tempfiles_signal_safe() { + let current_pid = std::process::id(); + let one_past_last_index = NEXT_MAP_INDEX.load(Ordering::SeqCst); + for idx in 0..one_past_last_index { + if let Some(entry) = REGISTRY.try_entry(idx) { + entry.and_modify(|tempfile| { + if tempfile + .as_ref() + .map_or(false, |tf| tf.owning_process_id == current_pid) + { + if let Some(tempfile) = tempfile.take() { + tempfile.drop_without_deallocation(); + } + } + }); + } + } +} + +/// Remove all tempfiles still registered on our global registry. +/// This happens on a best-effort basis with all errors being ignored. +/// +/// # Note +/// +/// Must not be called from within signal hooks. For that, use [`cleanup_tempfiles_signal_safe()`]. +pub fn cleanup_tempfiles() { + let current_pid = std::process::id(); + REGISTRY.iter_mut().for_each(|mut tf| { + if tf.as_ref().map_or(false, |tf| tf.owning_process_id == current_pid) { + tf.take(); + } + }); +} diff --git a/gix-tempfile/src/signal.rs b/gix-tempfile/src/signal.rs new file mode 100644 index 00000000000..3e0e7729c17 --- /dev/null +++ b/gix-tempfile/src/signal.rs @@ -0,0 +1,100 @@ +use crate::REGISTRY; +use once_cell::sync::Lazy; + +/// Initialize signal handlers and other state to keep track of tempfiles, and **must be called before the first tempfile is created**, +/// allowing to set the `mode` in which signal handlers are installed. +/// +/// Only has an effect the first time it is called. +/// +/// Note that it is possible to not call this function and instead call +/// [registry::cleanup_tempfiles_signal_safe()][crate::registry::cleanup_tempfiles_signal_safe()] +/// from a signal handler under the application's control. +pub fn setup(mode: handler::Mode) { + handler::MODE.store(mode as usize, std::sync::atomic::Ordering::SeqCst); + Lazy::force(®ISTRY); +} + +/// +pub mod handler { + use std::sync::atomic::AtomicUsize; + + pub(crate) static MODE: AtomicUsize = AtomicUsize::new(Mode::None as usize); + + /// Define how our signal handlers act + #[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq)] + pub enum Mode { + /// Do not install a signal handler at all, but have somebody else call our handler directly. + None = 0, + /// Delete all remaining registered tempfiles on termination. + DeleteTempfilesOnTermination = 1, + /// Delete all remaining registered tempfiles on termination and emulate the default handler behaviour. + /// + /// This typically leads to the process being aborted. + DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour = 2, + } + + impl Default for Mode { + /// By default we will emulate the default behaviour and abort the process. + /// + /// While testing, we will not abort the process. + fn default() -> Self { + if cfg!(test) { + Mode::DeleteTempfilesOnTermination + } else { + Mode::DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour + } + } + } + + /// On linux we can handle the actual signal as we know it. + #[cfg(not(windows))] + pub(crate) fn cleanup_tempfiles_nix(sig: &libc::siginfo_t) { + crate::registry::cleanup_tempfiles_signal_safe(); + let restore_original_behaviour = Mode::DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour as usize; + if MODE.load(std::sync::atomic::Ordering::SeqCst) == restore_original_behaviour { + signal_hook::low_level::emulate_default_handler(sig.si_signo).ok(); + } + } + + /// On windows, assume sig-term and emulate sig-term unconditionally. + #[cfg(windows)] + pub(crate) fn cleanup_tempfiles_windows() { + crate::registry::cleanup_tempfiles_signal_safe(); + let restore_original_behaviour = Mode::DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour as usize; + if MODE.load(std::sync::atomic::Ordering::SeqCst) == restore_original_behaviour { + signal_hook::low_level::emulate_default_handler(signal_hook::consts::SIGTERM).ok(); + } + } + + #[cfg(test)] + mod tests { + use std::path::Path; + + use crate::{AutoRemove, ContainingDirectory}; + + fn filecount_in(path: impl AsRef) -> usize { + std::fs::read_dir(path).expect("valid dir").count() + } + + #[test] + fn various_termination_signals_remove_tempfiles_unconditionally() -> Result<(), Box> { + crate::signal::setup(Default::default()); + let dir = tempfile::tempdir()?; + for sig in signal_hook::consts::TERM_SIGNALS { + let _tempfile = crate::new(dir.path(), ContainingDirectory::Exists, AutoRemove::Tempfile)?; + assert_eq!( + filecount_in(dir.path()), + 1, + "only one tempfile exists no matter the iteration" + ); + signal_hook::low_level::raise(*sig)?; + assert_eq!( + filecount_in(dir.path()), + 0, + "the signal triggers removal but won't terminate the process (anymore)" + ); + } + Ok(()) + } + } +} diff --git a/gix-tempfile/tests/tempfile/mod.rs b/gix-tempfile/tests/tempfile/mod.rs index 4a1c983f56c..b22fc867bbf 100644 --- a/gix-tempfile/tests/tempfile/mod.rs +++ b/gix-tempfile/tests/tempfile/mod.rs @@ -1,11 +1,15 @@ mod fs; mod handle; +mod registry; +#[cfg(feature = "signals")] mod setup { #[test] fn can_be_called_multiple_times() { // we could probably be smart and figure out that this does the right thing, but… it's good enough it won't fail ;). - gix_tempfile::setup(gix_tempfile::SignalHandlerMode::DeleteTempfilesOnTermination); - gix_tempfile::setup(gix_tempfile::SignalHandlerMode::DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour); + gix_tempfile::signal::setup(gix_tempfile::signal::handler::Mode::DeleteTempfilesOnTermination); + gix_tempfile::signal::setup( + gix_tempfile::signal::handler::Mode::DeleteTempfilesOnTerminationAndRestoreDefaultBehaviour, + ); } } diff --git a/gix-tempfile/tests/tempfile/registry.rs b/gix-tempfile/tests/tempfile/registry.rs new file mode 100644 index 00000000000..219290ad938 --- /dev/null +++ b/gix-tempfile/tests/tempfile/registry.rs @@ -0,0 +1,30 @@ +use std::io::Write; +use std::path::Path; + +use gix_tempfile::{AutoRemove, ContainingDirectory}; + +fn filecount_in(path: impl AsRef) -> usize { + std::fs::read_dir(path).expect("valid dir").count() +} + +#[test] +fn cleanup_tempfiles() -> Result<(), Box> { + let dir = tempfile::tempdir()?; + let mut tempfile = gix_tempfile::new(dir.path(), ContainingDirectory::Exists, AutoRemove::Tempfile)?; + assert_eq!( + filecount_in(dir.path()), + 1, + "only one tempfile exists no matter the iteration" + ); + gix_tempfile::registry::cleanup_tempfiles(); + assert_eq!( + filecount_in(dir.path()), + 0, + "the signal triggers removal but won't terminate the process (anymore)" + ); + assert!( + tempfile.write_all(b"bogus").is_err(), + "cannot write into a tempfile that doesn't exist in registry" + ); + Ok(()) +} diff --git a/gix/src/interrupt.rs b/gix/src/interrupt.rs index 7e49ab68c5a..38aa7f05c90 100644 --- a/gix/src/interrupt.rs +++ b/gix/src/interrupt.rs @@ -89,7 +89,7 @@ mod init { } let msg_idx = INTERRUPT_COUNT.fetch_add(1, Ordering::SeqCst); if msg_idx == 1 { - gix_tempfile::handler::cleanup_tempfiles(); + gix_tempfile::registry::cleanup_tempfiles_signal_safe(); signal_hook::low_level::emulate_default_handler(*sig).ok(); } interrupt(); From c6785fc7082b90c8a27cef6a0f5cc5acd8cb8951 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 26 Feb 2023 17:22:42 +0100 Subject: [PATCH 3/3] adjust to changes in `gix-tempfile` --- gix/Cargo.toml | 2 +- gix/src/interrupt.rs | 2 +- tests/journey/gix.sh | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gix/Cargo.toml b/gix/Cargo.toml index bdbb57d27be..8b544c3fd1b 100644 --- a/gix/Cargo.toml +++ b/gix/Cargo.toml @@ -92,7 +92,7 @@ cache-efficiency-debug = ["gix-features/cache-efficiency-debug"] [dependencies] gix-ref = { version = "^0.24.1", path = "../gix-ref" } gix-discover = { version = "^0.13.1", path = "../gix-discover" } -gix-tempfile = { version = "^3.0.0", path = "../gix-tempfile" } +gix-tempfile = { version = "^3.0.0", path = "../gix-tempfile", features = ["signals"] } gix-lock = { version = "^3.0.0", path = "../gix-lock" } gix-validate = { version = "^0.7.3", path = "../gix-validate" } gix-sec = { version = "^0.6.2", path = "../gix-sec" } diff --git a/gix/src/interrupt.rs b/gix/src/interrupt.rs index 38aa7f05c90..c94cbdbfa89 100644 --- a/gix/src/interrupt.rs +++ b/gix/src/interrupt.rs @@ -100,7 +100,7 @@ mod init { } // This means that they won't setup a handler allowing us to call them right before we actually abort. - gix_tempfile::setup(gix_tempfile::SignalHandlerMode::None); + gix_tempfile::signal::setup(gix_tempfile::signal::handler::Mode::None); Ok(Deregister(hooks)) } diff --git a/tests/journey/gix.sh b/tests/journey/gix.sh index d2d924ed280..196ac442b2a 100644 --- a/tests/journey/gix.sh +++ b/tests/journey/gix.sh @@ -11,9 +11,9 @@ title "gix-tempfile crate" (when "running the example program to raise a signal with a tempfile present" it "fails as the process aborts" && { - expect_run $ABORTED cargo run --example delete-tempfiles-on-sigterm + expect_run $ABORTED cargo run --features signals --example delete-tempfiles-on-sigterm } - TEMPFILE="$(cargo run --example delete-tempfiles-on-sigterm 2>/dev/null || true)" + TEMPFILE="$(cargo run --features signals --example delete-tempfiles-on-sigterm 2>/dev/null || true)" it "outputs a tempfile with an expected name" && { expect_run $SUCCESSFULLY test "$TEMPFILE" = "tempfile.ext" } @@ -25,12 +25,12 @@ title "gix-tempfile crate" (when "running the example program to help assure there cannot be deadlocks" ABORTED=134 it "succeeds as it won't deadlock" && { - expect_run $ABORTED cargo run --release --example try-deadlock-on-cleanup -- 5 + expect_run $ABORTED cargo run --release --features signals --example try-deadlock-on-cleanup -- 1 } ) ) -title "gix-tempfile crate" +title "gix crate" (when "testing 'gix'" snapshot="$snapshot/gix" cd gix