GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-138720: Make Buffered closed check match flush by cmaloney · Pull Request #138724 · python/cpython

Conversation

@cmaloney

@cmaloney cmaloney commented Sep 10, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

  • Issue: GC of unclosed io.BuffererdRWPair after .readinto results in unraisable exception #138720

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@cmaloney
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 bedevere-app Bot added the awaiting review label Sep 10, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Sep 10, 2025
GC of unclosed io.BuffererdRWPair after .readinto results in unraisable exception #138720
Closed
cmaloney added 4 commits September 9, 2025 17:59
@cmaloney
add blurb
ef75829
@cmaloney
Fix docs
c368fd6
@cmaloney
try, try, try again
a9200d3
@cmaloney
news: Doesn't actually touch BufferedIOBase, be more accurate for cla…
60fe309 
…sses
This was referenced Sep 10, 2025
gh-138013: Move I/O test infrastructre to test_io.utils #138475
Merged
gh-138013: Remove load_tests in test_io.test_general #138771
Merged
@cmaloney

cmaloney commented Sep 16, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou

encukou commented Sep 17, 2025

Copy link
Copy Markdown
Member

!buildbot PPC64LE.Fedora.Stable.LTO
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
encukou
encukou approved these changes Sep 17, 2025
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thank you! This looks like a correct change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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 encukou Sep 17, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is this still necessary?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Sep 17, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cmaloney cmaloney Sep 17, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@encukou encukou Sep 18, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Sep 17, 2025
@encukou
encukou merged commit db68bfc into python:main Sep 18, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 18, 2025
@vstinner

vstinner commented Sep 18, 2025

Copy link
Copy Markdown
Member

Should we backport this bugfix?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou

encukou commented Sep 18, 2025

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cmaloney
cmaloney deleted the bufferedrwpair_gc branch September 18, 2025 19:26
@cmaloney

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.
vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@encukou encukou encukou approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@cmaloney @encukou @bedevere-bot @vstinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.