Fix windows socket #6408
Fix windows socket #6408
Conversation
WalkthroughTwo public method signatures updated for improved type handling: Socket methods Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![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
🧹 Nitpick comments (1)
crates/stdlib/src/socket.rs (1)
1462-1468: Verify Windows SOCKET handle range assumptions.The code assumes that socket handles fit within
i64::MAXon Windows. While this is true in practice (Windows SOCKET handles are kernel handles that won't exceed this range), the cast fromusize(which SOCKET is on 64-bit Windows) toi64could theoretically overflow for values abovei64::MAX.Consider adding a debug assertion or comment documenting this assumption.
#[pymethod] fn close(&self) -> io::Result<()> { let sock = self.detach(); + // Windows SOCKET handles are kernel handles that always fit in i64 if sock != INVALID_SOCKET as i64 { close_inner(sock as RawSocket)?; } Ok(()) }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/test/test_selectors.pyis excluded by!Lib/**Lib/test/test_socketserver.pyis excluded by!Lib/**Lib/test/test_zipimport.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/stdlib/src/socket.rs(2 hunks)crates/vm/src/stdlib/nt.rs(3 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/vm/src/stdlib/nt.rscrates/stdlib/src/socket.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/nt.rs (1)
crates/wtf8/src/lib.rs (1)
from_wide(336-352)
⏰ 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 rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- 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 (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (3)
crates/stdlib/src/socket.rs (2)
1068-1068: LGTM!The
repr_strformatting correctly useszelf.fileno()directly now thatfileno()returnsi64instead ofRawSocket.
1478-1483: Thefileno()method correctly returnsi64for consistency withdetach().The implementation properly handles closed sockets by returning
INVALID_SOCKET as i64(-1) and correctly casts valid socket file descriptors toi64. The existing caller in__repr__(line 1068) is compatible with thei64return type.crates/vm/src/stdlib/nt.rs (1)
1125-1131: The function design is correct; no caller updates are needed.This is a
#[pyfunction]exposed to Python, not an internal Rust API. The#[pyfunction]macro automatically converts types implementingToPyObject(includingWtf8Bufand tuples thereof) to Python objects. The return type(Wtf8Buf, Wtf8Buf)correctly produces a Python tuple of two strings without requiring manual conversion by callers—there are no Rust callers to update.The
_vmparameter is unused but follows the established pattern in the codebase (e.g.,re.rs,gc.rs,typing.rs,os.rs,imp.rs) where unused parameters are retained for API consistency.Likely an incorrect or invalid review comment.
| fn detach(&self) -> i64 { | ||
| let sock = self.sock.write().take(); | ||
| sock.map_or(INVALID_SOCKET, into_sock_fileno) | ||
| sock.map_or(INVALID_SOCKET as i64, |s| into_sock_fileno(s) as i64) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type rust '\.detach\s*\(' -B 3 -A 3Repository: RustPython/RustPython
Length of output: 508
🏁 Script executed:
rg -n 'fn detach' crates/stdlib/src/socket.rs -B 2 -A 5Repository: RustPython/RustPython
Length of output: 335
🏁 Script executed:
rg -n 'into_sock_fileno|close_inner' crates/stdlib/src/socket.rs -B 2 -A 5Repository: RustPython/RustPython
Length of output: 1943
🏁 Script executed:
rg -n 'fn fileno' crates/stdlib/src/socket.rs -B 2 -A 5Repository: RustPython/RustPython
Length of output: 315
🏁 Script executed:
rg -n 'INVALID_SOCKET|cfg.*windows' crates/stdlib/src/socket.rs | head -40Repository: RustPython/RustPython
Length of output: 1563
🏁 Script executed:
sed -n '2374,2385p' crates/stdlib/src/socket.rsRepository: RustPython/RustPython
Length of output: 333
🏁 Script executed:
rg -n 'RawSocket' crates/stdlib/src/socket.rs | grep -E 'type|cfg' | head -20Repository: RustPython/RustPython
Length of output: 171
🏁 Script executed:
# Check if there are any comments or TODOs about Windows socket limitations
rg -n 'TODO|FIXME|BUG|XXX|HACK' crates/stdlib/src/socket.rs | grep -i 'socket\|window\|handle\|fd'Repository: RustPython/RustPython
Length of output: 47
Verify the return type change impact across the codebase.
The detach() method now returns i64 instead of RawSocket. This is a breaking API change affecting:
- The
close()method (line 1463), which has been properly updated to handlei64and cast back toRawSocketforclose_inner() - Python code calling
detach(), which receivesi64(Python'sintsafely represents this range)
On Windows, casting RawSocket (which is usize) to i64 can theoretically overflow if a socket handle exceeds i64::MAX. This same issue also affects the fileno() method (line 1482), suggesting it may be an accepted limitation of the current design. Consider documenting whether this is a known limitation or requires a fix.
1 participant
Summary by CodeRabbit
Release Notes
API Changes
fileno()anddetach()now return integer values instead of raw socket objects. Code using these methods may require updates.Refactoring
✏️ Tip: You can customize this high-level summary in your review settings.