Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdded EINTR-aware retry loops (PEP 475) to VM stdlib write paths and refactored memory view handling in Changes
Sequence Diagram(s)sequenceDiagram
participant VM as VM (raw_write)
participant Mem as MemoryView / Buffer
participant OS as OS (write syscall)
participant Sig as SignalHandler
VM->>Mem: into_ref / obtain view
VM->>OS: write(buf)
OS-->>VM: EINTR
VM->>Sig: trap_eintr() / check signals
alt signal handled / retry allowed
VM->>OS: write(buf) -- retry loop until success or other error
OS-->>VM: n_written / EAGAIN / error
end
VM->>Mem: restore internal borrow buffer (if used)
VM-->>Caller: return bytes written or propagate error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
crates/vm/src/stdlib/io.rs (1)
1004-1035: Deduplicate the EINTR retry loop inraw_write.Both branches run the same
write+trap_eintrloop; extracting it into a small helper (e.g.,write_with_eintr) would reduce drift and simplify maintenance. As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/io.rs` around lines 1004 - 1035, The EINTR retry loop in raw_write is duplicated in both branches; extract the common loop into a small helper (e.g., write_with_eintr) that takes the callable target and the prepared mem_obj and performs the vm.call_method + trap_eintr retry logic, then replace both branches with code that prepares their respective mem_obj (PyMemoryView from buffer_range or from VecBuffer::from(...).into_pybuffer(...)) and calls write_with_eintr(self.raw.as_ref().unwrap(), mem_obj, vm). Ensure the helper returns the same Result/Option shape as the current loop so callers in raw_write can handle Ok(Some)/Ok(None)/Err uniformly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/vm/src/stdlib/io.rs`:
- Around line 1004-1035: The EINTR retry loop in raw_write is duplicated in both
branches; extract the common loop into a small helper (e.g., write_with_eintr)
that takes the callable target and the prepared mem_obj and performs the
vm.call_method + trap_eintr retry logic, then replace both branches with code
that prepares their respective mem_obj (PyMemoryView from buffer_range or from
VecBuffer::from(...).into_pybuffer(...)) and calls
write_with_eintr(self.raw.as_ref().unwrap(), mem_obj, vm). Ensure the helper
returns the same Result/Option shape as the current loop so callers in raw_write
can handle Ok(Some)/Ok(None)/Err uniformly.
Summary by CodeRabbit