Fix signal handler after fork by youknowone · Pull Request #7012 · RustPython/RustPython · GitHub

Summary by CodeRabbit

Internal Changes
- Improved lazy initialization and thread-safety of signal handler storage.
- Signal APIs now enforce main-thread-only usage and will raise an error when invoked from non-main threads.
- After fork, the child process ensures signal handler state is initialized for the child’s main thread, avoiding missing handler state.

📝 Walkthrough

Walkthrough

Refactors VM signal handler storage from an Option to a OnceCell-backed lazy container, adds a crate helper to create the handler array, enforces main-thread-only signal operations, and ensures signal handlers are initialized in forked child processes.

Changes

Cohort / File(s)	Summary
VM signal storage crates/vm/src/vm/mod.rs, crates/vm/src/vm/thread.rs	Replaced Option<Box<RefCell<[Option<PyObjectRef>; signal::NSIG]>>> with OnceCell<Box<RefCell<[Option<PyObjectRef>; signal::NSIG]>>>; initialize with OnceCell::new() and add is_main_thread() helper.
Signal core API crates/vm/src/signal.rs	Added pub(crate) fn new_signal_handlers() -> Box<RefCell<[Option<PyObjectRef>; NSIG]>> and updated imports and access patterns to use RefCell + get()/get_or_init() instead of direct Option unwraps.
stdlib signal logic crates/vm/src/stdlib/signal.rs	Switched handler initialization to get_or_init(signal::new_signal_handlers), replaced optional-presence checks with vm.is_main_thread() for main-thread enforcement, and consistently use the lazy-initialized container in signal, getsignal, and set_wakeup_fd.
Post-fork child init crates/vm/src/stdlib/posix.rs	In py_os_after_fork_child, initialize vm.signal_handlers via get_or_init(crate::signal::new_signal_handlers) so forked children have a valid handler container.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    rect rgba(200,220,255,0.5)
    participant Py as Python code
    participant VM as VirtualMachine
    participant SIG as signal_handlers (OnceCell<Box<RefCell...>>)
    participant OS as OS/Thread
    end
    Py->>VM: call signal.signal(signum, handler)
    VM->>OS: check is_main_thread()
    alt not main thread
        VM-->>Py: raise ValueError
    else main thread
        VM->>SIG: get_or_init(new_signal_handlers)
        SIG->>SIG: borrow & store handler at index
        SIG-->>VM: stored
        VM-->>Py: return old handler
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

impl {raise_,str}signal #6411: Edits crates/vm/src/stdlib/signal.rs with overlapping signal handler access and initialization changes.
clear_after_fork #6933: Also touches child after-fork initialization for signal state in py_os_after_fork_child.
Update faulthandler to 3.14.2 #6949: Modifies crates/vm/src/signal.rs and signal processing logic that overlaps with the handler storage changes.

Suggested reviewers

ShaharNaveh

Poem

🐰 I hopped through OnceCell, neat and spry,
Handlers tucked where they safely lie,
Forked children wake with tidy stacks,
Main threads guard the signal tracks,
A little rabbit cheers — hop, apply! ✨

🚥 Pre-merge checks | ✅ 3 ✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main change: fixing signal handler initialization after fork by switching from Option to OnceCell and adding proper lazy initialization in child processes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

crates/vm/src/stdlib/signal.rs (1)
254-262: getsignal() lazily initializes signal handlers on non-main threads — consider using get() instead.

When getsignal is called on a child thread's VM (where OnceCell is empty), get_or_init allocates a new [Option<PyObjectRef>; NSIG] array just to return None for every signal. Using get() with a fallback would avoid this unnecessary allocation:
Suggested change
 fn getsignal(signalnum: i32, vm: &VirtualMachine) -> PyResult {
     signal::assert_in_range(signalnum, vm)?;
-    let signal_handlers = vm.signal_handlers.get_or_init(signal::new_signal_handlers);
-    let handler = signal_handlers.borrow()[signalnum as usize]
-        .clone()
-        .unwrap_or_else(|| vm.ctx.none());
-    Ok(handler)
+    let handler = vm
+        .signal_handlers
+        .get()
+        .and_then(|h| h.borrow()[signalnum as usize].clone())
+        .unwrap_or_else(|| vm.ctx.none());
+    Ok(handler)
 }

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.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/vm/src/stdlib/posix.rs (1)

783-798: ⚠️ Potential issue | 🟡 Minor

fork() does not handle libc::fork() returning -1 (error).

If libc::fork() fails it returns -1, but the function unconditionally runs py_os_after_fork_parent on the error path (since -1 != 0). This is pre-existing behavior and not introduced by this PR, but worth noting: the -1 is also returned directly to the Python caller without raising OSError.

🤖 Fix all issues with AI agents

In `@crates/vm/src/stdlib/signal.rs`:
- Around line 380-381: The error message in set_wakeup_fd is wrong; locate the
branch that checks vm.is_main_thread() inside the set_wakeup_fd implementation
and change the ValueError text from "signal only works in main thread" to
"set_wakeup_fd only works in main thread" so the raised error accurately names
the function (ensure the change occurs in the function/method named
set_wakeup_fd and only affects the vm.new_value_error call in that check).
- Around line 258-260: The getsignal() implementation currently enforces a
main-thread check using vm.is_main_thread() and returns
Err(vm.new_value_error(...)); remove that main-thread restriction so getsignal()
no longer returns an error for non-main threads: locate the getsignal function
in signal.rs, delete or bypass the vm.is_main_thread() check and its Err return,
and ensure the function simply returns the current handler value for the given
signal regardless of thread; keep the main-thread check only in signal.signal()
if present.

🧹 Nitpick comments (2)

crates/vm/src/stdlib/posix.rs (1)
714-718: Duplicated signal_handlers initialization expression across files.

The Box::new(const { RefCell::new([const { None }; NSIG]) }) pattern is repeated here, in signal.rs (lines 250, 263), and in vm/mod.rs (line 187). Consider extracting a helper (e.g., signal_handlers_default()) to keep this in one place and reduce divergence risk.
Example helper
// In crates/vm/src/signal.rs or a shared location:
pub(crate) fn new_signal_handlers() -> Box<RefCell<[Option<PyObjectRef>; NSIG]>> {
    Box::new(const { RefCell::new([const { None }; NSIG]) })
}
Then each call site becomes:
-vm.signal_handlers.get_or_init(|| {
-    Box::new(const { core::cell::RefCell::new([const { None }; crate::signal::NSIG]) })
-});
+vm.signal_handlers.get_or_init(signal::new_signal_handlers);
crates/vm/src/stdlib/signal.rs (1)
181-181: get().unwrap() in init_signal_handlers could panic if called on a worker thread.

Unlike signal() / getsignal() which use get_or_init, this uses get().unwrap(). This is safe today because module_exec runs during main-thread initialization where the OnceCell is pre-filled, but it's fragile. Consider using get_or_init for consistency, or adding a debug assertion with a clear message.
Proposed fix
-vm.signal_handlers.get().unwrap().borrow_mut()[signum] = py_handler;
+vm.signal_handlers
+    .get_or_init(|| Box::new(const { RefCell::new([const { None }; signal::NSIG]) }))
+    .borrow_mut()[signum] = py_handler;

- Change `signal_handlers` type from `Option<Box<...>>` to `OnceCell<Box<...>>` so fork children can lazily initialize it - Add `VirtualMachine::is_main_thread()` using runtime thread ID comparison instead of structural `signal_handlers.is_none()` check - Initialize signal handlers in `py_os_after_fork_child()` via `get_or_init()` for workers that fork - Use `get_or_init()` in `signal()` and `getsignal()` functions - Remove `set_wakeup_fd(-1)` special-case workaround (d6d0303) - Extract `signal::new_signal_handlers()` to deduplicate init expr - Allow `getsignal()` from any thread (matches CPython behavior) - Fix `set_wakeup_fd` error message to name the correct function

youknowone force-pushed the regrtest branch from 5a23e22 to 30f60b4 Compare February 8, 2026 16:03

youknowone mentioned this pull request Feb 8, 2026

Fix signal #7044

Closed

youknowone force-pushed the regrtest branch from 30f60b4 to 7c61ef6 Compare February 9, 2026 06:09

youknowone marked this pull request as ready for review February 9, 2026 08:25

youknowone changed the title temp regrtest save_env patch Fix signal handler after fork Feb 9, 2026

coderabbitai Bot reviewed Feb 9, 2026

View reviewed changes

Comment thread crates/vm/src/stdlib/signal.rs Outdated

Comment thread crates/vm/src/stdlib/signal.rs Outdated

youknowone added 2 commits February 9, 2026 23:30

temp regrtest save_env patch

bcb5a3d

youknowone force-pushed the regrtest branch from 7c61ef6 to c50f6a9 Compare February 9, 2026 14:30

youknowone merged commit c16f6f8 into RustPython:main Feb 9, 2026
14 checks passed

youknowone deleted the regrtest branch February 9, 2026 15:38

coderabbitai Bot mentioned this pull request Feb 17, 2026

Use try_lock in py_os_after_fork_child #7178

Merged

Fix signal handler after fork by youknowone · Pull Request #7012 · RustPython/RustPython · GitHub

Uh oh!

Fix signal handler after fork - #7012

Fix signal handler after fork#7012
youknowone merged 2 commits into
RustPython:mainfrom
youknowone:regrtest

youknowone commented Feb 5, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

Uh oh!

coderabbitai Bot commented Feb 5, 2026 •
edited
Loading

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Fix signal handler after fork by youknowone · Pull Request #7012 · RustPython/RustPython · GitHub

Uh oh!

Fix signal handler after fork - #7012

Fix signal handler after fork#7012youknowone merged 2 commits intoRustPython:mainRustPython/RustPython:mainfrom youknowone:regrtestyouknowone/RustPython:regrtestCopy head branch name to clipboard

Conversation

youknowone commented Feb 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Fix signal handler after fork#7012
youknowone merged 2 commits into
RustPython:mainfrom
youknowone:regrtest

youknowone commented Feb 5, 2026 •
edited by coderabbitai Bot
Loading

coderabbitai Bot commented Feb 5, 2026 •
edited
Loading