X Tutup
Skip to content

io.TextIoWrapper.reconfigure to always flush #7281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 2 commits into from
Mar 1, 2026
Merged

io.TextIoWrapper.reconfigure to always flush #7281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
954630a
Always flush on reconfigure (io.TextWrapper)
ShaharNaveh Feb 28, 2026
53fca08
Add test
ShaharNaveh Feb 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions crates/vm/src/stdlib/io.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3029,7 +3029,6 @@ mod _io {
let mut newline_changed = false;
let mut line_buffering = None;
let mut write_through = None;
let mut flush_on_reconfigure = false;

if let Some(enc) = args.encoding {
if enc.as_str().contains('\0') && enc.as_str().starts_with("locale") {
Expand All @@ -3056,11 +3055,9 @@ mod _io {
}

if let OptionalArg::Present(Some(value)) = args.line_buffering {
flush_on_reconfigure = true;
line_buffering = Some(Self::bool_from_index(value, vm)?);
}
if let OptionalArg::Present(Some(value)) = args.write_through {
flush_on_reconfigure = true;
write_through = Some(Self::bool_from_index(value, vm)?);
}

Expand All @@ -3077,12 +3074,10 @@ mod _io {
));
}

if flush_on_reconfigure {
if data.pending.num_bytes > 0 {
data.write_pending(vm)?;
}
vm.call_method(&data.buffer, "flush", ())?;
if data.pending.num_bytes > 0 {
data.write_pending(vm)?;
}
vm.call_method(&data.buffer, "flush", ())?;

if encoding_changed || errors_changed || newline_changed {
if data.pending.num_bytes > 0 {
Expand Down
28 changes: 27 additions & 1 deletion extra_tests/snippets/stdlib_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import os
from io import BufferedReader, BytesIO, FileIO, RawIOBase, StringIO
from io import BufferedReader, BytesIO, FileIO, RawIOBase, StringIO, TextIOWrapper
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove unused BytesIO and StringIO imports.

Line 2 introduces unused imports that are already flagged by static analysis (F401), which adds noise to linting.

Proposed fix 
-from io import BufferedReader, BytesIO, FileIO, RawIOBase, StringIO, TextIOWrapper
+from io import BufferedReader, FileIO, RawIOBase, TextIOWrapper
🧰 Tools
🪛 Flake8 (7.3.0)

[error] 2-2: 'io.BytesIO' imported but unused

(F401)

[error] 2-2: 'io.StringIO' imported but unused

(F401)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@extra_tests/snippets/stdlib_io.py` at line 2, Remove the unused imports
BytesIO and StringIO from the import statement in stdlib_io.py; edit the line
that currently imports BufferedReader, BytesIO, FileIO, RawIOBase, StringIO,
TextIOWrapper and delete BytesIO and StringIO so only actually used symbols
(e.g., BufferedReader, FileIO, RawIOBase, TextIOWrapper) remain, resolving the
F401 lint warnings.


from testutils import assert_raises

Expand Down Expand Up @@ -58,3 +58,29 @@ def readinto(self, b):

with assert_raises(ValueError):
f.isatty()


class Gh6588:
def __init__(self):
self.textio = None
self.closed = False

def writable(self):
return True

def readable(self):
return False

def seekable(self):
return False

def write(self, data):
self.textio.reconfigure(encoding="utf-8")
return len(data)


raw = Gh6588()
textio = TextIOWrapper(raw, encoding="utf-8", write_through=True)
raw.textio = textio
with assert_raises(AttributeError):
textio.writelines(["x"])
Comment on lines +63 to +86
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

This regression test currently validates missing flush, not reentrancy handling.

Gh6588 has no flush, so Line 86 can fail with AttributeError before validating the reentrant-lock path from issue #6588. This can let the panic/reentrancy bug regress undetected.

Proposed test adjustment to exercise reentrant `reconfigure` path 
 class Gh6588:
@@
     def write(self, data):
         self.textio.reconfigure(encoding="utf-8")
         return len(data)
+
+    def flush(self):
+        self.textio.reconfigure(encoding="utf-8")
@@
-with assert_raises(AttributeError):
+with assert_raises(RuntimeError):
     textio.writelines(["x"])
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@extra_tests/snippets/stdlib_io.py` around lines 63 - 86, The test fails early
with AttributeError because Gh6588 lacks a flush method, so
TextIOWrapper.writelines() raises instead of exercising the reentrant
reconfigure-path; update the Gh6588 class to implement a flush(self) method
(e.g., forward to self.textio.flush() or a no-op) so TextIOWrapper (created as
textio via TextIOWrapper(raw, ...)) can call flush and the test will exercise
the reentrant call in write/reconfigure rather than short-circuiting on missing
flush.

Copy link
Copy Markdown
Member

@youknowone youknowone Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShaharNaveh is this comment correct?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Copy Markdown
Contributor Author

@ShaharNaveh ShaharNaveh Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShaharNaveh is this comment correct?

No, the entire point of the test is to align the behavior to be like cpython and to get an AttributeError

Loading
X Tutup