7833: signal: remember the result of SetConsoleCtrlHandler#46
7833: signal: remember the result of SetConsoleCtrlHandler#46martin-augment wants to merge 1 commit intomasterfrom
SetConsoleCtrlHandler#46Conversation
The unix implementation remembers whether it failed or not, so let's mirror that for the windows implementation as well. This PR also simplifies the unix implementation by replacing the (`Once`, `AtomicBool`) pair with a single `OnceLock<Result<(), i32>>`.
WalkthroughThis change modernizes synchronization primitives in Tokio's signal handling modules by replacing ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Code Review: PR 46 - signal: remember the result of SetConsoleCtrlHandler Thank you for this PR! This is a well-structured refactoring that modernizes the signal initialization code by migrating from Once+AtomicBool to OnceLock. STRENGTHS:
CRITICAL ISSUES:
QUALITY OBSERVATIONS:
SECURITY:
TEST COVERAGE:
DOCUMENTATION:
PERFORMANCE:
SUMMARY:
Should Address: Overall, this is a solid refactoring. With the unwrap issues fixed, this will be ready to merge! |
🤖 Augment PR SummarySummary: Makes one-time signal handler initialization remember and reuse the first initialization result, so later callers don’t accidentally see success after a failure. Changes:
Technical Notes: Caches an OS error code ( 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| match signal_hook_registry::register_unchecked(signal, move |_| action(globals, signal)) | ||
| { | ||
| Ok(_) => Ok(()), | ||
| Err(e) => Err(e.raw_os_error().unwrap()), |
There was a problem hiding this comment.
e.raw_os_error().unwrap() can panic if the io::Error returned by signal_hook_registry isn’t backed by an OS errno (i.e., raw_os_error() is None), which would turn a registration failure into a process crash.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:useful; category:bug; feedback:The Augment AI reviewer is correct! Using .unwrap() looks safe here because the OS error should always be EINVAL. But to be prevent any eventual panic here it would be better to use .unwrap_or(EINVAL)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tokio/src/signal/unix.rstokio/src/signal/windows/sys.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: martin-augment
Repo: martin-augment/tokio PR: 11
File: tokio/src/util/usdt/stapsdt_aarch64.rs:1-3
Timestamp: 2025-11-04T09:48:44.541Z
Learning: In USDT (User Statically-Defined Tracing) probe implementations in Rust, semaphores must be declared as `static mut` primitive types (e.g., `static mut __usdt_sema_tokio_task__details: u16`), not atomic types. This is required for ABI compatibility with external tracing tools like DTrace and SystemTap, which modify these semaphores directly. The inline assembly code references the raw address of these semaphores, and using Rust atomic types would break the USDT mechanism.
📚 Learning: 2025-11-04T09:48:44.541Z
Learnt from: martin-augment
Repo: martin-augment/tokio PR: 11
File: tokio/src/util/usdt/stapsdt_aarch64.rs:1-3
Timestamp: 2025-11-04T09:48:44.541Z
Learning: In USDT (User Statically-Defined Tracing) probe implementations in Rust, semaphores must be declared as `static mut` primitive types (e.g., `static mut __usdt_sema_tokio_task__details: u16`), not atomic types. This is required for ABI compatibility with external tracing tools like DTrace and SystemTap, which modify these semaphores directly. The inline assembly code references the raw address of these semaphores, and using Rust atomic types would break the USDT mechanism.
Applied to files:
tokio/src/signal/unix.rs
📚 Learning: 2025-10-28T15:30:50.368Z
Learnt from: martin-augment
Repo: martin-augment/tokio PR: 9
File: tokio/src/fs/file.rs:824-855
Timestamp: 2025-10-28T15:30:50.368Z
Learning: In io-uring write implementations for tokio::fs::File, writes must loop until the buffer is fully drained. A single Op::write_at call may perform a partial write. The code must: (1) loop calling Op::write_at until buf.is_empty(), (2) return io::ErrorKind::WriteZero when 0 bytes are written, (3) only return Operation::Write(Ok(())) after the buffer is completely empty. This applies to both poll_write and poll_write_vectored implementations.
Applied to files:
tokio/src/signal/unix.rs
📚 Learning: 2025-11-05T14:24:03.551Z
Learnt from: martin-augment
Repo: martin-augment/tokio PR: 13
File: tokio-util/src/io/simplex.rs:184-209
Timestamp: 2025-11-05T14:24:03.551Z
Learning: In AsyncWrite implementations, zero-length writes (when buf.is_empty()) should return Poll::Ready(Ok(0)) immediately without blocking or registering wakers. Only non-empty buffers that cannot be written due to backpressure should return Poll::Pending. This prevents indefinite hangs when empty slices are written.
Applied to files:
tokio/src/signal/unix.rs
🧬 Code graph analysis (1)
tokio/src/signal/unix.rs (1)
tokio/src/signal/registry.rs (1)
globals(156-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (4)
tokio/src/signal/unix.rs (2)
17-17: LGTM: Clean import update.The import change from
Once/AtomicBooltoOnceLockaligns with the modernized synchronization approach.
241-245: LGTM: Cleaner synchronization primitive.The refactoring from separate
OnceandAtomicBoolfields toOnceLock<Result<(), i32>>is more idiomatic and simplifies the initialization pattern. The#[derive(Default)]works correctly since bothEventInfoandOnceLockimplementDefault.tokio/src/signal/windows/sys.rs (2)
2-2: LGTM: Import aligns with refactoring.The import of
OnceLockreplaces the previousOnceprimitive, consistent with the modernization across Unix and Windows signal modules.
87-99: LGTM: Consistent cross-platform initialization pattern.The refactoring to use
OnceLock<Result<(), i32>>mirrors the Unix implementation and simplifies the initialization logic. The.unwrap()on Line 93 is safe here becauseio::Error::last_os_error()always constructs errors with OS error codes fromGetLastError().The approach ensures that:
- Initialization happens exactly once
- Both success and failure results are cached
- Errors are propagated consistently via
from_raw_os_error
| siginfo | ||
| .init | ||
| .get_or_init(|| unsafe { | ||
| match signal_hook_registry::register_unchecked(signal, move |_| action(globals, signal)) | ||
| { | ||
| Ok(_) => Ok(()), | ||
| Err(e) => Err(e.raw_os_error().unwrap()), | ||
| } | ||
| }) | ||
| .map_err(io::Error::from_raw_os_error) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the code snippet exists and examine the context
cd /tmp && find . -name "unix.rs" -path "*/signal/*" 2>/dev/null | head -5Repository: martin-augment/tokio
Length of output: 46
🏁 Script executed:
# Search for the signal_hook_registry usage in the repository
rg -n "signal_hook_registry" --type rust -A 3 -B 3Repository: martin-augment/tokio
Length of output: 1926
🏁 Script executed:
# Look specifically for the register_unchecked usage
rg -n "register_unchecked" --type rust -A 5 -B 2Repository: martin-augment/tokio
Length of output: 573
🌐 Web query:
signal_hook_registry crate error handling raw_os_error
💡 Result:
Use the Error returned by register as a std::io::Error and call raw_os_error() on it.
Example:
let res = unsafe {
signal_hook_registry::register(signal_hook::consts::SIGTERM, || { /* ... */ })
};
match res {
Ok(id) => { /* registered */ }
Err(e) => {
if let Some(errno) = e.raw_os_error() {
eprintln!("register failed: errno = {}", errno);
} else {
eprintln!("register failed: {}", e);
}
}
}
Reference: signal_hook_registry::register returns Result<SigId, Error> where examples and docs use std::io::Error; use std::io::Error::raw_os_error() to get the OS errno. [1][2]
Sources:
- signal_hook_registry::register docs. [1]
- signal-hook crate docs / examples showing std::io::Error usage. [2]
Remove .unwrap() from line 293; signal_hook_registry errors don't always contain raw OS error codes.
The call to e.raw_os_error().unwrap() will panic if the error lacks a raw OS error code. The signal_hook_registry documentation shows that raw_os_error() returns Option<i32>, which requires proper handling. Use e.raw_os_error().unwrap_or(1) or similar fallback, or restructure to handle the None case explicitly.
🤖 Prompt for AI Agents
In tokio/src/signal/unix.rs around lines 287 to 296, remove the panic-causing
call to e.raw_os_error().unwrap() and handle the Option properly; replace the
unwrap with a safe fallback (for example e.raw_os_error().unwrap_or(1)) or
otherwise convert the error into a meaningful io::Error (e.g., map None to a
generic errno or io::Error::new(io::ErrorKind::Other, e.to_string())). Ensure
the Err branch never calls unwrap so it cannot panic when raw_os_error() returns
None.
There was a problem hiding this comment.
value:useful; category:bug; feedback:The CodeRabbit AI reviewer is correct! Using .unwrap() looks safe here because the OS error should always be EINVAL. But to be prevent any eventual panic here it would be better to use .unwrap_or(EINVAL)
value:useful; category:bug; feedback:The Claude AI reviewer is correct! Using |
value:useful; category:bug; feedback:The Claude AI reviewer is correct! Using |
2 participants
7833: To review by AI