Implement Windows SemLock in _multiprocessing module#7199
Open
youknowone wants to merge 6 commits intoRustPython:mainfrom
Open
Implement Windows SemLock in _multiprocessing module#7199youknowone wants to merge 6 commits intoRustPython:mainfrom
youknowone wants to merge 6 commits intoRustPython:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Windows-specific _multiprocessing implementation with SemLock/SemHandle types and full Win32 synchronization semantics, changes socket recv to return a truncated Changes
Sequence Diagram(s)sequenceDiagram
actor Py as Python caller
participant SemLock as SemLock (Rust)
participant Win32 as Win32 API
participant Kernel as Kernel/Handles
Py->>SemLock: acquire(timeout?)
SemLock->>Win32: WaitForSingleObjectEx(handle, timeout)
Win32->>Kernel: resolve wait on HANDLE
Kernel-->>Win32: WAIT_OBJECT_0 / WAIT_TIMEOUT / WAIT_FAILED
Win32-->>SemLock: result
alt acquired
SemLock-->>Py: True
else timeout
SemLock-->>Py: False
else error
SemLock-->>Py: raise OSError (include winerror)
end
Py->>SemLock: release()
SemLock->>Win32: ReleaseSemaphore / ReleaseMutex
Win32-->>SemLock: success / error
SemLock-->>Py: return / raise OSError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin win-semlock |
Add SemLock class using Windows semaphore APIs (CreateSemaphoreW, WaitForSingleObjectEx, ReleaseSemaphore) so test_multiprocessing suites are no longer skipped with "lacks a functioning sem_open". Also add sem_unlink as no-op and flags dict for Windows.
bbdfb4c to
067ca98
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/stdlib/src/multiprocessing.rs (1)
179-185: Fast-path ignoresWAIT_FAILED, causing a redundant system callWhen the fast-path
WaitForSingleObjectExreturnsWAIT_FAILED(handle invalid, etc.), the result is silently ignored and the main blocking wait is called immediately after, almost certainly returning the same error. Checking the fast-path result forWAIT_FAILEDsaves the redundant call:♻️ Proposed fix
- if unsafe { WaitForSingleObjectEx(self.handle.as_raw(), 0, 0) } == WAIT_OBJECT_0 { + let fast = unsafe { WaitForSingleObjectEx(self.handle.as_raw(), 0, 0) }; + if fast == WAIT_FAILED { + return Err(vm.new_last_os_error()); + } + if fast == WAIT_OBJECT_0 { self.last_tid .store(unsafe { GetCurrentThreadId() }, Ordering::Release); self.count.fetch_add(1, Ordering::Release); return Ok(true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/multiprocessing.rs` around lines 179 - 185, The fast-path call to WaitForSingleObjectEx in the block around self.handle currently ignores WAIT_FAILED; change that fast-path to check for WAIT_FAILED and immediately propagate the failure instead of falling through to the blocking wait. Specifically, in the fast-path around WaitForSingleObjectEx(self.handle.as_raw(), 0, 0) detect if the return equals WAIT_FAILED and return an Err created from the OS error (e.g., std::io::Error::last_os_error()) so the code in the blocking path (and callers relying on WaitForSingleObjectEx/WAIT_FAILED) aren't invoked redundantly; otherwise keep the existing success path that stores GetCurrentThreadId() into self.last_tid and increments self.count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/src/multiprocessing.rs`:
- Around line 327-329: The current validation allows maxvalue == 0 which leads
CreateSemaphoreW/WaitForSingleObjectEx to fail with an OS
ERROR_INVALID_PARAMETER; update the bounds check around args (the one that now
reads `if args.value < 0 || args.value > args.maxvalue`) to also reject
non‑positive maxvalue (e.g. `args.maxvalue <= 0`) and return a clear Python
ValueError via vm.new_value_error; mention
CreateSemaphoreW/WaitForSingleObjectEx in the comment if helpful and keep the
error message descriptive (e.g. "invalid maxvalue").
- Around line 78-83: Remove the stale GetLastError check block after
CreateSemaphoreW: the code that reads
windows_sys::Win32::Foundation::GetLastError(), compares to
0/ERROR_ALREADY_EXISTS, calls CloseHandle(handle) and then returns
vm.new_last_os_error() must be deleted because anonymous semaphores will not set
ERROR_ALREADY_EXISTS and a prior nonzero last-error can close a valid handle and
produce misleading errors; locate the sequence around CreateSemaphoreW,
GetLastError, CloseHandle and vm.new_last_os_error in multiprocessing.rs and
simply delete that entire branch; if named-semaphore support is added later,
implement the correct pattern: check only for last_err == ERROR_ALREADY_EXISTS,
save the error before CloseHandle, and restore it via SetLastError before
calling vm.new_last_os_error().
---
Nitpick comments:
In `@crates/stdlib/src/multiprocessing.rs`:
- Around line 179-185: The fast-path call to WaitForSingleObjectEx in the block
around self.handle currently ignores WAIT_FAILED; change that fast-path to check
for WAIT_FAILED and immediately propagate the failure instead of falling through
to the blocking wait. Specifically, in the fast-path around
WaitForSingleObjectEx(self.handle.as_raw(), 0, 0) detect if the return equals
WAIT_FAILED and return an Err created from the OS error (e.g.,
std::io::Error::last_os_error()) so the code in the blocking path (and callers
relying on WaitForSingleObjectEx/WAIT_FAILED) aren't invoked redundantly;
otherwise keep the existing success path that stores GetCurrentThreadId() into
self.last_tid and increments self.count.
…lity - recv() now returns bytes instead of int (matching CPython) - Remove spurious GetLastError() check after CreateSemaphoreW - Add signal checking during blocking SemLock acquire
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/logging dependencies:
dependent tests: (10 tests)
[x] lib: cpython/Lib/socket.py dependencies:
dependent tests: (53 tests)
Legend:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/stdlib/src/multiprocessing.rs (1)
21-24: Consider importingWAIT_TIMEOUT,WAIT_FAILED, andERROR_TOO_MANY_POSTSfromwindows-sysinstead of hardcoding.
WAIT_OBJECT_0is imported from the crate, butWAIT_TIMEOUT,WAIT_FAILED, andERROR_TOO_MANY_POSTSare hardcoded. Using the crate's constants avoids divergence if values change or types are redefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/multiprocessing.rs` around lines 21 - 24, Replace the hardcoded constants with the corresponding exports from the windows-sys crate: stop defining WAIT_TIMEOUT, WAIT_FAILED and ERROR_TOO_MANY_POSTS as local consts and instead import the matching symbols from windows-sys (the same place WAIT_OBJECT_0 is imported from) so types and values stay consistent; leave INFINITE if it isn’t provided by your current import but prefer using the windows-sys INFINITE if available, and update any references to WAIT_TIMEOUT, WAIT_FAILED and ERROR_TOO_MANY_POSTS in the module (e.g., where WAIT_OBJECT_0 is used) to use the imported names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/stdlib/src/multiprocessing.rs`:
- Around line 343-345: The current validation in multiprocessing.rs only checks
args.value against args.maxvalue but allows maxvalue == 0, which causes
CreateSemaphoreW to fail; update the validation that returns vm.new_value_error
to also reject non‑positive maxvalue (e.g., args.maxvalue <= 0) so that any call
where args.maxvalue is zero or negative triggers a clean ValueError before
calling CreateSemaphoreW; modify the existing check that uses args.value and
args.maxvalue to include this maxvalue > 0 requirement (referencing the same
args variable and the CreateSemaphoreW call).
---
Nitpick comments:
In `@crates/stdlib/src/multiprocessing.rs`:
- Around line 21-24: Replace the hardcoded constants with the corresponding
exports from the windows-sys crate: stop defining WAIT_TIMEOUT, WAIT_FAILED and
ERROR_TOO_MANY_POSTS as local consts and instead import the matching symbols
from windows-sys (the same place WAIT_OBJECT_0 is imported from) so types and
values stay consistent; leave INFINITE if it isn’t provided by your current
import but prefer using the windows-sys INFINITE if available, and update any
references to WAIT_TIMEOUT, WAIT_FAILED and ERROR_TOO_MANY_POSTS in the module
(e.g., where WAIT_OBJECT_0 is used) to use the imported names.
12e824e to
801a5ed
Compare
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/stdlib/src/multiprocessing.rs`:
- Around line 339-345: The constructor validation in py_new for the
multiprocessing primitive is missing a check that Args.maxvalue must be > 0
(CreateSemaphoreW requires lMaximumCount > 0); update the validation in py_new
(the function handling Args.kind, Args.value, Args.maxvalue against
RECURSIVE_MUTEX and SEMAPHORE) to explicitly reject maxvalue <= 0 by returning
vm.new_value_error("invalid maxvalue" or similar), and keep the existing value
vs maxvalue check (value < 0 || value > maxvalue) so invalid maxvalue is
rejected at Python level instead of provoking CreateSemaphoreW
ERROR_INVALID_PARAMETER.
When filename is None, __reduce__ was reconstructing a 2-element (errno, msg) tuple, dropping the winerror at position 3 in the original args. Use the original args tuple instead, matching CPython.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/exceptions.rs`:
- Around line 2064-2084: The Windows-specific branch that pushes winerror even
when filename2 is None (affecting args_reduced in OSError.__reduce__)
intentionally diverges from CPython to preserve winerror across pickle
round-trips; add a concise comment above the branch referencing filename2,
winerror, args_reduced and the TODO about errno/winerror to explain this
deliberate behavior (or, if you prefer CPython compatibility instead, remove the
winerror push in the else branch so the reduced tuple stays 3 elements when
filename2 is absent).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/stdlib/src/multiprocessing.rs (1)
385-395:recvnow correctly returns the actual bytes received — consider cappingsizetoi32::MAX.The truncation to
n_readis correct, and returningVec<u8>is the right abstraction. However, the pre-existingsize as i32cast on line 388 means asize > i32::MAXwould wrap to a negativelenforWinSock::recvwhile also allocating a multi-GB buffer. Consider clamping or validating:🛡️ Suggested size guard
fn recv(socket: usize, size: usize, vm: &VirtualMachine) -> PyResult<Vec<u8>> { + if size > i32::MAX as usize { + return Err(vm.new_overflow_error("recv size too large".to_owned())); + } let mut buf = vec![0u8; size];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/multiprocessing.rs` around lines 385 - 395, The recv function currently casts size to i32 and allocates a buffer of size bytes which can overflow or allocate huge memory when size > i32::MAX; before allocating or calling WinSock::recv (in function recv, referencing buf, size, and WinSock::recv), validate or clamp size to i32::MAX (e.g., cap a local len = size.min(i32::MAX as usize) or return an error for too-large sizes) and use that clamped len for buffer allocation and for the i32 cast passed to WinSock::recv so you never pass a negative length or allocate an oversized buffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/stdlib/src/multiprocessing.rs`:
- Around line 385-395: The recv function currently casts size to i32 and
allocates a buffer of size bytes which can overflow or allocate huge memory when
size > i32::MAX; before allocating or calling WinSock::recv (in function recv,
referencing buf, size, and WinSock::recv), validate or clamp size to i32::MAX
(e.g., cap a local len = size.min(i32::MAX as usize) or return an error for
too-large sizes) and use that clamped len for buffer allocation and for the i32
cast passed to WinSock::recv so you never pass a negative length or allocate an
oversized buffer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes