FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-138720: Make Buffered closed check match flush by cmaloney · Pull Request #138724 · python/cpython · GitHub

/ cpython Public

gh-138720: Make Buffered closed check match flush - #138724

Merged
encukou merged 5 commits into
python:mainfrom
cmaloney:bufferedrwpair_gc
Sep 18, 2025
ConversationCommits (5)ChecksFiles changed
Merged

gh-138720: Make Buffered closed check match flush#138724
encukou merged 5 commits into
python:mainfrom
cmaloney:bufferedrwpair_gc

Conversation

cmaloney commented Sep 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

In _io__Buffered_flush_impl the macro CHECK_CLOSED is used to check the buffered* is in a good state to be flushed. That differs slightly from buffered_closed.

In some cases, that difference would result in close() (_io__Buffered_close_impl) thinking the file needed to be flushed and closed while flush() thought the file was already closed.

This could happen during GC and would result in an unraisable exception.

pythongh-138720: Make Buffered closed check match flush
e93877d 
In `_io__Buffered_flush_impl` the macro `CHECK_CLOSED` is used to check
the `buffered*` is in a good state to be flushed. That differs slightly
from `buffered_closed`.

In some cases, that difference would result in `close()` thinking the
file needed to be flushed and closed while `flush()` thought the file
was already closed.

This could happen during GC and would result in an unraisable exception.
bedevere-app Bot mentioned this pull request Sep 10, 2025
Closed
This was referenced Sep 10, 2025
Merged
Merged

cmaloney commented Sep 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

For this, I also looked at resolving two other ways:

  1. Updating .closed member: This makes it so .close() isn't called (and therefore .flush()). A lot more tests break with that change, this to me is more minimal fix; BufferedRWPair is not often used (and was thought of being removed once).
  2. Figure out why the GC isn't clearing out in a clean order and resolve: Again fairly intricate to do with a lot more potential unintended consequences.

encukou commented Sep 17, 2025

Copy link
Copy Markdown
Member

!buildbot PPC64LE.Fedora.Stable.LTO

bedevere-bot commented Sep 17, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @encukou for commit 60fe309 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F138724%2Fmerge

The command will test the builders whose names match following regular expression: PPC64LE.Fedora.Stable.LTO

The builders matched are:

  • PPC64LE Fedora Stable LTO PR
  • PPC64LE Fedora Stable LTO + PGO PR

encukou approved these changes Sep 17, 2025
View reviewed changes

encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you! This looks like a correct change.

Comment thread Lib/test/test_io/test_general.py
Comment on lines +2290 to +2293
# gh-138720: C BufferedRWPair would destruct in a bad order resulting in
# an unraisable exception.
support.gc_collect()

encukou Sep 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this still necessary?

vstinner Sep 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The test is way more likely to triggered the issue (fixed behavior) with this change.

cmaloney Sep 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

On my dev box (64 bit archlinux) adding the explicit GC takes this from "one in a hundred" -> every time

encukou Sep 18, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK! I couldn't reproduce it myself, but I'll trust the buildbots :)

encukou merged commit db68bfc into python:main Sep 18, 2025
51 checks passed

vstinner commented Sep 18, 2025

Copy link
Copy Markdown
Member

Should we backport this bugfix?

encukou commented Sep 18, 2025

Copy link
Copy Markdown
Member

Can you reproduce it on previous versions? If so, please backport :)

vstinner commented Sep 18, 2025

Copy link
Copy Markdown
Member

Can you reproduce it on previous versions? If so, please backport :)

Oh. I'm unable to reproduce the issue on 3.14.

cmaloney deleted the bufferedrwpair_gc branch September 18, 2025 19:26

cmaloney commented Sep 18, 2025

Copy link
Copy Markdown
Contributor Author

The code has been the same since the C I/O implementation was added; unless/until someone has an issue on older versions I wouldn't backport.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner left review comments
encukou encukou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL