Repository navigation
Align signal wakeup errors and delivery with Python 3.15 - #9199
Conversation
Assisted-by: OpenAI Codex:model version unavailable
Assisted-by: OpenAI Codex:model version unavailable
Assisted-by: OpenAI Codex:model version unavailable
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
⛔ Files ignored due to path filters: 1
📒 Files selected for processing: 5
✨ 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: :4: SyntaxWarning: invalid decimal literal dependencies:
dependent tests: (102 tests)
Legend:
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
- Report pending wakeup write/send errors after Python signal handlers and VM pending calls, and re-arm the eval breaker when a handler or pending call raises. - Save and restore errno once in run_signal around trip_signal instead of inside each write path; stop preserving _doserrno. - Add crt_fd::write_noraise and use it for the Unix and Windows wakeup writes, sharing the single-write helper with _write. - Keep pending wakeup errors across fork. - Revert the suppress_iph handler type change and the unrelated quote change in test_signal. Assisted-by: Claude Code:claude-opus-5-5
_set_thread_local_invalid_parameter_handler takes and returns a nullable function pointer; declare it as Option<InvalidParamHandler> so a NULL previous handler is restored as None. Assisted-by: Claude Code:claude-opus-5-5
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb5aa8e0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .signal_handlers | ||
| .get() | ||
| .expect("should never fail since we check above"); | ||
| let can_handle_signals = vm.is_main_thread() && vm.state.is_main_interpreter(); |
There was a problem hiding this comment.
When _interpreters.exec() runs a subinterpreter on the process main OS thread, signal.signal() still passes its existing guard because that guard only checks vm.is_main_thread(), and it stores the callable in the subinterpreter's signal_handlers. This new predicate then refuses to drain that table; once the main interpreter resumes, it consumes the trigger against a different handler table, so the registered callable is never invoked and the process signal may be silently swallowed. Add the missing is_main_interpreter() rejection to signal() before restricting delivery this way.
Useful? React with 👍 / 👎.
|
|
||
| @unittest.skipIf(_testcapi is None, 'need _testcapi') | ||
| @unittest.skipUnless(hasattr(os, "pipe"), "requires os.pipe()") | ||
| @force_not_colorized |
There was a problem hiding this comment.
Adding @force_not_colorized and its import changes test execution metadata, but this repository explicitly limits test-file edits to adding or removing expectedFailure markers. Revert this synchronization and validate the runtime behavior without modifying the CPython test in this way.
AGENTS.md reference: AGENTS.md:L280-L282
Useful? React with 👍 / 👎.
| #[cfg(target_os = "wasi")] | ||
| host_signal::notify_signal(signum, WAKEUP.load(Ordering::Relaxed)); |
There was a problem hiding this comment.
On wasm32-wasip1, a closed, read-only, or nonblocking-full wakeup fd still takes this branch, and crates/host_env/src/signal.rs's WASI notify_signal() discards the libc::write result while WAKEUP_WRITE_ERRORS and report_wakeup_errors() are not compiled for WASI. Consequently the warn_on_full_buffer value now stored by set_wakeup_fd() has no effect there and even the default True never reports the unraisable OSError; extend the pending-error path to WASI or retain the still-accurate TODO that this commit removed.
AGENTS.md reference: AGENTS.md:L165-L165
Useful? React with 👍 / 👎.
Restore the intentional policy from RustPython#9199 follow-up b46dcae. The mega replay incorrectly retained the old pending-error clearing loop. Remove only that loop, leaving the complete signal module identical to landed main and keeping the existing wakeup-fd invalidation unchanged. CPython retains these pending-call records across fork separately from signal flags. This correction does not claim prompt child delivery or change the existing RustPython eval-breaker scheduling behavior. Assisted-by: OpenAI Codex: model version unavailable
Integrate main at 5bccf31 without replacing the existing Ruff vendor. Preserve the landed Cranelift dependency family and RustPython#9282 original filename handling, including surrogate/bytes filenames and arbitrary warning-hook exceptions. Thread the same filename object through remaining mega numeric and relative-lazy warning/recovery paths without changing event ordering. The preceding signal correction restores RustPython#9199 pending-error preservation; it does not claim immediate post-fork delivery. Keep all tests unchanged. Assisted-by: OpenAI Codex: model version unavailable
Summary
Independent main-based signal-wakeup closure from #8954, following
Modules/signalmodule.cof Python 3.15:run_signalsaves and restores errno aroundtrip_signal, assignal_handlerdoes.crt_fd::write_noraise(_Py_write_noraise): EINTR is retried and the errno is returned without allocating. On Windows it shares the nonblocking-pipe ENOSPC handling with_write.senderrors) are stored in bounded pending slots and honorwarn_on_full_bufferon all platforms. They are reported only by the main interpreter's main thread, after Python signal handlers and VM pending calls, matching thehandle_signals→make_pending_callsorder of_Py_HandlePending._Py_ThreadCanHandleSignals)._set_thread_local_invalid_parameter_handleris declared with a nullableOption<InvalidParamHandler>, so a NULL previous handler is restored without UB.test_signalis synced with CPython v3.15.0 (force_not_colorizedontest_wakeup_write_error); existing RustPython markers are kept.Validation
test_signalpasses (run=57, skipped=26) on macOS.test_wakeup_write_erroris skipped without_testcapi. Its body (EBADF reported insidecaptured_stderrbefore a raising handler's exception escapes) and full-buffer reporting with default/True/Falsewarn_on_full_bufferproduce the same output on RustPython and CPython 3.15.0rc2.rustpython-host_envClippy passes forx86_64-pc-windows-msvc; Windows runtime behavior relies on CI.AI assistance: OpenAI Codex (model version unavailable) prepared the initial split; Claude Code (claude-opus-5-5) reworked it against signalmodule.c.
🤖 Generated with Claude Code