impl multiprocessing SemLock #6542
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a Unix-focused SemLock implementation and sem_unlink to Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant SemLock as _multiprocessing.SemLock
participant Handle as SemHandle
participant Kernel as POSIX Kernel
App->>SemLock: py_new(kind,value,maxvalue,name,unlink)
SemLock->>Handle: create/open semaphore
Handle->>Kernel: sem_open / sem_unlink + sem_open
Kernel-->>Handle: semaphore handle
Handle-->>SemLock: initialized
rect rgb(220,240,220)
Note over App,SemLock: Acquire flow
App->>SemLock: acquire(blocking, timeout)
SemLock->>SemLock: check owner / recursive count
alt already owner
SemLock->>SemLock: increment count
SemLock-->>App: True
else not owner
SemLock->>Handle: sem_timedwait / sem_wait
Handle->>Kernel: wait on semaphore
Kernel-->>Handle: success / timeout / error
Handle-->>SemLock: result
SemLock->>SemLock: update state (owner/count)
SemLock-->>App: True / False / raise
end
end
rect rgb(240,220,220)
Note over App,SemLock: Release flow
App->>SemLock: release()
SemLock->>SemLock: decrement count
alt count > 0
SemLock-->>App: still held
else count == 0
SemLock->>Handle: sem_post()
Handle->>Kernel: post semaphore
Kernel-->>Handle: ack
Handle-->>SemLock: success
SemLock-->>App: released
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
37aa27b to
b354fba
Compare
Co-authored-by: youknowone <69878+youknowone@users.noreply.github.com>
Co-authored-by: youknowone <69878+youknowone@users.noreply.github.com>
Co-authored-by: youknowone <69878+youknowone@users.noreply.github.com>
Co-authored-by: youknowone <69878+youknowone@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/stdlib/src/multiprocessing.rs (1)
233-242: Verify the null check in Drop -SEM_FAILEDis typically(sem_t*)-1, not null.The Drop implementation checks
!self.raw.is_null(), butlibc::SEM_FAILEDis usually((sem_t*)-1)on POSIX systems, not null. This should still be safe since a valid semaphore pointer won't be null, but consider adding a comment clarifying this is guarding against default/uninitialized state rather than SEM_FAILED.🔎 Suggested clarification
impl Drop for SemHandle { fn drop(&mut self) { + // Guard against default/uninitialized state. + // Note: SEM_FAILED is (sem_t*)-1, not null, but valid handles are never null. if !self.raw.is_null() { // SEM_CLOSE(sem) sem_close(sem) unsafe { libc::sem_close(self.raw); } } } }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
Lib/test/_test_multiprocessing.pyis excluded by!Lib/**Lib/test/mp_fork_bomb.pyis excluded by!Lib/**Lib/test/mp_preload.pyis excluded by!Lib/**Lib/test/mp_preload_flush.pyis excluded by!Lib/**Lib/test/test_importlib/test_threaded_import.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**
📒 Files selected for processing (4)
.cspell.dict/cpython.txtcrates/stdlib/src/multiprocessing.rscrates/stdlib/src/posixshmem.rscrates/stdlib/src/pystruct.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/stdlib/src/pystruct.rscrates/stdlib/src/posixshmem.rscrates/stdlib/src/multiprocessing.rs
🧠 Learnings (2)
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/stdlib/src/multiprocessing.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/stdlib/src/multiprocessing.rs
🧬 Code graph analysis (2)
crates/stdlib/src/pystruct.rs (1)
crates/vm/src/stdlib/ctypes/base.rs (1)
offset(1654-1656)
crates/stdlib/src/multiprocessing.rs (1)
crates/stdlib/src/faulthandler.rs (2)
current_thread_id(225-227)current_thread_id(230-232)
🔇 Additional comments (11)
crates/stdlib/src/pystruct.rs (1)
74-86: LGTM! Correct boundary check fix.The updated condition
offset + needed > buffer_lenproperly validates that the buffer has sufficient space for the entire operation, not just that the offset is within bounds. This fixes cases where the offset was valid but insufficient space remained for the required bytes.crates/stdlib/src/posixshmem.rs (1)
14-31: Clean refactoring to argument struct pattern.The
ShmOpenArgsstruct withFromArgsis a good improvement for argument handling. The default mode0o600is a secure choice for shared memory permissions.crates/stdlib/src/multiprocessing.rs (8)
62-140: LGTM! macOS polled fallback correctly implements CPython's approach.The exponential backoff (1ms to 20ms cap), deadline checking, and signal handling match CPython's
sem_timedwait_saveimplementation. Good handling of the three error cases.
286-289: Relaxed ordering in recursive mutex path may be correct but deserves scrutiny.The
fetch_add(1, Ordering::Relaxed)is used after theismine!check which usesAcquire. However, sinceismine!only reads, there's no synchronization with other threads that might be checking or modifyingcount. In practice, since only the owning thread (verified byismine!) modifiescountin this path, Relaxed should be safe. Consider adding a brief comment explaining this reasoning.
508-527: LGTM! Context manager methods correctly delegate to acquire/release.
529-555: LGTM! The_handleparameter is intentionally unused.The rebuild logic correctly opens the semaphore by name (the handle value is platform-specific and not portable for pickling). Initial count/last_tid of 0 is correct for a freshly rebuilt lock.
557-565: LGTM! Good fork safety handling.Resetting both
countandlast_tidafter fork is appropriate. The comment correctly notes thelast_tidreset is an addition over CPython.
606-630: Note: macOS_is_zerohas inherent TOCTOU limitation.The trywait/post pattern for checking zero on macOS has an inherent race: between the successful
sem_trywaitand thesem_postto restore the value, another thread could be unblocked by the post. This is a known limitation of the workaround and matches CPython's approach. The method's result should be considered advisory.
657-685: LGTM! Constructor with appropriate validation.The kind and value validation is correct. The
value as u32cast is safe due to the precedingvalue >= 0check.
729-754: LGTM! Helper functions are well-implemented.The
semaphore_namecorrectly handles POSIX naming conventions,os_errorproperly maps to specific exception types (FileExistsError, FileNotFoundError), andcurrent_thread_idmatches the pattern used elsewhere in the codebase..cspell.dict/cpython.txt (1)
33-44: LGTM! Dictionary entries support the new SemLock implementation.Both
ismine(matching CPython's ISMINE macro) andnewsemlockobject(matching CPython's internal function) are correctly added and alphabetically ordered.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
Lib/test/_test_multiprocessing.pyis excluded by!Lib/**Lib/test/mp_fork_bomb.pyis excluded by!Lib/**Lib/test/mp_preload.pyis excluded by!Lib/**Lib/test/mp_preload_flush.pyis excluded by!Lib/**Lib/test/test_importlib/test_threaded_import.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**
📒 Files selected for processing (4)
.cspell.dict/cpython.txtcrates/stdlib/src/multiprocessing.rscrates/stdlib/src/posixshmem.rscrates/stdlib/src/pystruct.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/stdlib/src/posixshmem.rs
- crates/stdlib/src/pystruct.rs
- .cspell.dict/cpython.txt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/stdlib/src/multiprocessing.rs
🧠 Learnings (2)
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/stdlib/src/multiprocessing.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/stdlib/src/multiprocessing.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (19)
crates/stdlib/src/multiprocessing.rs (19)
44-61: LGTM!The module setup and imports are appropriate for a POSIX semaphore implementation. The use of atomic types for shared state is correct.
62-140: LGTM! macOS workaround for missing sem_timedwait.The polled fallback implementation correctly handles the macOS limitation where
sem_timedwaitis unavailable. The exponential backoff (capped at 20ms), signal checking, and timeout handling align with CPython's approach.
142-152: LGTM!Constants match CPython's multiprocessing module. The
ismine!macro correctly usesAcquireordering to read shared state.
154-178: LGTM!Data structures are well-designed. The
unsafe impl Send/Sync for SemHandleis correct since POSIX semaphores are thread-safe.
231-243: Drop implementation is correct.The null check is sufficient since
SEM_FAILEDis never stored (errors are returned immediately at lines 200-202 and 218-221). The comment accurately explains this invariant.
245-266: LGTM!Property getters are straightforward and correct.
330-424: Acquire logic correctly handles signals and timeouts.The implementation properly:
- Attempts fast acquisition with
sem_trywait- Falls back to blocking wait with appropriate platform handling
- Checks signals after
EINTR- Uses correct memory ordering (
Release) when updatingcountandlast_tid
438-442: LGTM! Race condition resolved.The code now correctly uses
fetch_sub(line 440) for atomic decrement, addressing the race condition flagged in the previous review. TheReleaseordering ensures proper synchronization.
460-481: macOS maxvalue validation is limited to maxvalue==1.The
HAVE_BROKEN_SEM_GETVALUEworkaround on macOS only validates themaxvalue==1case. For highermaxvalue, releases are not validated. This is a known CPython limitation and is acceptable.
484-490: LGTM!The
sem_postcall and atomic count decrement correctly release the semaphore.
493-512: LGTM!Context manager implementation correctly delegates to
acquire()andrelease().
514-539: LGTM!The
_rebuildclassmethod correctly reconstructs a SemLock from pickled state, matching CPython's pattern. Initializingcountandlast_tidto zero is correct for a deserialized object.
541-611: LGTM! Introspection methods handle platform differences correctly.The implementations appropriately handle macOS's
HAVE_BROKEN_SEM_GETVALUElimitation:
_get_value()raisesNotImplementedErroron macOS_is_zero()uses thesem_trywait+sem_postworkaround on macOSThe
_after_fork()method correctly resets acquisition state for child processes.
613-635: LGTM!The
extend_classmethod correctly exposes constants and computesSEM_VALUE_MAXusingsysconf, with appropriate fallback toi32::MAX.
637-664: LGTM!Constructor properly validates inputs and initializes the SemLock with correct initial state.
666-676: LGTM!The
sem_unlinkfunction correctly wraps the POSIXsem_unlinkcall with appropriate error handling.
678-704: LGTM!The
flagsfunction correctly exposes platform capabilities matching CPython's feature detection.
706-731: LGTM! Helper functions are correctly implemented.The implementations match CPython's patterns:
semaphore_nameensures POSIX-compliant namingos_errormaps errno values to appropriate exception typescurrent_thread_idmatches the pattern used infaulthandler.rsBased on learnings, the platform-specific implementation approach is appropriate for RustPython.
44-732: Ensure code passescargo fmtandcargo clippychecks before merging.Per the repository's coding guidelines for Rust files, run
cargo fmtto format the code andcargo clippyto lint it, addressing any warnings or violations introduced by these changes.
| // deadline calculation: | ||
| // long sec = (long) timeout; | ||
| // long nsec = (long) (1e9 * (timeout - sec) + 0.5); | ||
| // deadline.tv_sec = now.tv_sec + sec; | ||
| // deadline.tv_nsec = now.tv_usec * 1000 + nsec; | ||
| // deadline.tv_sec += (deadline.tv_nsec / 1000000000); | ||
| // deadline.tv_nsec %= 1000000000; | ||
| let sec = timeout as libc::c_long; | ||
| let nsec = (1e9 * (timeout - sec as f64) + 0.5) as libc::c_long; | ||
| let mut deadline = libc::timespec { | ||
| tv_sec: tv.tv_sec + sec as libc::time_t, | ||
| tv_nsec: (tv.tv_usec as libc::c_long * 1000 + nsec) as _, | ||
| }; | ||
| deadline.tv_sec += (deadline.tv_nsec / 1_000_000_000) as libc::time_t; | ||
| deadline.tv_nsec %= 1_000_000_000; | ||
| Some(deadline) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Consider adding overflow checks for extremely large timeout values in deadline calculation.
The deadline arithmetic (lines 317-324) could silently overflow with extreme timeout values. The cast timeout as libc::c_long and the addition tv.tv_sec + sec lack bounds checking. If timeout approaches or exceeds c_long::MAX, the cast will wrap, producing incorrect deadline values. While this behavior mirrors CPython's implementation, RustPython should either validate input ranges before the cast, use saturating arithmetic (saturating_add, saturating_cast), or document this known limitation explicitly.
Consider adding a check like: if timeout > (c_long::MAX as f64) { ... handle error ... }
close #6506
fix #3965
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.