Merge branch 'push-yvzxzqrkkvry'

Byron · Dec 8, 2023 · 4917beb · 4917beb
2 parents ff1542c + 8ef0538
commit 4917beb
Show file tree

Hide file tree

Showing 11 changed files with 67 additions and 39 deletions.
diff --git a/gix/examples/clone.rs b/gix/examples/clone.rs
@@ -11,7 +11,10 @@ fn main() -> anyhow::Result<()> {
         .nth(2)
         .context("The second argument is the directory to clone the repository into")?;
 
-    gix::interrupt::init_handler(1, || {})?;
+    // SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
+    unsafe {
+        gix::interrupt::init_handler(1, || {})?;
+    }
     std::fs::create_dir_all(&dst)?;
     let url = gix::url::parse(repo_url.to_str().unwrap().into())?;
 

diff --git a/gix/examples/interrupt-handler-allows-graceful-shutdown.rs b/gix/examples/interrupt-handler-allows-graceful-shutdown.rs
@@ -6,7 +6,10 @@ fn main() -> anyhow::Result<()> {
 #[cfg(feature = "interrupt")]
 fn main() -> anyhow::Result<()> {
     use gix_tempfile::{AutoRemove, ContainingDirectory};
-    gix::interrupt::init_handler(1, || {})?;
+    // SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
+    unsafe {
+        gix::interrupt::init_handler(1, || {})?;
+    }
     eprintln!("About to emit the first term signal");
     let tempfile_path = std::path::Path::new("example-file.tmp");
     let _keep_tempfile = gix_tempfile::mark_at(tempfile_path, ContainingDirectory::Exists, AutoRemove::Tempfile)?;

diff --git a/gix/examples/reversible-interrupt-handlers.rs b/gix/examples/reversible-interrupt-handlers.rs
@@ -6,7 +6,8 @@ fn main() -> anyhow::Result<()> {
 #[cfg(feature = "interrupt")]
 fn main() -> anyhow::Result<()> {
     {
-        let _deregister_on_drop = gix::interrupt::init_handler(1, || {})?.auto_deregister();
+        // SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
+        let _deregister_on_drop = unsafe { gix::interrupt::init_handler(1, || {})?.auto_deregister() };
     }
     eprintln!("About to emit the first term signal, which acts just like a normal one");
     signal_hook::low_level::raise(signal_hook::consts::SIGTERM)?;

diff --git a/gix/src/clone/fetch/mod.rs b/gix/src/clone/fetch/mod.rs
@@ -47,7 +47,8 @@ impl PrepareFetch {
     ///
     /// ### Note for users of `async`
     ///
-    /// Even though
+    /// Even though `async` is technically supported, it will still be blocking in nature as it uses a lot of non-async writes
+    /// and computation under the hood. Thus it should be spawned into a runtime which can handle blocking futures.
     #[gix_protocol::maybe_async::maybe_async]
     pub async fn fetch_only<P>(
         &mut self,

diff --git a/gix/src/interrupt.rs b/gix/src/interrupt.rs
@@ -89,7 +89,10 @@ mod init {
     ///
     /// It will abort the process on second press and won't inform the user about this behaviour either as we are unable to do so without
     /// deadlocking even when trying to write to stderr directly.
-    pub fn init_handler(
+    ///
+    /// SAFETY: `interrupt()` will be called from a signal handler. See [`signal_hook::low_level::register()`] for details about.
+    #[allow(unsafe_code, clippy::missing_safety_doc)]
+    pub unsafe fn init_handler(
         grace_count: usize,
         interrupt: impl Fn() + Send + Sync + Clone + 'static,
     ) -> io::Result<Deregister> {
@@ -110,21 +113,22 @@ mod init {
             // * we only set atomics or call functions that do
             // * there is no use of the heap
             let interrupt = interrupt.clone();
+            let action = move || {
+                static INTERRUPT_COUNT: AtomicUsize = AtomicUsize::new(0);
+                if !super::is_triggered() {
+                    INTERRUPT_COUNT.store(0, Ordering::SeqCst);
+                }
+                let msg_idx = INTERRUPT_COUNT.fetch_add(1, Ordering::SeqCst);
+                if msg_idx == grace_count {
+                    gix_tempfile::registry::cleanup_tempfiles_signal_safe();
+                    signal_hook::low_level::emulate_default_handler(*sig).ok();
+                }
+                interrupt();
+                super::trigger();
+            };
             #[allow(unsafe_code)]
             unsafe {
-                let hook_id = signal_hook::low_level::register(*sig, move || {
-                    static INTERRUPT_COUNT: AtomicUsize = AtomicUsize::new(0);
-                    if !super::is_triggered() {
-                        INTERRUPT_COUNT.store(0, Ordering::SeqCst);
-                    }
-                    let msg_idx = INTERRUPT_COUNT.fetch_add(1, Ordering::SeqCst);
-                    if msg_idx == grace_count {
-                        gix_tempfile::registry::cleanup_tempfiles_signal_safe();
-                        signal_hook::low_level::emulate_default_handler(*sig).ok();
-                    }
-                    interrupt();
-                    super::trigger();
-                })?;
+                let hook_id = signal_hook::low_level::register(*sig, action)?;
                 hooks.push((*sig, hook_id));
             }
         }

diff --git a/gix/tests/interrupt.rs b/gix/tests/interrupt.rs
@@ -9,14 +9,18 @@ mod needs_feature {
         static V1: AtomicUsize = AtomicUsize::new(0);
         static V2: AtomicBool = AtomicBool::new(false);
 
-        let reg1 = gix::interrupt::init_handler(3, || {
-            V1.fetch_add(1, Ordering::SeqCst);
-        })
+        // SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
+        let reg1 = unsafe {
+            gix::interrupt::init_handler(3, || {
+                V1.fetch_add(1, Ordering::SeqCst);
+            })
+        }
         .expect("succeeds");
         assert!(!gix::interrupt::is_triggered());
         assert_eq!(V1.load(Ordering::Relaxed), 0);
-        let reg2 =
-            gix::interrupt::init_handler(2, || V2.store(true, Ordering::SeqCst)).expect("multi-initialization is OK");
+        // SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
+        let reg2 = unsafe { gix::interrupt::init_handler(2, || V2.store(true, Ordering::SeqCst)) }
+            .expect("multi-initialization is OK");
         assert!(!V2.load(Ordering::Relaxed));
 
         signal_hook::low_level::raise(SIGTERM).expect("signal can be raised");
@@ -36,13 +40,17 @@ mod needs_feature {
             "the deregistration succeeded and this is an optional side-effect"
         );
 
-        let reg1 = gix::interrupt::init_handler(3, || {
-            V1.fetch_add(1, Ordering::SeqCst);
-        })
+        // SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
+        let reg1 = unsafe {
+            gix::interrupt::init_handler(3, || {
+                V1.fetch_add(1, Ordering::SeqCst);
+            })
+        }
         .expect("succeeds");
         assert_eq!(V1.load(Ordering::Relaxed), 2, "nothing changed yet");
-        let reg2 =
-            gix::interrupt::init_handler(2, || V2.store(true, Ordering::SeqCst)).expect("multi-initialization is OK");
+        // SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
+        let reg2 = unsafe { gix::interrupt::init_handler(2, || V2.store(true, Ordering::SeqCst)) }
+            .expect("multi-initialization is OK");
         assert!(!V2.load(Ordering::Relaxed));
 
         signal_hook::low_level::raise(SIGTERM).expect("signal can be raised");

diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs
@@ -136,10 +136,14 @@ pub fn main() -> Result<()> {
     let auto_verbose = !progress && !args.no_verbose;
 
     let should_interrupt = Arc::new(AtomicBool::new(false));
-    gix::interrupt::init_handler(1, {
-        let should_interrupt = Arc::clone(&should_interrupt);
-        move || should_interrupt.store(true, Ordering::SeqCst)
-    })?;
+    #[allow(unsafe_code)]
+    unsafe {
+        // SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
+        gix::interrupt::init_handler(1, {
+            let should_interrupt = Arc::clone(&should_interrupt);
+            move || should_interrupt.store(true, Ordering::SeqCst)
+        })?;
+    }
 
     match cmd {
         Subcommands::Status(crate::plumbing::options::status::Platform {

diff --git a/src/porcelain/main.rs b/src/porcelain/main.rs
@@ -18,10 +18,14 @@ pub fn main() -> Result<()> {
         time::util::local_offset::set_soundness(time::util::local_offset::Soundness::Unsound);
     }
     let should_interrupt = Arc::new(AtomicBool::new(false));
-    gix::interrupt::init_handler(1, {
-        let should_interrupt = Arc::clone(&should_interrupt);
-        move || should_interrupt.store(true, Ordering::SeqCst)
-    })?;
+    #[allow(unsafe_code)]
+    unsafe {
+        // SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
+        gix::interrupt::init_handler(1, {
+            let should_interrupt = Arc::clone(&should_interrupt);
+            move || should_interrupt.store(true, Ordering::SeqCst)
+        })?;
+    }
     let trace = false;
     let verbose = !args.quiet;
     let progress = args.progress;

diff --git a/tests/snapshots/panic-behaviour/expected-failure b/tests/snapshots/panic-behaviour/expected-failure
@@ -1,3 +1,3 @@
-thread 'main' panicked at src/porcelain/main.rs:41:42:
+thread 'main' panicked at src/porcelain/main.rs:45:42:
 something went very wrong
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
diff --git a/tests/snapshots/panic-behaviour/expected-failure-in-thread b/tests/snapshots/panic-behaviour/expected-failure-in-thread
@@ -1,4 +1,4 @@
-thread 'main' panicked at src/porcelain/main.rs:41:42:
+thread 'main' panicked at src/porcelain/main.rs:45:42:
 something went very wrong
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
 [2K
diff --git a/tests/snapshots/panic-behaviour/expected-failure-in-thread-with-progress b/tests/snapshots/panic-behaviour/expected-failure-in-thread-with-progress
@@ -1,4 +1,4 @@
-[?1049h[?25lthread '<unnamed>' panicked at src/porcelain/main.rs:41:42:
+[?1049h[?25lthread '<unnamed>' panicked at src/porcelain/main.rs:45:42:
 something went very wrong
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
 [?25h[?1049l