Align winapi with CPython behavior #6988
Conversation
📝 WalkthroughWalkthroughAdds 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)
✅ Passed checks (2 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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin winapi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@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.
🧹 Nitpick comments (2)
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 callBatchedWaitForMultipleObjectssimultaneously on first use, both might create separate events, and only one will be stored viaset_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
CreateEventWreturns NULL (failure), this code stores 0 viaset_sigint_event(0). While the subsequentif handle == 0 { None }check at line 1380 handles this, it silently swallows the error. Consider logging or at least not callingset_sigint_eventon failure:♻️ Proposed improvement
let 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 });
| 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.
🧩 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()returnsERROR_ALREADY_EXISTS. [1] - Failure: returns
NULL. For extended error information, callGetLastError()(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)))📝 Committable suggestion
‼️ 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))) |
🤖 Prompt for AI Agents
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.
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 | 🟠 MajorFix off-by-one error in SIGINT event detection.
WaitForMultipleObjectsreturnsWAIT_OBJECT_0 + indexwhere index is 0-based. Since the sigint_event is appended as the last item at indexlen - 1, the condition should compare againstWAIT_OBJECT_0 + (len - 1), notWAIT_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 | 🟠 MajorHandle 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 callsGetLastError(), 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)); + } + } + }
1 participant
Summary by CodeRabbit