mmap for windows #6329
mmap for windows #6329
Conversation
WalkthroughThe PR extends the mmap module from Unix-only to non-wasm32 platforms (including Windows), updates stdlib Cargo dependencies and Win32 feature flags, and implements Windows-specific mmap handling (creation, resizing, sizing, handle management, unified access, and resource cleanup). Changes
Sequence Diagram(s)sequenceDiagram
participant PyCode as Python code
participant PyCtor as PyMmap constructor
participant RustMmap as Rust mmap module
participant OSUnix as Unix APIs
participant OSWin as Windows APIs
participant VM as Virtual memory subsystem
PyCode->>PyCtor: mmap(fileno, length, access, ...)
PyCtor->>RustMmap: py_new(args)
alt unix path
RustMmap->>OSUnix: fstat(fd) / mmap(fd, offset, length, prot, flags)
OSUnix->>VM: create mapping
VM-->>RustMmap: mapped pointer / memmap object
RustMmap->>RustMmap: store fd, offset
else windows path
RustMmap->>OSWin: DuplicateHandle(fileno) / CreateFileMapping
OSWin->>VM: create file mapping
VM-->>RustMmap: mapping handle / view (MapViewOfFile)
RustMmap->>RustMmap: store HANDLE (AtomicCell), offset
end
RustMmap-->>PyCtor: return PyMmap object
Note over RustMmap,VM: On Drop -> Unmap/munmap and CloseHandle/cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 mmap |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/stdlib/src/mmap.rs (2)
1073-1087: Consider using mutable slice instead of raw pointer copy.The
copy_nonoverlappingwith a cast from*const u8to*mut u8works but bypasses borrow checker guarantees. Using the mutable slice interface is safer:// Copy data from old mmap to new mmap if let Some(old_mmap) = mmap_guard.as_ref() { let src = match old_mmap { MmapObj::Write(m) => &m[..copy_size], MmapObj::Read(m) => &m[..copy_size], }; - // Safety: we just created new_mmap and know it's large enough - unsafe { - std::ptr::copy_nonoverlapping( - src.as_ptr(), - new_mmap.as_ptr() as *mut u8, - copy_size, - ); - } + new_mmap[..copy_size].copy_from_slice(src); }
462-468: Acknowledged TODO for named mappings.The
tagnameparameter is documented as not yet implemented. Named file mappings are a Windows-specific feature for inter-process shared memory. This is acceptable for initial Windows support.Would you like me to open an issue to track the implementation of named mappings for Windows mmap?
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/stdlib/Cargo.toml(2 hunks)crates/stdlib/src/lib.rs(2 hunks)crates/stdlib/src/mmap.rs(15 hunks)
🧰 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 Rust 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/lib.rscrates/stdlib/src/mmap.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/lib.rscrates/stdlib/src/mmap.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/lib.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). (9)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (9)
crates/stdlib/Cargo.toml (2)
147-155: Windows-sys features are appropriate for the mmap implementation.The added features (
Win32_Foundation,Win32_Storage_FileSystem,Win32_System_Threading) align with the Windows API calls used inmmap.rs(DuplicateHandle,GetFileSize,SetEndOfFile,SetFilePointerEx,GetCurrentProcess, etc.).
106-112: Dependency grouping and version update looks good.The consolidation of mmap-related dependencies under
not(target_arch = "wasm32")correctly enables cross-platform support. The memmap2 version bump from 0.5.10 to 0.9.9 is stable—memmap2 0.9.9 does not introduce breaking changes. The significant API change in the 0.9 series occurred at 0.9.0 (Advice type split), not in 0.9.9.crates/stdlib/src/mmap.rs (5)
211-229: Drop implementation correctly prevents double-close.The atomic swap pattern ensures the handle is closed exactly once, whether via explicit
close()or implicit drop. The swap returns the old value, and the closure only proceeds if the handle was valid.
520-530: Large file size handling is correct but deserves attention.The
GetFileSizeAPI returnsINVALID_FILE_SIZE(0xFFFFFFFF) both on error and when the low 32 bits of a large file happen to be 0xFFFFFFFF. The check at line 525 correctly verifieslast_os_error() != Some(0)to distinguish real errors. This matches CPython's approach.
1202-1221: Windows size() implementation is correct.Properly distinguishes between anonymous mappings (returns internal size) and file-backed mappings (queries actual file size via GetFileSize with appropriate error handling).
880-895: madvise correctly gated to supported Unix platforms.The
madvisemethod is appropriately excluded from Windows and Redox where the underlying syscall is not available. Based on learnings, this follows RustPython's pattern of conditional declaration for platform-specific features.
350-457: Unix constructor implementation is solid.The Unix path correctly handles:
- Validation of offset and length
- Access mode mapping to prot/flags
- File descriptor duplication for ownership
- Anonymous mapping fallback
The offset type change to
i64aligns with the Windows implementation.crates/stdlib/src/lib.rs (2)
39-40: Module gating correctly extended to Windows.The
#[cfg(any(unix, windows))]correctly enables mmap on both platforms while maintaining the existing Unix support.
189-196: Module registration properly split.The separation of
posixsubprocess(Unix-only) frommmap(Unix and Windows) in the registration macro is correct and maintains the appropriate platform constraints.
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 (1)
crates/stdlib/src/mmap.rs (1)
775-792:resizeon Windows can operate on closed mmaps and resurrect them as anonymous mappings
close()setsclosed = true, clearsself.mmap, and setshandletoINVALID_HANDLE_VALUE. In the Windowsresizeimplementation:
- There is no
check_valid/ closed check up front.is_anonymousis computed purely fromhandle == INVALID_HANDLE_VALUE, so callingresize()on a closed file-backed mapping takes the anonymous branch.- A new anonymous
map_anonis created and stored inself.mmap, butclosedremainstrue.This lets
resize()“reopen” a closed mmap as an anonymous mapping while theclosedflag still readsTrue, which is inconsistent with the rest of the API and with CPython semantics (operations on closed mmaps should fail).A minimal fix is to reject operations on closed/invalid mappings at the start of
resizeon all platforms:#[cfg(windows)] #[pymethod] fn resize(&self, newsize: PyIntRef, vm: &VirtualMachine) -> PyResult<()> { - self.check_resizeable(vm)?; + // Disallow resizing closed/invalid mappings + { + // Drop the guard immediately; we only need the validity check. + let _ = self.check_valid(vm)?; + } + self.check_resizeable(vm)?; @@ Ok(()) }You might want the same
check_validpattern (or an explicitself.closed()test) in the Unix stub to align behavior across platforms.Also applies to: 1006-1121
♻️ Duplicate comments (1)
crates/stdlib/src/mmap.rs (1)
1267-1295:create_mmap_windowscan close the underlying HANDLE on mmap creation failure
File::from_raw_handletakes ownership of the HANDLE. Because the mapping calls use?inside thematch, any error frommap_mut/map/map_copywill:
- Cause an early return from
create_mmap_windows.- Drop
fileon the way out, closing the underlying HANDLE.- Leave the caller (e.g.
resize) still holding the raw handle value inself.handle, assuming it is valid.This can put the
PyMmapinto a partially broken state and may lead to later failures or incorrect resource handling.To ensure the handle is never closed here (it’s only borrowed), avoid
?inside thematch, capture theResultfirst, thenforget(file)before returning:use std::fs::File; @@ - // Create an owned handle wrapper for memmap2 - // We need to create a File from the handle - let file = unsafe { File::from_raw_handle(handle as RawHandle) }; + // Create an owned handle wrapper for memmap2, but don't let it + // close the HANDLE; we treat it as borrowed. + let file = unsafe { File::from_raw_handle(handle as RawHandle) }; @@ - let result = match access { - AccessMode::Default | AccessMode::Write => { - MmapObj::Write(unsafe { mmap_opt.map_mut(&file) }?) - } - AccessMode::Read => MmapObj::Read(unsafe { mmap_opt.map(&file) }?), - AccessMode::Copy => { - MmapObj::Write(unsafe { mmap_opt.map_copy(&file) }?) - } - }; - - // Don't close the file handle - we're borrowing it - std::mem::forget(file); - - Ok(result) + let result = match access { + AccessMode::Default | AccessMode::Write => { + unsafe { mmap_opt.map_mut(&file) }.map(MmapObj::Write) + } + AccessMode::Read => unsafe { mmap_opt.map(&file) }.map(MmapObj::Read), + AccessMode::Copy => unsafe { mmap_opt.map_copy(&file) }.map(MmapObj::Write), + }; + + // Don't close the file handle – we're only borrowing it + std::mem::forget(file); + + resultThis guarantees the HANDLE is never closed by
create_mmap_windows, regardless of whether the mapping itself succeeds or fails.
🧹 Nitpick comments (1)
crates/stdlib/src/mmap.rs (1)
218-235: Windowssize()returns length for both closed file-backed and anonymous mappings, creating inconsistency with Unix behaviorOn Windows,
size()returnsself.__len__()whenhandle == INVALID_HANDLE_VALUE, but this state occurs for both:
- True anonymous mappings (never had a file handle)
- Closed file-backed mappings (handle was set to
INVALID_HANDLE_VALUEinclose_handle())CPython's Unix implementation raises
ValueErrorwhen the mmap is closed. On Windows, CPython usesGetFileSizewhen a handle is present; when no valid handle exists, it returns the stored mapping size (similar to the current behavior). However, this conflation prevents distinguishing between intentionally anonymous mappings and erroneously closed ones.Consider:
- Raising an exception when the mapping is closed (tracking closure state separately), or
- Adding a separate
has_file_backingflag to distinguish closed file-backed mappings from true anonymous mappingsThis would align Windows behavior more closely with Unix (which errors on closed descriptors) and reduce surprising behavior.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_mmap.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/stdlib/Cargo.toml(2 hunks)crates/stdlib/src/lib.rs(2 hunks)crates/stdlib/src/mmap.rs(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/src/lib.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 Rust 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/mmap.rs
🧠 Learnings (2)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Applied to files:
crates/stdlib/src/mmap.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 using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/stdlib/src/mmap.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). (8)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (5)
crates/stdlib/Cargo.toml (3)
106-112: Clarify the purpose of networking crates in mmap support.The addition of
gethostname,socket2, anddns-lookupalongside memmap and page_size is unclear. These are networking-related crates, not directly related to memory mapping. Ensure these are not extraneous additions or confirm they are required for mmap functionality on Windows.
144-156: Approve Windows API features for mmap support.The added Windows features (
Win32_Foundation,Win32_Storage_FileSystem,Win32_System_Threading) are appropriate for implementing memory mapping on Windows:
Win32_Foundationprovides fundamental Windows typesWin32_Storage_FileSystemprovides file system APIs needed for mmap operationsWin32_System_Threadingsupports thread-safe operationsThe configuration looks correct for cross-platform mmap implementation.
106-112: Verify memmap2 API compatibility with the 0.9.9 upgrade.The memmap2 dependency is upgraded from 0.5.10 to 0.9.9, a significant version jump with documented breaking changes:
- The
Advicetype was split in 0.9.0 intoAdvice(for safe operations) andUncheckedAdvice(for unsafe operations). Code directly using the previousAdvicetype will require updates.- The
adviseAPI behavior changed in 0.8.0, with Advice variants becoming unsafe to construct directly.- The libc dependency was raised to >= 0.2.151 in v0.9.4.
Ensure your stdlib code using memmap2 has been updated to work with these API changes, particularly if it calls
advise(),advise_range(), or constructsAdvicevalues directly.crates/stdlib/src/mmap.rs (2)
192-199: Good refactor withMmapObj::as_sliceto unify read-only accessCentralizing read-only access in
MmapObj::as_sliceand reusing it inas_byteskeeps read paths consistent acrossMmap/MmapMutand removes duplicate matching logic. This looks clean and idiomatic; no issues spotted here.Also applies to: 708-713
261-276: Clarify Windows mmap semantics fortagnameparameterThe
tagnameparameter is accepted inMmapNewArgsbut never used in the implementation (see#[allow(dead_code)]). On Windows, whenfileno == -1, an anonymous mapping is created; otherwise,filenois passed to_get_osfhandle. This means:
- Named mappings via
tagnameare not supported, but callers won't be told this.- The
tagnameparameter accepts any value but silently ignores it.If Windows named mappings should be supported, implement the
tagnameparameter forfileno == -1cases or raiseNotImplementedErrorwhentagnameis provided. If they are intentionally unsupported, either remove the parameter or document this limitation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/stdlib/Cargo.toml (1)
110-112: Clarify if networking dependencies are required for mmap or other modules.The comment indicates "mmap + socket dependencies" but new networking-related dependencies (gethostname, socket2, dns-lookup) are added. These don't appear directly related to mmap functionality. Please clarify:
- Are these dependencies required for the socket module rather than mmap?
- Should the comment be updated to reflect the actual purpose of these dependencies?
crates/stdlib/src/mmap.rs (1)
1042-1048: Use mutable access instead of const-to-mut cast.The code obtains
new_mmap.as_ptr()which returns*const u8, then casts to*mut u8for the write. While this works, it's clearer to get mutable access directly.Apply this diff:
// Safety: we just created new_mmap and know it's large enough - unsafe { - std::ptr::copy_nonoverlapping( - src.as_ptr(), - new_mmap.as_ptr() as *mut u8, - copy_size, - ); - } + new_mmap[..copy_size].copy_from_slice(src);This is safer and more idiomatic.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_mmap.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/stdlib/Cargo.toml(2 hunks)crates/stdlib/src/lib.rs(2 hunks)crates/stdlib/src/mmap.rs(21 hunks)
🧰 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 Rust 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/lib.rscrates/stdlib/src/mmap.rs
🧠 Learnings (3)
📚 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/lib.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/lib.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Applied to files:
crates/stdlib/src/mmap.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 tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (17)
crates/stdlib/src/lib.rs (2)
39-40: LGTM! Cross-platform cfg for mmap module.The change correctly broadens mmap support from Unix-only to both Unix and Windows platforms.
189-196: LGTM! Module registration updated for cross-platform support.The mmap module registration is correctly moved from the Unix-only section to a cross-platform section with
any(unix, windows)gating, while posixsubprocess remains Unix-only as expected.crates/stdlib/Cargo.toml (2)
147-156: LGTM! Windows features correctly added for mmap support.The Windows-sys features Win32_Foundation, Win32_Storage_FileSystem, Win32_System_IO, and Win32_System_Threading are appropriate for the Windows mmap implementation requiring file handle management, file system operations, and handle duplication.
108-108: Verify memmap2 API compatibility after major version upgrade.The memmap2 dependency is upgraded from 0.5.10 to 0.9.9, a major version bump with confirmed breaking changes. Key migration points:
- The
Advicetype was split intoAdviceandUncheckedAdvicein 0.9.0; update any existing calls toadvise()/advise_range()orunchecked_advise()/unchecked_advise_range()accordingly- memmap2 0.9.4+ requires libc >= 0.2.151; verify this dependency is satisfied in your dependency graph
Ensure all memmap2 API usage in the codebase is updated for compatibility.
crates/stdlib/src/mmap.rs (13)
25-46: LGTM! Imports properly organized for cross-platform support.The imports are correctly gated with platform-specific cfg attributes:
- Unix-specific: nix, rustpython_common::crt_fd
- Windows-specific: Windows handle APIs, file system APIs, and suppress_iph macro
48-71: LGTM! validate_advice correctly restricted to Unix platforms.The madvise constants and validation are Unix-specific, so the cfg(unix) guard is appropriate. The implementation correctly validates advice values for different Unix variants.
181-188: LGTM! Unified data access via as_slice().The
as_slice()method provides a clean abstraction for accessing the underlying memory across both Read and Write variants of MmapObj. This simplifies data access throughout the codebase.
207-231: LGTM! Proper resource cleanup implemented.The
close_handle()method correctly closes file descriptors on Unix and handles on Windows, using atomic swap to avoid double-close. The Drop implementation ensures cleanup on object destruction. Handle/fd values are properly set to invalid sentinels after closing.
233-277: LGTM! Platform-specific constructor arguments with validation.The MmapNewArgs struct is appropriately split for Unix and Windows with platform-specific fields:
- Unix: flags, prot
- Windows: tagname
The validate_new_args() method correctly checks for negative values and returns the validated size.
369-462: LGTM! Unix constructor implementation preserved.The Unix implementation correctly:
- Validates arguments and determines map_size from file when length is 0
- Handles access mode conversion
- Duplicates the file descriptor to maintain independent ownership
- Creates appropriate mmap types based on access mode
778-780: LGTM! close() now properly closes underlying handle.The close method correctly calls
close_handle()to release the fd/handle resource in addition to dropping the mmap.
850-870: LGTM! madvise correctly gated for Unix (excluding redox).The madvise implementation is appropriately restricted to Unix platforms that support it, excluding redox. The call to
validate_adviceand libc::madvise is correct.
992-998: Unix resize remains unimplemented - placeholder is correct.The Unix resize method correctly returns an error indicating mremap() is not available. This is appropriate as the TODO comment indicates future implementation is planned.
1156-1183: LGTM! Windows size() implementation correct.The Windows implementation correctly:
- Returns mmap length for anonymous mappings (no underlying file)
- Uses GetFileSize for file-backed mappings
- Properly combines high and low 32-bit values for 64-bit file size
- Handles GetFileSize error cases
1261-1289: LGTM! Handle lifetime managed correctly with forget.The past review concern about handle closing on error has been addressed. The current implementation:
- Creates File from raw handle (takes ownership)
- Creates mmap using the File reference
- Uses
std::mem::forget(file)to prevent File from closing the handle on drop- Returns the mmap result
If mmap creation fails and returns early via
?on lines 1279, 1281, or 1282, the File is dropped before the forget call, which correctly closes the handle. However, this is actually incorrect behavior for the caller's expectations.Wait, let me re-read the past review comment... The past comment says "the caller (in resize) still expects to own the handle after an error."
Looking at the resize function (lines 1073-1096), after calling create_mmap_windows, if it returns an error, the code attempts to restore the original mmap using the same handle. But if create_mmap_windows closed the handle on error, then the restore attempt would fail or use an invalid handle.
Actually, I see the fix was applied: the code no longer uses
?inside the match. Instead:
- Line 1277: Creates the result without
?- Line 1286: Calls forget unconditionally
- Line 1288: Returns the result (propagates error if any)
This ensures forget is always called, preventing the handle from being closed even on error. This is correct for the caller's expectations.
However, there's still an issue: If the mmap APIs return an error, we call forget, then return the error. The handle remains open and owned by the caller, but the caller (resize) expects it to be in the PyMmap's handle field, which it already is. So this is actually correct.
Based on learnings, the implementation correctly follows the pattern established in the codebase for Windows handle management.
464-609: Code approved — handle cleanup is correct on all paths.The Windows constructor properly manages the duplicated handle across all error cases:
- Pre-transfer errors (lines 516–562): All explicitly close
duplicated_handlebefore returning.- Post-transfer on mmap failure (line 585): The
OwnedHandleautomatically drops whenmmap_resultfails, triggering itsDropimpl which callsCloseHandle.- Success path (lines 588–589): The raw handle is extracted and
OwnedHandleis forgotten, preventingDropfrom closing the handle prematurely.No resource leaks exist.
1000-1115: Correct the error recovery analysis; the resize implementation properly handles file size mismatches.The concern about error recovery at lines 1073-1096 is not valid. Windows API guarantees that if SetEndOfFile fails, the file size remains unchanged on disk. Since the code already drops the mmap at line 1055 before attempting SetFilePointerEx/SetEndOfFile, recreating the mmap with
old_sizeafter SetEndOfFile failure is correct—the file will still be at its original size.The only remaining minor consideration is that the pointer cast at lines 1042-1048 (casting
*const u8to*mut u8) is valid but could use a mutable reference for clarity, though this is a style preference rather than a correctness issue.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/stdlib/src/mmap.rs (2)
250-264: Consider documenting whytagnameis unused.The
tagnameparameter is part of Python's mmap API on Windows but is currently marked as#[allow(dead_code)]. Consider adding a comment explaining that tagname support (for named shared memory) is not yet implemented, or consider implementing it for full Python compatibility.
272-274: Minor: Error message slightly misleading.The check
offset < 0correctly allows zero, but the error message says "must be positive". Consider changing to "must be non-negative" for accuracy.if self.offset < 0 { - return Err(vm.new_overflow_error("memory mapped offset must be positive")); + return Err(vm.new_overflow_error("memory mapped offset must be non-negative")); }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_mmap.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/stdlib/Cargo.toml(2 hunks)crates/stdlib/src/lib.rs(2 hunks)crates/stdlib/src/mmap.rs(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/stdlib/Cargo.toml
- crates/stdlib/src/lib.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 Rust 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/mmap.rs
🧠 Learnings (1)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Applied to files:
crates/stdlib/src/mmap.rs
🧬 Code graph analysis (1)
crates/stdlib/src/mmap.rs (1)
crates/common/src/crt_fd.rs (1)
try_borrow_raw(260-266)
⏰ 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 snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (12)
crates/stdlib/src/mmap.rs (12)
30-46: LGTM!Platform-specific imports are properly gated with
#[cfg(unix)]and#[cfg(windows)]attributes. The Windows imports cover the necessary Win32 APIs for handle management and file operations.
48-71: LGTM!The
validate_advicefunction is correctly restricted to Unix sincemadviseis a Unix-specific system call not available on Windows.
181-188: LGTM!The
as_slice()method provides a clean unified access pattern for bothReadandWritevariants, reducing code duplication throughout the file.
207-231: LGTM!The
close_handle()method uses atomic swap with sentinel values (-1 for Unix fd,INVALID_HANDLE_VALUEfor Windows handle) to ensure thread-safe, idempotent cleanup. TheDropimplementation guarantees resources are released even ifclose()is not explicitly called.
570-595: LGTM!The
OwnedHandlepattern correctly manages the duplicated handle lifecycle. If mmap creation fails at line 585, theOwnedHandleis dropped which closes the duplicated handle - this is correct behavior since we don't want to leak the handle on error. If mmap succeeds, the handle is extracted and forgotten to maintain ownership in thePyMmapstruct.
514-522: LGTM!Correct error handling pattern for
GetFileSize. Per Windows API documentation,INVALID_FILE_SIZE(u32::MAX) can indicate either an error or a legitimate size, so checkingGetLastError() == NO_ERRORis the proper approach.
850-870: LGTM!The
madviseimplementation correctly validates the advice value first, then calls the libc function directly with proper pointer casting and error handling.
1254-1282: LGTM!The fix from the previous review is correctly applied. The
Fileis forgotten before returning (regardless of success/failure), ensuring the handle is never closed by this function. The caller inresize()maintains ownership of the handle.
1157-1176: LGTM!Correct differentiation between anonymous mappings (returning internal size) and file-backed mappings (querying actual file size via
GetFileSize).
540-564: LGTM!The file extension logic correctly matches CPython's Windows behavior where requesting a larger map size than the file size automatically extends the file. Using
SetFilePointerEx+SetEndOfFileis the correct Windows API pattern.
697-702: LGTM!Consistent use of
as_slice()throughout the codebase for unified data access replaces previous variant matching.
406-427: LGTM!The Unix implementation correctly handles the
i64offset type, with proper file size validation and bounds checking.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/stdlib/src/mmap.rs (2)
570-594: Consider overflow check for file resize calculation.At line 571,
required_size = offset + map_size as i64could theoretically overflow i64 if both values are very large. While unlikely in practice (would require petabyte-scale files), an explicit overflow check would make this more defensive.Consider adding:
} else { // If map_size > file_len, extend the file (Windows behavior) let required_size = offset + map_size as i64; + if required_size < offset { + unsafe { CloseHandle(duplicated_handle) }; + return Err(vm.new_overflow_error("mmap size would cause file size overflow")); + } if required_size > file_len {
1100-1125: Consider extracting duplicate error recovery logic.The error recovery blocks at lines 1100-1108 and 1116-1124 are nearly identical:
match Self::create_mmap_windows(...) { Ok(mmap) => *mmap_guard = Some(mmap), Err(_) => self.closed.store(true), }This pattern appears twice in the file-backed resize path.
Consider extracting into a helper:
fn try_restore_mmap( &self, mmap_guard: &mut Option<MmapObj>, handle: HANDLE, size: usize, ) -> bool { match Self::create_mmap_windows(handle, self.offset, size, &self.access) { Ok(mmap) => { *mmap_guard = Some(mmap); true } Err(_) => { self.closed.store(true); false } } }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/stdlib/src/mmap.rs(23 hunks)
🧰 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 Rust 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/mmap.rs
🧠 Learnings (3)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Applied to files:
crates/stdlib/src/mmap.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 using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/stdlib/src/mmap.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/mmap.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). (10)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
🔇 Additional comments (8)
crates/stdlib/src/mmap.rs (8)
600-625: Verify handle cleanup on mmap creation failure.The code creates an
OwnedHandle, attempts mmap creation, and on success forgets the handle and stores the raw value. If mmap creation fails (line 615), theOwnedHandleis dropped, which closes the handle. Then the error is propagated.However, since the handle was duplicated specifically for this mmap object, closing it on mmap creation failure is the correct behavior. The caller still owns their original handle.
This pattern correctly ensures:
- Success path: handle kept alive via raw storage, closed by PyMmap::Drop
- Failure path: handle closed immediately via OwnedHandle::Drop
1296-1324: LGTM - Previous handle ownership issue resolved.The function now correctly ensures
std::mem::forget(file)is always called before returning, preventing the handle from being closed on error. The restructured code using.map()instead of?inside the match guarantees the forget call happens regardless of mmap creation success or failure.
1053-1079: LGTM - Anonymous mmap resize correctly preserves data.The implementation correctly:
- Creates the new mmap before dropping the old one, ensuring the original remains valid if creation fails
- Copies only the minimum of old and new size, which handles both shrink and grow cases
- Handles both Read and Write variants of MmapObj appropriately
202-209: Good refactoring - unified data access.The
as_slice()method provides a clean abstraction over the Read/Write variants, eliminating repetitive pattern matching throughout the codebase. This improves maintainability and reduces the chance of errors.
179-189: PAGESIZE/ALLOCATIONGRANULARITY guards updated for cross-platform support.The cfg guards changed from excluding
redoxto excludingwasm32. This makes sense because:
- The page_size crate likely supports redox
- wasm32 doesn't have traditional memory mapping concepts
- Aligns with the broader unix-or-windows cfg strategy
221-221: Offset type changed to i64 for cross-platform consistency.Changing from
libc::off_ttoi64provides a consistent type across Unix and Windows. Thevalidate_new_argsmethod ensures the offset is non-negative before it's cast tou64forMmapOptions, making this safe.Also applies to: 268-268, 284-284
1193-1212: LGTM - Windows size() correctly handles anonymous and file-backed mappings.The implementation properly:
- Returns the mmap length for anonymous mappings (no file handle)
- Uses GetFileSize for file-backed mappings
- Handles GetFileSize's unusual error signaling (returns u32::MAX on error AND when file size has all low bits set)
- Validates errors via
raw_os_error() != Some(0)
429-435: Check fcntl error handling for F_FULLFSYNC call.The code ignores the return value of
fcntl(fd, F_FULLFSYNC). While F_FULLFSYNC may fail silently on some systems, consider whether errors should be logged or handled, especially since this is a data durability operation. Verify that discarding the result aligns with the intended behavior for this mmap flush operation on macOS.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/stdlib/src/mmap.rs (1)
202-209: LGTM!The new
as_slice()method provides a unified interface for read-only access to mmap data across bothReadandWritevariants. This is a good refactoring that eliminates code duplication at multiple call sites (lines 732, 841, 858, 978, 994, 1011, 1020, 1327, 1340, 1345, 1353, 1357).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/stdlib/src/mmap.rs(23 hunks)
🧰 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 Rust 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/mmap.rs
🧠 Learnings (3)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
Applied to files:
crates/stdlib/src/mmap.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 using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/stdlib/src/mmap.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/mmap.rs
🧬 Code graph analysis (1)
crates/stdlib/src/mmap.rs (1)
crates/common/src/crt_fd.rs (1)
try_borrow_raw(260-266)
⏰ 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 snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (9)
crates/stdlib/src/mmap.rs (9)
48-82: LGTM!The platform-specific advice validation is well-structured, properly gated for Unix systems, and includes appropriate platform-specific constants.
228-252: LGTM!Handle lifecycle management is properly implemented with atomic operations to prevent double-close, and Drop ensures cleanup. The platform-specific implementations correctly handle both Unix file descriptors and Windows HANDLEs.
271-298: LGTM!Windows-specific argument structure correctly differs from Unix (tagname instead of flags/prot), and validation properly checks for negative values before casting.
524-598: LGTM!The Windows handle duplication and file size management logic is well-implemented:
- Properly duplicates the handle so Python can close the original
- Correctly handles file size calculation with GetFileSize (including high/low word combination)
- Uses
checked_addfor overflow protection when calculating required file size- Extends files when map_size exceeds file_len (expected Windows behavior)
- Cleans up duplicated handle on all error paths
603-628: LGTM!The mmap creation logic using
OwnedHandleand theforgetpattern is correct. Per past review comments, the handle lifecycle issue was addressed in commits bf72b6e to 0682b22, ensuring the handle is not closed prematurely on error.
1037-1129: LGTM!The Windows resize implementation correctly handles both anonymous and file-backed mappings:
Anonymous resize (lines 1056-1082):
- Creates new mmap, copies data up to min(old_size, newsize), properly handles both Read/Write variants
File-backed resize (lines 1083-1120):
- Drops mmap before resizing file (prevents access conflicts)
- Uses Windows APIs (SetFilePointerEx/SetEndOfFile) to extend the file
- Calls
try_restore_mmapon failure to maintain consistency (addressed in commit 01cdaa3 per past review)Position adjustment (lines 1123-1126):
- Correctly clamps position when it exceeds new size
1281-1320: LGTM!Both helper methods were properly addressed in previous review iterations:
create_mmap_windows (lines 1281-1309):
- Addressed in commits bf72b6e to 0682b22
- Correctly uses
forget(file)before returning to prevent handle closure- Proper error handling without premature cleanup
try_restore_mmap (lines 1311-1320):
- Addressed in commit 01cdaa3
- Sets
closed=trueif restoration fails, preventing inconsistent state wheremmap_guard=Nonebutclosed=false
35-46: Verify suppress_iph macro availability in rustpython_common crate.The
suppress_iph!macro is imported fromrustpython_commonand used at line 512 to safely calllibc::get_osfhandle. This macro is critical for preventing crashes from invalid file descriptors on Windows. Confirm that the macro is defined in rustpython_common, properly exported, and correctly invoked at the usage site.
506-520: No action required—libc::get_osfhandle is the correct Windows binding.The libc crate correctly exposes
get_osfhandleon Windows (internally mapping to the C runtime's_get_osfhandle). The code uses the proper Rust abstraction and handles errors correctly by checking for-1andINVALID_HANDLE_VALUE.
fix #3847
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.