Newtype SigalNum#8096
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces a ChangesSignalNum wrapper and signal API refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/signal.py dependencies:
dependent tests: (99 tests)
Legend:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/signal.rs`:
- Around line 74-90: The `signal_handlers` borrow from
`signal_handlers.borrow()` is held across the `callable.invoke()` call, which
causes a panic if the Python signal handler tries to call back into
`_signal.signal()` to modify signal handlers due to a conflict on the same
`RefCell`. Restructure the code to extract the handler and convert it to a
callable inside a short borrow scope, then explicitly drop the borrow before
invoking the callable. This can be achieved by moving the handler lookup, the
callable extraction, and the invoke call outside the scope of the borrow, or by
using a scoped block that ensures the borrow is released before the invoke call
happens.
- Around line 203-210: The set_interrupt_ex function incorrectly compares a
validated SignalNum against SIG_DFL and SIG_IGN handler sentinel values, which
are not signal numbers. Since SIG_IGN equals 1 on Unix, this causes valid signal
1 to be silently ignored. Remove the match branch that checks for SIG_DFL or
SIG_IGN and instead always forward the validated SignalNum directly to
run_signal, letting the installed handler object determine default/ignore
behavior rather than the signal number itself.
In `@crates/vm/src/stdlib/_signal.rs`:
- Around line 425-427: The strsignal() function parameter type was changed to
SignalNum, which causes argument parsing to reject invalid integers before the
function can return None, breaking backward compatibility. Change the function
parameter from SignalNum back to i32, then perform the conversion from i32 to
SignalNum inside the function body before calling host_signal::strsignal(),
allowing the function to preserve its original behavior of returning None for
out-of-range integers instead of raising a ValueError.
- Around line 411-414: In crates/vm/src/stdlib/_signal.rs at lines 411-414, the
error mapping using vm.new_os_error() is being performed inside the
vm.allow_threads() closure, where the thread is detached and VM methods cannot
be safely called. Refactor this code to call only host_signal::raise_signal()
inside the allow_threads() closure, then move the error mapping operation
outside the closure. This ensures vm.new_os_error() is invoked only when the
thread is properly attached to the VM.
In `@crates/vm/src/stdlib/_thread.rs`:
- Line 22: The import of signal::SignalNum on line 22 is unconditional, but this
type is only used in the cfg-gated interrupt_main function (lines 618-620). Add
the appropriate cfg attribute to the signal::SignalNum import statement to match
the cfg guards that protect the interrupt_main function, preventing unused
import warnings on platforms where that function is not compiled. Run cargo
clippy to verify the warning is resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1cbe1453-3a39-4e30-96f8-6802515e5270
⛔ Files ignored due to path filters (1)
Lib/test/test_signal.pyis excluded by!Lib/**
📒 Files selected for processing (4)
crates/host_env/src/signal.rscrates/vm/src/signal.rscrates/vm/src/stdlib/_signal.rscrates/vm/src/stdlib/_thread.rs
| let signal_handlers = vm | ||
| .signal_handlers | ||
| .get() | ||
| .expect("should never fail since we check above") | ||
| .borrow(); | ||
|
|
||
| for (signum, trigger) in TRIGGERS.iter().enumerate().skip(1) { | ||
| let triggered = trigger.swap(false, Ordering::Relaxed); | ||
|
|
||
| // SAFETY: TRIGGERS has the same length as the signal_handlers | ||
| let signum = unsafe { SignalNum::new_unchecked(signum as i32) }; | ||
|
|
||
| if triggered | ||
| && let Some(handler) = &signal_handlers[signum] | ||
| && let Some(callable) = handler.to_callable() | ||
| { | ||
| callable.invoke((signum, vm.ctx.none()), vm)?; | ||
| callable.invoke((signum.as_i32(), vm.ctx.none()), vm)?; |
There was a problem hiding this comment.
Drop the signal_handlers borrow before invoking Python.
signal_handlers.borrow() now lives across callable.invoke(...). If the Python signal handler calls back into _signal.signal() to replace a handler, crates/vm/src/stdlib/_signal.rs, Lines 222-255, will hit borrow_mut() on the same RefCell and panic on a valid delivery path. Copy out the PyObjectRef (or callable) inside a short borrow scope, then invoke it after the borrow is dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/signal.rs` around lines 74 - 90, The `signal_handlers` borrow
from `signal_handlers.borrow()` is held across the `callable.invoke()` call,
which causes a panic if the Python signal handler tries to call back into
`_signal.signal()` to modify signal handlers due to a conflict on the same
`RefCell`. Restructure the code to extract the handler and convert it to a
callable inside a short borrow scope, then explicitly drop the borrow before
invoking the callable. This can be achieved by moving the handler lookup, the
callable extraction, and the invoke call outside the scope of the borrow, or by
using a scoped block that ensures the borrow is released before the invoke call
happens.
| pub fn set_interrupt_ex(signum: SignalNum) -> PyResult<()> { | ||
| use crate::stdlib::_signal::_signal::{SIG_DFL, SIG_IGN, run_signal}; | ||
| assert_in_range(signum, vm)?; | ||
|
|
||
| match signum as usize { | ||
| match signum.as_usize() { | ||
| SIG_DFL | SIG_IGN => Ok(()), | ||
| _ => { | ||
| // interrupt the main thread with given signal number | ||
| run_signal(signum); | ||
| run_signal(signum.into()); |
There was a problem hiding this comment.
Don't treat SIG_DFL/SIG_IGN as signal numbers here.
signum is already a validated SignalNum, but this branch still compares it to handler sentinel values. On Unix, SIG_IGN == 1, so _thread.interrupt_main(1) from crates/vm/src/stdlib/_thread.rs, Lines 616-621, becomes a silent no-op even though signal 1 is valid. set_interrupt_ex should forward every valid SignalNum to run_signal; default/ignore behavior is decided later from the installed handler object, not from the signal number itself.
Suggested fix
pub fn set_interrupt_ex(signum: SignalNum) -> PyResult<()> {
- use crate::stdlib::_signal::_signal::{SIG_DFL, SIG_IGN, run_signal};
-
- match signum.as_usize() {
- SIG_DFL | SIG_IGN => Ok(()),
- _ => {
- // interrupt the main thread with given signal number
- run_signal(signum.into());
- Ok(())
- }
- }
+ use crate::stdlib::_signal::_signal::run_signal;
+
+ // interrupt the main thread with given signal number
+ run_signal(signum.into());
+ Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn set_interrupt_ex(signum: SignalNum) -> PyResult<()> { | |
| use crate::stdlib::_signal::_signal::{SIG_DFL, SIG_IGN, run_signal}; | |
| assert_in_range(signum, vm)?; | |
| match signum as usize { | |
| match signum.as_usize() { | |
| SIG_DFL | SIG_IGN => Ok(()), | |
| _ => { | |
| // interrupt the main thread with given signal number | |
| run_signal(signum); | |
| run_signal(signum.into()); | |
| pub fn set_interrupt_ex(signum: SignalNum) -> PyResult<()> { | |
| use crate::stdlib::_signal::_signal::run_signal; | |
| // interrupt the main thread with given signal number | |
| run_signal(signum.into()); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/signal.rs` around lines 203 - 210, The set_interrupt_ex
function incorrectly compares a validated SignalNum against SIG_DFL and SIG_IGN
handler sentinel values, which are not signal numbers. Since SIG_IGN equals 1 on
Unix, this causes valid signal 1 to be silently ignored. Remove the match branch
that checks for SIG_DFL or SIG_IGN and instead always forward the validated
SignalNum directly to run_signal, letting the installed handler object determine
default/ignore behavior rather than the signal number itself.
| fn strsignal(signalnum: SignalNum) -> Option<String> { | ||
| host_signal::strsignal(signalnum.into()) | ||
| } |
There was a problem hiding this comment.
Preserve strsignal()'s None fallback for out-of-range integers.
Changing this parameter to SignalNum makes argument parsing reject invalid integers before the function can return None, so this turns a tolerant Python API into a ValueError-raising one. Keep the Python-facing argument as i32 and convert inside the function instead.
Suggested fix
- fn strsignal(signalnum: SignalNum) -> Option<String> {
- host_signal::strsignal(signalnum.into())
+ fn strsignal(signalnum: i32) -> Option<String> {
+ let signalnum = SignalNum::try_from(signalnum).ok()?;
+ host_signal::strsignal(signalnum.into())
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/stdlib/_signal.rs` around lines 425 - 427, The strsignal()
function parameter type was changed to SignalNum, which causes argument parsing
to reject invalid integers before the function can return None, breaking
backward compatibility. Change the function parameter from SignalNum back to
i32, then perform the conversion from i32 to SignalNum inside the function body
before calling host_signal::strsignal(), allowing the function to preserve its
original behavior of returning None for out-of-range integers instead of raising
a ValueError.
Summary
Summary by CodeRabbit
Release Notes
API Changes
SignalNuminstead of raw signal integers (includingsignal,getsignal,pidfd_send_signal,siginterrupt,strsignal, and_thread.interrupt_main).Bug Fixes
pidfd_send_signalsupport so it works on both Linux and Android.Chores