Fix install ujson #6502
Fix install ujson #6502
Conversation
📝 WalkthroughWalkthroughThe changes enhance SSL read handling in non-blocking socket scenarios and timeout behavior, introducing retry logic for feeding TLS data to rustls, improved EOF/connection-closed detection, and a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/stdlib/src/ssl/compat.rs (1)
424-466: Core fix looks correct, but consider documenting the buffer size choice.This retry logic properly handles the case where rustls reports it doesn't need more data but additional TLS records may still arrive from the network. The distinction between non-blocking (immediate return) and blocking/timeout sockets (attempt read) is correct.
Minor observations:
- Line 439: The hardcoded
2048buffer size is reasonable for TLS records but lacks explanation. Consider adding a comment explaining why this size was chosen (e.g., typical TLS record overhead + payload).- Line 449-453: The
clone()andunwrap_or(0)are safe but slightly inefficient. The clone is necessary fortry_into_value, and the unwrap_or provides a safe fallback if conversion fails.💡 Optional: Add explanatory comment for buffer size
- let data = match socket.sock_recv(2048, vm) { + // Read up to 2048 bytes (sufficient for typical TLS record: 5-byte header + up to ~16KB payload fragment) + let data = match socket.sock_recv(2048, vm) {
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/stdlib/src/ssl/compat.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/ssl/compat.rs
🧬 Code graph analysis (1)
crates/stdlib/src/ssl/compat.rs (2)
crates/stdlib/src/ssl.rs (2)
timeout(4165-4168)e(1115-1115)crates/stdlib/src/socket.rs (4)
timeout(707-709)std(1538-1538)std(1598-1598)new(2446-2450)
⏰ 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). (1)
- GitHub Check: Run snippets and cpython tests (windows-latest)
🔇 Additional comments (4)
crates/stdlib/src/ssl/compat.rs (4)
399-399: LGTM: Timeout variant appropriately distinguishes timeout errors.The new
Timeout(String)variant properly separates socket timeout scenarios from generic IO errors and WantRead conditions, improving error clarity and matching Python's timeout exception semantics.
479-481: LGTM: Simplified fallback defers to main buffer handling.The simplified
continueis appropriate here since the primary buffer-full logic is now handled inssl_read_tls_records(lines 569-585). This serves as a safety net fallback.
560-599: Excellent buffer-full handling with proper safeguards.The enhanced retry logic correctly handles rustls buffer exhaustion:
- When
read_tlsreturns 0, callsprocess_new_packetsto consume buffered data and make room- Retries once to feed remaining data
- Critical: Line 576-577 breaks if still can't consume, preventing infinite loops
The dual handling of Ok(0) and Err("buffer full") paths ensures robustness across different rustls error modes.
656-659: LGTM: Correct semantic distinction between timeout and would-block.Changing from
WantReadtoTimeoutwhen the socket times out duringselectis semantically correct.WantReadindicates a non-blocking socket would block, whileTimeoutindicates an operation exceeded its time limit. This properly aligns with Python'ssocket.timeoutexception behavior.
close #6500
fix #6491
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.