Skip to content

Commit 4917beb

Browse files
ByronByron
committed
Merge branch 'push-yvzxzqrkkvry'
2 parents ff1542c + 8ef0538 commit 4917beb

11 files changed

+67
-39
lines changed

Diff for: ‎gix/examples/clone.rs

+4-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ fn main() -> anyhow::Result<()> {
1111
.nth(2)
1212
.context("The second argument is the directory to clone the repository into")?;
1313

14-
gix::interrupt::init_handler(1, || {})?;
14+
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
15+
unsafe {
16+
gix::interrupt::init_handler(1, || {})?;
17+
}
1518
std::fs::create_dir_all(&dst)?;
1619
let url = gix::url::parse(repo_url.to_str().unwrap().into())?;
1720

Diff for: ‎gix/examples/interrupt-handler-allows-graceful-shutdown.rs

+4-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,10 @@ fn main() -> anyhow::Result<()> {
66
#[cfg(feature = "interrupt")]
77
fn main() -> anyhow::Result<()> {
88
use gix_tempfile::{AutoRemove, ContainingDirectory};
9-
gix::interrupt::init_handler(1, || {})?;
9+
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
10+
unsafe {
11+
gix::interrupt::init_handler(1, || {})?;
12+
}
1013
eprintln!("About to emit the first term signal");
1114
let tempfile_path = std::path::Path::new("example-file.tmp");
1215
let _keep_tempfile = gix_tempfile::mark_at(tempfile_path, ContainingDirectory::Exists, AutoRemove::Tempfile)?;

Diff for: ‎gix/examples/reversible-interrupt-handlers.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ fn main() -> anyhow::Result<()> {
66
#[cfg(feature = "interrupt")]
77
fn main() -> anyhow::Result<()> {
88
{
9-
let _deregister_on_drop = gix::interrupt::init_handler(1, || {})?.auto_deregister();
9+
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
10+
let _deregister_on_drop = unsafe { gix::interrupt::init_handler(1, || {})?.auto_deregister() };
1011
}
1112
eprintln!("About to emit the first term signal, which acts just like a normal one");
1213
signal_hook::low_level::raise(signal_hook::consts::SIGTERM)?;

Diff for: ‎gix/src/clone/fetch/mod.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,8 @@ impl PrepareFetch {
4747
///
4848
/// ### Note for users of `async`
4949
///
50-
/// Even though
50+
/// Even though `async` is technically supported, it will still be blocking in nature as it uses a lot of non-async writes
51+
/// and computation under the hood. Thus it should be spawned into a runtime which can handle blocking futures.
5152
#[gix_protocol::maybe_async::maybe_async]
5253
pub async fn fetch_only<P>(
5354
&mut self,

Diff for: ‎gix/src/interrupt.rs

+18-14
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,10 @@ mod init {
8989
///
9090
/// 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
9191
/// deadlocking even when trying to write to stderr directly.
92-
pub fn init_handler(
92+
///
93+
/// SAFETY: `interrupt()` will be called from a signal handler. See [`signal_hook::low_level::register()`] for details about.
94+
#[allow(unsafe_code, clippy::missing_safety_doc)]
95+
pub unsafe fn init_handler(
9396
grace_count: usize,
9497
interrupt: impl Fn() + Send + Sync + Clone + 'static,
9598
) -> io::Result<Deregister> {
@@ -110,21 +113,22 @@ mod init {
110113
// * we only set atomics or call functions that do
111114
// * there is no use of the heap
112115
let interrupt = interrupt.clone();
116+
let action = move || {
117+
static INTERRUPT_COUNT: AtomicUsize = AtomicUsize::new(0);
118+
if !super::is_triggered() {
119+
INTERRUPT_COUNT.store(0, Ordering::SeqCst);
120+
}
121+
let msg_idx = INTERRUPT_COUNT.fetch_add(1, Ordering::SeqCst);
122+
if msg_idx == grace_count {
123+
gix_tempfile::registry::cleanup_tempfiles_signal_safe();
124+
signal_hook::low_level::emulate_default_handler(*sig).ok();
125+
}
126+
interrupt();
127+
super::trigger();
128+
};
113129
#[allow(unsafe_code)]
114130
unsafe {
115-
let hook_id = signal_hook::low_level::register(*sig, move || {
116-
static INTERRUPT_COUNT: AtomicUsize = AtomicUsize::new(0);
117-
if !super::is_triggered() {
118-
INTERRUPT_COUNT.store(0, Ordering::SeqCst);
119-
}
120-
let msg_idx = INTERRUPT_COUNT.fetch_add(1, Ordering::SeqCst);
121-
if msg_idx == grace_count {
122-
gix_tempfile::registry::cleanup_tempfiles_signal_safe();
123-
signal_hook::low_level::emulate_default_handler(*sig).ok();
124-
}
125-
interrupt();
126-
super::trigger();
127-
})?;
131+
let hook_id = signal_hook::low_level::register(*sig, action)?;
128132
hooks.push((*sig, hook_id));
129133
}
130134
}

Diff for: ‎gix/tests/interrupt.rs

+18-10
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,18 @@ mod needs_feature {
99
static V1: AtomicUsize = AtomicUsize::new(0);
1010
static V2: AtomicBool = AtomicBool::new(false);
1111

12-
let reg1 = gix::interrupt::init_handler(3, || {
13-
V1.fetch_add(1, Ordering::SeqCst);
14-
})
12+
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
13+
let reg1 = unsafe {
14+
gix::interrupt::init_handler(3, || {
15+
V1.fetch_add(1, Ordering::SeqCst);
16+
})
17+
}
1518
.expect("succeeds");
1619
assert!(!gix::interrupt::is_triggered());
1720
assert_eq!(V1.load(Ordering::Relaxed), 0);
18-
let reg2 =
19-
gix::interrupt::init_handler(2, || V2.store(true, Ordering::SeqCst)).expect("multi-initialization is OK");
21+
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
22+
let reg2 = unsafe { gix::interrupt::init_handler(2, || V2.store(true, Ordering::SeqCst)) }
23+
.expect("multi-initialization is OK");
2024
assert!(!V2.load(Ordering::Relaxed));
2125

2226
signal_hook::low_level::raise(SIGTERM).expect("signal can be raised");
@@ -36,13 +40,17 @@ mod needs_feature {
3640
"the deregistration succeeded and this is an optional side-effect"
3741
);
3842

39-
let reg1 = gix::interrupt::init_handler(3, || {
40-
V1.fetch_add(1, Ordering::SeqCst);
41-
})
43+
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
44+
let reg1 = unsafe {
45+
gix::interrupt::init_handler(3, || {
46+
V1.fetch_add(1, Ordering::SeqCst);
47+
})
48+
}
4249
.expect("succeeds");
4350
assert_eq!(V1.load(Ordering::Relaxed), 2, "nothing changed yet");
44-
let reg2 =
45-
gix::interrupt::init_handler(2, || V2.store(true, Ordering::SeqCst)).expect("multi-initialization is OK");
51+
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
52+
let reg2 = unsafe { gix::interrupt::init_handler(2, || V2.store(true, Ordering::SeqCst)) }
53+
.expect("multi-initialization is OK");
4654
assert!(!V2.load(Ordering::Relaxed));
4755

4856
signal_hook::low_level::raise(SIGTERM).expect("signal can be raised");

Diff for: ‎src/plumbing/main.rs

+8-4
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,14 @@ pub fn main() -> Result<()> {
136136
let auto_verbose = !progress && !args.no_verbose;
137137

138138
let should_interrupt = Arc::new(AtomicBool::new(false));
139-
gix::interrupt::init_handler(1, {
140-
let should_interrupt = Arc::clone(&should_interrupt);
141-
move || should_interrupt.store(true, Ordering::SeqCst)
142-
})?;
139+
#[allow(unsafe_code)]
140+
unsafe {
141+
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
142+
gix::interrupt::init_handler(1, {
143+
let should_interrupt = Arc::clone(&should_interrupt);
144+
move || should_interrupt.store(true, Ordering::SeqCst)
145+
})?;
146+
}
143147

144148
match cmd {
145149
Subcommands::Status(crate::plumbing::options::status::Platform {

Diff for: ‎src/porcelain/main.rs

+8-4
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,14 @@ pub fn main() -> Result<()> {
1818
time::util::local_offset::set_soundness(time::util::local_offset::Soundness::Unsound);
1919
}
2020
let should_interrupt = Arc::new(AtomicBool::new(false));
21-
gix::interrupt::init_handler(1, {
22-
let should_interrupt = Arc::clone(&should_interrupt);
23-
move || should_interrupt.store(true, Ordering::SeqCst)
24-
})?;
21+
#[allow(unsafe_code)]
22+
unsafe {
23+
// SAFETY: The closure doesn't use mutexes or memory allocation, so it should be safe to call from a signal handler.
24+
gix::interrupt::init_handler(1, {
25+
let should_interrupt = Arc::clone(&should_interrupt);
26+
move || should_interrupt.store(true, Ordering::SeqCst)
27+
})?;
28+
}
2529
let trace = false;
2630
let verbose = !args.quiet;
2731
let progress = args.progress;

Diff for: ‎tests/snapshots/panic-behaviour/expected-failure

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
thread 'main' panicked at src/porcelain/main.rs:41:42:
1+
thread 'main' panicked at src/porcelain/main.rs:45:42:
22
something went very wrong
33
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Diff for: ‎tests/snapshots/panic-behaviour/expected-failure-in-thread

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
thread 'main' panicked at src/porcelain/main.rs:41:42:
1+
thread 'main' panicked at src/porcelain/main.rs:45:42:
22
something went very wrong
33
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
44


Diff for: ‎tests/snapshots/panic-behaviour/expected-failure-in-thread-with-progress

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
[?1049h[?25lthread '<unnamed>' panicked at src/porcelain/main.rs:41:42:
1+
[?1049h[?25lthread '<unnamed>' panicked at src/porcelain/main.rs:45:42:
22
something went very wrong
33
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
44
[?25h[?1049l

0 commit comments

Comments
 (0)