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

gh-140650: fix SystemError in io.BufferedWriter.close when closed errors by inventshah · Pull Request #140653 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-140650: fix SystemError in io.BufferedWriter.close when closed errors - #140653

Merged
serhiy-storchaka merged 7 commits into
python:mainfrom
inventshah:fix-140650
Nov 5, 2025
ConversationCommits (7)ChecksFiles changed
Merged

gh-140650: fix SystemError in io.BufferedWriter.close when closed errors#140653
serhiy-storchaka merged 7 commits into
python:mainfrom
inventshah:fix-140650

Conversation

inventshah commented Oct 27, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Restores the 3.14 behavior of emitting a TypeError. Preservation of the sign helps in the error checking here:

cpython/Modules/_io/bufferedio.c

Lines 556 to 563 in 3dab11f

/* gh-138720: Use IS_CLOSED to match flush CHECK_CLOSED. */
r = IS_CLOSED(self);
if (r < 0)
goto end;
if (r > 0) {
res = Py_NewRef(Py_None);
goto end;
}

db68bfc changed from buffered_closed to IS_CLOSED.

bedevere-app Bot mentioned this pull request Oct 27, 2025
Closed
cmaloney self-requested a review October 27, 2025 05:36
ashm-dev suggested changes Oct 27, 2025
View reviewed changes
Comment thread Modules/_io/bufferedio.c Outdated
serhiy-storchaka reviewed Oct 27, 2025
View reviewed changes

serhiy-storchaka 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

This does not look right. If IS_CLOSED() returns a positive value and sets an error, there is a bug in IS_CLOSED().

cmaloney commented Oct 27, 2025

Copy link
Copy Markdown
Contributor

@serhiy-storchaka : Overall problem is code assumes IS_CLOSED can't result in an exception being set but if a user-provided implementation does unusual things (ex. closed = NotImplemented) then that breaks.

Another option here is "Exception" can be turned into a True result for "IS_CLOSED" + clear the exception but that seems like hiding an implemention issue rather than helping resolve it to me.

serhiy-storchaka commented Oct 27, 2025

Copy link
Copy Markdown
Member

IS_CLOSED() should return -1 on error.

cmaloney reviewed Oct 27, 2025
View reviewed changes
Comment thread Modules/_io/bufferedio.c Outdated
cmaloney reviewed Oct 27, 2025
View reviewed changes

cmaloney left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Also update CHECK_CLOSED to handle IS_CLOSED returning non-zero + setting an exception. In particular, if there's an exception CHECK_CLOSED should always return.

cmaloney changed the title gh-140650: fix SystemError in io.BufferedWriter.close when closed is not implemented gh-140650: fix SystemError in io.BufferedWriter.close when closed errors Oct 27, 2025

inventshah commented Oct 27, 2025

Copy link
Copy Markdown
Contributor Author

Should I still be using PyErr_Occurred() or based on @serhiy-storchaka's feedback, should we make IS_CLOSED() return -1 on exception and check the sign in both CHECK_CLOSED() and _io__Buffered_close_impl()?

serhiy-storchaka commented Oct 27, 2025

Copy link
Copy Markdown
Member

PyErr_Occurred() is only used when we cannot distinguish the value signalling an error from a valid value, like in PyLong_AsLong(). IS_CLOSED() returns -1 on error, 0 -- false, 1 -- true. Or so it should be.

cmaloney reviewed Oct 27, 2025
View reviewed changes
Comment thread Modules/_io/bufferedio.c Outdated
efimov-mikhail approved these changes Oct 29, 2025
View reviewed changes

efimov-mikhail 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

LGTM

serhiy-storchaka reviewed Oct 29, 2025
View reviewed changes
Comment thread Modules/_io/bufferedio.c Outdated
Comment thread Misc/NEWS.d/next/Library/2025-10-27-00-40-49.gh-issue-140650.DYJPJ9.rst Outdated
Comment thread Lib/test/test_io/test_bufferedio.py Outdated
serhiy-storchaka approved these changes Nov 5, 2025
View reviewed changes

serhiy-storchaka 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

LGTM. 👍

serhiy-storchaka merged commit 1d25b75 into python:main Nov 5, 2025
50 checks passed
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
pythongh-140650: Fix write(), flush() and close() methods of io.Buffe…
692957b 
…redWriter (pythonGH-140653)

They could raise SystemError or crash when getting the "closed" attribute
or converting it to boolean raises an exception.
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

efimov-mikhail efimov-mikhail approved these changes
cmaloney cmaloney approved these changes
serhiy-storchaka serhiy-storchaka approved these changes
+1 more reviewer
ashm-dev ashm-dev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL