Align winapi with CPython behavior - #6988
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughAdds Windows-only SIGINT event storage and getter/setter, integrates SIGINT event signaling into signal delivery on Windows, and substantially refactors Windows stdlib winapi overlapped I/O, event creation, and wait APIs to return richer result shapes and handle more error cases. Changes
Sequence DiagramsequenceDiagram
participant Handler as Signal Handler
participant SigMod as crates::vm::signal
participant WinAPI as stdlib::winapi
participant WakeFD as wakeup_fd
Handler->>SigMod: deliver(SIGINT)
SigMod->>SigMod: run_signal logic
SigMod->>SigMod: get_sigint_event()
alt event handle exists
SigMod->>WinAPI: SetEvent(handle)
WinAPI-->>SigMod: success/err
end
SigMod->>WakeFD: write wakeup byte (if configured)
WakeFD-->>SigMod: acknowledged
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem🚥 Pre-merge checks | ✅ 2 | ❌ 1 ❌ Failed checks (1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin winapi |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agentsIn `@crates/vm/src/stdlib/winapi.rs`: - Around line 1210-1218: The error handling around CreateEventW is wrong: replace the check for INVALID_HANDLE_VALUE with a null check first — if handle.is_null() return Err(vm.new_last_os_error()); otherwise wrap the non-null handle in Ok(Some(WinHandle(handle))). Remove the INVALID_HANDLE_VALUE branch entirely so we don’t silently return Ok(None); ensure the return sequence in the function that constructs WinHandle uses is_null() first and only returns Ok(Some(...)) for non-null handles.
crates/vm/src/stdlib/winapi.rs (2)1369-1386: Potential race condition on SIGINT event initialization.
The get_sigint_event().unwrap_or_else(...) pattern at lines 1373-1379 can race: if two threads call BatchedWaitForMultipleObjects simultaneously on first use, both might create separate events, and only one will be stored via set_sigint_event. The other event handle would leak.
However, since this only runs on the main thread (checked at line 1371-1372), this race is unlikely in practice. Consider using an atomic compare-and-swap pattern for robustness if this becomes a concern.
1374-1378: CreateEventW failure not checked before calling set_sigint_event.
If CreateEventW returns NULL (failure), this code stores 0 via set_sigint_event(0). While the subsequent if handle == 0 { None } check at line 1380 handles this, it silently swallows the error. Consider logging or at least not calling set_sigint_event on failure:
♻️ Proposed improvementlet handle = crate::signal::get_sigint_event().unwrap_or_else(|| { let handle = unsafe { WinCreateEventW(null(), 1, 0, null()) }; - if !handle.is_null() { - crate::signal::set_sigint_event(handle as isize); + if handle.is_null() { + return 0; } + crate::signal::set_sigint_event(handle as isize); handle as isize });
Sorry, something went wrong.
| if handle == INVALID_HANDLE_VALUE { | ||
| return Err(vm.new_last_os_error()); | ||
| } | ||
|
|
||
| Ok(WinHandle(handle)) | ||
| if handle.is_null() { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| Ok(Some(WinHandle(handle))) |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain🌐 Web query:
Windows API CreateEventW return value failure NULL documentation
💡 Result:
CreateEventW returns a HANDLE.
- Success: returns a non-NULL handle to the event object. If the named event already existed, it still returns a valid handle, and GetLastError() returns ERROR_ALREADY_EXISTS. [1]
- Failure: returns NULL. For extended error information, call GetLastError() (immediately after the failure). [1]
Documentation: Microsoft Learn “CreateEventW function (synchapi.h)”. [1]
Sources
[1] Microsoft Learn: CreateEventW function (synchapi.h) – Win32 apps, return value section: (learn.microsoft.com)
Citations:
🏁 Script executed:
# Let's examine the actual code in the file
cd crates/vm/src/stdlib && head -n 1230 winapi.rs | tail -n 50Repository: RustPython/RustPython
Length of output: 1527
Fix CreateEventW error handling: it returns NULL on failure, not INVALID_HANDLE_VALUE.
The CreateEventW Windows API returns NULL on failure (documented in Microsoft Learn), not INVALID_HANDLE_VALUE. The current code incorrectly checks for INVALID_HANDLE_VALUE (which CreateEventW never returns) and then silently returns Ok(None) when the handle is null, losing the error information. The correct logic should check is_null() first and return an error:
Fix- if handle == INVALID_HANDLE_VALUE {
+ if handle.is_null() {
return Err(vm.new_last_os_error());
}
- if handle.is_null() {
- return Ok(None);
- }
-
Ok(Some(WinHandle(handle)))‼️ 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.
| if handle == INVALID_HANDLE_VALUE { | |
| return Err(vm.new_last_os_error()); | |
| } | |
| Ok(WinHandle(handle)) | |
| if handle.is_null() { | |
| return Ok(None); | |
| } | |
| Ok(Some(WinHandle(handle))) | |
| if handle.is_null() { | |
| return Err(vm.new_last_os_error()); | |
| } | |
| Ok(Some(WinHandle(handle))) |
In `@crates/vm/src/stdlib/winapi.rs` around lines 1210 - 1218, The error handling around CreateEventW is wrong: replace the check for INVALID_HANDLE_VALUE with a null check first — if handle.is_null() return Err(vm.new_last_os_error()); otherwise wrap the non-null handle in Ok(Some(WinHandle(handle))). Remove the INVALID_HANDLE_VALUE branch entirely so we don’t silently return Ok(None); ensure the return sequence in the function that constructs WinHandle uses is_null() first and only returns Ok(Some(...)) for non-null handles.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)crates/vm/src/stdlib/winapi.rs (2)1555-1577: ⚠️ Potential issue | 🟠 Major
Fix off-by-one error in SIGINT event detection.
WaitForMultipleObjects returns WAIT_OBJECT_0 + index where index is 0-based. Since the sigint_event is appended as the last item at index len - 1, the condition should compare against WAIT_OBJECT_0 + (len - 1), not WAIT_OBJECT_0 + len. The current code will never detect the interrupt.
Proposed fix- } else if sigint_event.is_some() - && result == WAIT_OBJECT_0 + thread_handles_raw.len() as u32 + } else if sigint_event.is_some() + && result == WAIT_OBJECT_0 + (thread_handles_raw.len() as u32 - 1) { Some(windows_sys::Win32::Foundation::ERROR_CONTROL_C_EXIT) } else {
1010-1035: ⚠️ Potential issue | 🟠 Major
Handle ConnectNamedPipe success in overlapped mode; current code ignores the return value.
For overlapped ConnectNamedPipe, a nonzero return indicates immediate success and requires signaling the event. The current code discards the return value and unconditionally calls GetLastError(), which treats successful synchronous completion as an error. The synchronous path (lines 45–50) correctly checks the return value first; apply the same pattern to the overlapped path.
Proposed fix- let _ret = { + let ret = { let mut inner = ov.inner.lock(); unsafe { windows_sys::Win32::System::Pipes::ConnectNamedPipe( handle.0, &mut inner.overlapped, ) } }; - let err = unsafe { GetLastError() }; - match err { - ERROR_IO_PENDING => { - let mut inner = ov.inner.lock(); - inner.pending = true; - } - ERROR_PIPE_CONNECTED => { - let inner = ov.inner.lock(); - unsafe { - windows_sys::Win32::System::Threading::SetEvent(inner.overlapped.hEvent); - } - } - _ => { - return Err(std::io::Error::from_raw_os_error(err as i32).to_pyexception(vm)); - } - } + if ret != 0 { + let inner = ov.inner.lock(); + unsafe { + windows_sys::Win32::System::Threading::SetEvent(inner.overlapped.hEvent); + } + } else { + let err = unsafe { GetLastError() }; + match err { + ERROR_IO_PENDING => { + let mut inner = ov.inner.lock(); + inner.pending = true; + } + ERROR_PIPE_CONNECTED => { + let inner = ov.inner.lock(); + unsafe { + windows_sys::Win32::System::Threading::SetEvent( + inner.overlapped.hEvent + ); + } + } + _ => { + return Err(std::io::Error::from_raw_os_error(err as i32).to_pyexception(vm)); + } + } + }
Sorry, something went wrong.
Summary by CodeRabbit