GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Upgrade test_io from 3.13.11 and fix more io tests by youknowone · Pull Request #6565 · RustPython/RustPython Conversation
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughPer-field #[cfg(...)] handling was added to generated struct-sequence initializers, and the I/O subsystem gained atomic closing coordination, non-blocking semantics (Option-wrapped reads/writes), and API updates (renamed Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TextIO as TextIOWrapper
participant Buf as BufferedReader/Writer
participant Raw as FileIO
participant Kernel as OS/Kernel
rect rgb(235,245,255)
Note over Client,Kernel: Non-blocking read path (high-level)
end
Client->>TextIO: read(size, non_blocking=True)
TextIO->>Buf: read(size, non_blocking=True)
alt data in buffer
Buf-->>TextIO: Option<Some(bytes)>
TextIO-->>Client: Some(bytes)
else need raw I/O
Buf->>Raw: read(size, non_blocking=True)
Raw->>Kernel: sys_read(non-blocking)
alt kernel returns bytes
Kernel->>Raw: bytes
Raw-->>Buf: Option<Some(bytes)>
Buf-->>TextIO: Some(bytes)
TextIO-->>Client: Some(bytes)
else EAGAIN
Kernel->>Raw: EAGAIN
Raw-->>Buf: Option<None>
Buf-->>TextIO: None
TextIO-->>Client: None
end
end
sequenceDiagram
participant Writer
participant BufW as BufferedWriter
participant Buffer as LocalBuffer
participant Raw as FileIO
participant Closer as AtomicCloser
rect rgb(255,245,235)
Note over Writer,Closer: Write path with closing coordination
end
Writer->>BufW: write(data)
BufW->>Closer: check closing()
alt closing true
BufW->>Closer: spin-wait / short yield
Closer-->>BufW: still closing -> error
BufW-->>Writer: raise value/error (write to closed)
else closing false
BufW->>Buffer: append data
BufW->>Raw: flush()
alt raw write succeeds
Raw-->>BufW: Ok(n)
BufW-->>Writer: Some(n)
else EAGAIN / BlockingIOError
Raw-->>BufW: BlockingIOError (EAGAIN)
BufW->>Buffer: shift buffer, update positions
BufW-->>Writer: raise BlockingIOError (EAGAIN)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
9fd139d to
e54c2db
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Lib/test/test_io.py is excluded by !Lib/**crates/derive-impl/src/pystructseq.rscrates/vm/src/stdlib/io.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 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/derive-impl/src/pystructseq.rscrates/vm/src/stdlib/io.rscrates/vm/src/stdlib/ctypes/function.rs (2)
f(987-987)None(1687-1687)crates/stdlib/src/ssl/compat.rs (2)
v(1435-1435)v(1717-1717)crates/vm/src/vm/context.rs (1)
none(365-367)crates/vm/src/types/structseq.rs (2)
try_from_elements(172-174)Self(284-287)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| fn write(&self, obj: ArgBytesLike, vm: &VirtualMachine) -> PyResult<usize> { | ||
| // Check if close() is in progress (Issue #31976) | ||
| // If closing, wait for close() to complete by spinning until raw is closed | ||
| if self.writer().closing().load(Ordering::Acquire) { | ||
| // Wait for close() to complete - loop until raw.closed is True | ||
| loop { | ||
| let raw = { | ||
| let data = self.writer().lock(vm)?; | ||
| match &data.raw { | ||
| Some(raw) => raw.to_owned(), | ||
| None => break, // detached | ||
| } | ||
| }; | ||
| if file_closed(&raw, vm)? { | ||
| break; | ||
| } | ||
| // Yield to other threads | ||
| std::thread::yield_now(); | ||
| } | ||
| return Err(vm.new_value_error("write to closed file".to_owned())); | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Potential unbounded spin-wait in write when closing.
The spin-wait loop has no timeout or iteration limit. If close() stalls or raw.closed never becomes True, write() could hang indefinitely. While yield_now() prevents busy-waiting, it doesn't guarantee forward progress.
Consider adding a maximum iteration count or timeout to prevent potential deadlocks, or document the assumption that close() will always complete in finite time.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.