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 |
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/signal.py dependencies:
dependent tests: (99 tests)
Legend:
|
Sorry, something went wrong.
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
Sorry, something went wrong.
370fc76
into
RustPython:main
Jun 15, 2026
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