◐ Shell
reader mode source ↗
Skip to content

Newtype SigalNum#8096

Merged
youknowone merged 13 commits into
RustPython:mainfrom
ShaharNaveh:signals-newtype
Jun 15, 2026
Merged

Newtype SigalNum#8096
youknowone merged 13 commits into
RustPython:mainfrom
ShaharNaveh:signals-newtype

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Summary by CodeRabbit

Release Notes

  • API Changes

    • Updated signal-related APIs to use SignalNum instead of raw signal integers (including signal, getsignal, pidfd_send_signal, siginterrupt, strsignal, and _thread.interrupt_main).
    • Invalid signal numbers are rejected more consistently, with clearer conversion/validation behavior.
  • Bug Fixes

    • Improved pidfd_send_signal support so it works on both Linux and Android.
  • Chores

    • Strengthened overall signal handling safety and validation.

@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: adf9e996-f085-4257-bba4-65dd8e6417a7

📥 Commits

Reviewing files that changed from the base of the PR and between a7a355a and bbf319b.

📒 Files selected for processing (2)
  • crates/vm/src/stdlib/_signal.rs
  • crates/vm/src/stdlib/_thread.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/vm/src/stdlib/_thread.rs
  • crates/vm/src/stdlib/_signal.rs

📝 Walkthrough

Walkthrough

Introduces a SignalNum validated newtype replacing raw i32 signal numbers across the VM signal layer, stdlib _signal module, and _thread module. SignalHandlers is refactored to index by SignalNum via a new SignalHandlersInner type. Host env safety docs are clarified and is_valid_signal is removed.

Changes

SignalNum wrapper and signal API refactor

Layer / File(s) Summary
host_env safety doc updates and is_valid_signal removal
crates/host_env/src/signal.rs
Expands # Safety docs on probe_handler and install_handler to clarify caller requirements for valid signal numbers and ABI-accepted handlers, extends pidfd_send_signal to Android, and removes the Windows-only is_valid_signal(signalnum: i32) -> bool helper.
SignalNum type definition, conversions, and core VM signal handling
crates/vm/src/signal.rs
Defines the SignalNum newtype with VALID_RANGE, SIGINT, unchecked constructor, and conversion impls (From, TryFrom, TryFromObject, Display); refactors SignalHandlers to wrap SignalHandlersInner with Index/IndexMut keyed by SignalNum; updates module imports, trigger_signals dispatch loop, and set_interrupt_ex to accept and use SignalNum.
stdlib _signal: imports, constants, and initialization with SignalNum
crates/vm/src/stdlib/_signal.rs
Updates imports to bring SignalNum into scope, removes SIGNUM_RANGE constant, adds assertions on SignalNum::VALID_RANGE, rewrites init_signal_handlers to iterate SignalNum::VALID_RANGE and install SIGINT via SignalNum::SIGINT, and updates itimer error construction to use new error API.
stdlib _signal: signal/getsignal and error handling
crates/vm/src/stdlib/_signal.rs
Reworks signal and getsignal Python-exposed entry points to accept SignalNum, updating handler-table indexing and host installation; simplifies return types (getsignal no longer returns PyResult); wraps pause in vm.allow_threads; refines set_wakeup_fd error message.
stdlib _signal: platform-specific and utility functions
crates/vm/src/stdlib/_signal.rs
Updates pidfd_send_signal, siginterrupt, raise_signal, and strsignal to accept SignalNum; updates sigset_to_pyset and pthread_sigmask range filtering to use SignalNum::VALID_RANGE with corrected bounds formatting.
_thread::interrupt_main updated to SignalNum
crates/vm/src/stdlib/_thread.rs
Adds conditional import of SignalNum, refactors TIMEOUT_MAX_IN_MICROSECONDS to use runtime cfg!(...) expression, changes interrupt_main to accept OptionalArg<SignalNum> defaulting to SignalNum::SIGINT, and calls set_interrupt_ex(sig) without passing the VM.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • RustPython/RustPython#8089: Introduces and uses the SignalHandlers newtype in crates/vm/src/signal.rs, directly overlapping with this PR's SignalHandlersInner refactor.
  • RustPython/RustPython#7886: Both PRs modify pidfd_send_signal signal-handling and error propagation in crates/host_env/src/signal.rs.
  • RustPython/RustPython#8076: Modifies signal-number range/iteration and signal API behavior in crates/vm/src/stdlib/_signal.rs, the same file this PR updates with SignalNum::VALID_RANGE.

Suggested reviewers

  • coolreader18
  • youknowone

🐇 A number once bare as i32,
Now wrapped in SignalNum, safe to the core.
No raw ints shall pass through the gate,
The handler installs in a type-checked state.
Hop hop! The signals align just right. 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title references the main change (introducing a SignalNum newtype), but contains a typo ('Sigal' instead of 'Signal') that makes it unclear and less professional. Correct the typo to 'Newtype SignalNum' for clarity and accuracy.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/signal.py
[x] test: cpython/Lib/test/test_signal.py (TODO: 9)

dependencies:

  • signal

dependent tests: (99 tests)

  • signal: test_asyncio test_builtin test_concurrent_futures test_eintr test_faulthandler test_file_eintr test_fork1 test_inspect test_io test_largefile test_list test_os test_posix test_pty test_pyrepl test_regrtest test_runpy test_selectors test_signal test_socket test_socketserver test_subprocess test_support test_threading test_threadsignals test_unittest test_wsgiref
    • asyncio: test_asyncio test_logging test_pdb test_unittest
    • multiprocessing: test_compileall test_concurrent_futures test_fcntl test_genericalias test_memoryview test_multiprocessing_main_handling test_re
      • concurrent.futures.process: test_concurrent_futures
    • subprocess: test_android test_atexit test_audit test_bz2 test_c_locale_coercion test_cmd_line test_cmd_line_script test_ctypes test_dtrace test_embed test_gc test_gzip test_json test_launcher test_msvcrt test_ntpath test_osx_env test_peg_generator test_platform test_plistlib test_poll test_py_compile test_pyrepl test_quopri test_repl test_script_helper test_select test_shutil test_site test_sqlite3 test_sys test_sys_settrace test_sysconfig test_tempfile test_traceback test_unittest test_urllib2 test_utf8_mode test_venv test_wait3 test_webbrowser test_xpickle test_zipfile
      • ctypes.util: test_ctypes
      • ensurepip: test_ensurepip
      • http.server: test_httpservers test_robotparser test_urllib2_localnet test_xmlrpc
      • imaplib: test_imaplib
      • platform: test__locale test__osx_support test_baseexception test_cmath test_math test_mimetypes test_ssl test_strptime test_time test_winreg

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@ShaharNaveh ShaharNaveh marked this pull request as ready for review June 14, 2026 15:34

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide 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

📥 Commits

Reviewing files that changed from the base of the PR and between e423160 and a7a355a.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_signal.py is excluded by !Lib/**
📒 Files selected for processing (4)
  • crates/host_env/src/signal.rs
  • crates/vm/src/signal.rs
  • crates/vm/src/stdlib/_signal.rs
  • crates/vm/src/stdlib/_thread.rs

Hide details View details @youknowone youknowone merged commit 370fc76 into RustPython:main Jun 15, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants