impl multiprocessing SemLock by youknowone · Pull Request #6542 · RustPython/RustPython
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_timedwait is 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 uses Acquire ordering to read shared state.
154-178: LGTM!
Data structures are well-designed. The unsafe impl Send/Sync for SemHandle is correct since POSIX semaphores are thread-safe.
231-243: Drop implementation is correct.
The null check is sufficient since SEM_FAILED is 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. The Release ordering ensures proper synchronization.
460-481: macOS maxvalue validation is limited to maxvalue==1.
The HAVE_BROKEN_SEM_GETVALUE workaround on macOS only validates the maxvalue==1 case. For higher maxvalue, releases are not validated. This is a known CPython limitation and is acceptable.
484-490: LGTM!
The sem_post call and atomic count decrement correctly release the semaphore.
493-512: LGTM!
Context manager implementation correctly delegates to acquire() and release().
514-539: LGTM!
The _rebuild classmethod correctly reconstructs a SemLock from pickled state, matching CPython's pattern. Initializing count and last_tid to 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_GETVALUE limitation:
_get_value()raisesNotImplementedErroron macOS_is_zero()uses thesem_trywait+sem_postworkaround on macOS
The _after_fork() method correctly resets acquisition state for child processes.
613-635: LGTM!
The extend_class method correctly exposes constants and computes SEM_VALUE_MAX using sysconf, with appropriate fallback to i32::MAX.
637-664: LGTM!
Constructor properly validates inputs and initializes the SemLock with correct initial state.
666-676: LGTM!
The sem_unlink function correctly wraps the POSIX sem_unlink call with appropriate error handling.
678-704: LGTM!
The flags function 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.rs
Based on learnings, the platform-specific implementation approach is appropriate for RustPython.
44-732: Ensure code passes cargo fmt and cargo clippy checks before merging.
Per the repository's coding guidelines for Rust files, run cargo fmt to format the code and cargo clippy to lint it, addressing any warnings or violations introduced by these changes.