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-109523: Reading text from a non-blocking stream with read may now raise a BlockingIOError if the operation cannot immediately return bytes. by giosiragusa · Pull Request #122933 · python/cpython

Conversation

@giosiragusa

@giosiragusa giosiragusa commented Aug 12, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Updated _io_TextIOWrapper_read_impl to raise a BlockingIOError when read() does not return bytes.
  • Introduced test_read_non_blocking to ensure that a BlockingIOError is raised when attempting to read from a non-blocking pipe with no data using os.pipe().
  • Issue: sys.stdin.read() throws a TypeError when stdin is set to be non-blocking #109523

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@giosiragusa
Add unittest to handle non-blocking read scenarios.
61cdfc5 
- Introduced `test_read_non_blocking` to ensure proper handling of non-blocking reads using `os.pipe()`.
- The test verifies that a `BlockingIOError` is raised when attempting to read from a non-blocking pipe with no data.
- Updated code to raise `BlockingIOError` with a specific message when `read()` does not return bytes.
@bedevere-app bedevere-app Bot mentioned this pull request Aug 12, 2024
sys.stdin.read() throws a TypeError when stdin is set to be non-blocking #109523
Closed
@bedevere-app

bedevere-app Bot commented Aug 12, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@blurb-it
📜🤖 Added by blurb_it.
5123ff6
@giosiragusa
giosiragusa marked this pull request as ready for review August 14, 2024 00:12
@bedevere-app bedevere-app Bot added the awaiting review label Aug 14, 2024
cmaloney
cmaloney requested changes Aug 14, 2024
View reviewed changes

@cmaloney cmaloney left a comment

Copy link
Copy Markdown
Contributor

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 code in _pyio's implementation of TextIO should ideally be changed in this PR as well so the two main implementations stay in sync (I think, in specific the code around https://github.com/python/cpython/blob/main/Lib/_pyio.py#L2288-L2293, when updating the test to use self.io.open the test I think will fail on PyIO which should help validate where it should go to match behavior)

Definitely liking the direction, looking forward to the next iteration

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
giosiragusa reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Modules/_io/textio.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_io.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Aug 14, 2024
giosiragusa added 5 commits August 15, 2024 10:56
@giosiragusa
Update documentation for Text I/O and Buffered Streams to include han…
7a9037f 
…dling of BlockingIOError in non-blocking I/O scenarios
@giosiragusa
Fix double backticks for inline literals.
c31b0c7
@giosiragusa
Add a more meaningful message for the BlockingIOError.
ade7db0
@giosiragusa
Updated the TextIOWrapper test to use self.io.open() instead of open(…
0953ad0 
…) to ensure the test runs under both _io and _pyio implementations. Also, explicitly specified 'rt' mode in the open() call for clarity, as the test is focused on TextIO behavior.
@giosiragusa
- Raise BlockingIOError in TextIOWrapper.read() in _pyio.py whe…
100c4cb 
…n `buffer.read()` returns zero bytes.

- Fix double backticks in the documentation.
cmaloney
cmaloney approved these changes Aug 16, 2024
View reviewed changes

@cmaloney cmaloney left a comment

Copy link
Copy Markdown
Contributor

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.

Overall I think all the code is correct. I have some personal stylistic nitpicks (How I program / what I'd match doesn't match what you did, but I don't have any strong argument to leave or change, take them or leave them as you wish). Think this needs a more experienced Python reviewer than me at this point.

Thanks a lot for the improvements!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
giosiragusa reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Doc/library/io.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/io.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/_pyio.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_io/textio.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/C_API/2024-08-12-10-15-19.gh-issue-109523.S2c3fi.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
giosiragusa added 6 commits August 17, 2024 19:05
@giosiragusa
Update NEWS entry to include references to _io and _pyio for Blocking…
bf5cc2b 
…IOError change

Revised the NEWS entry in `MISC/NEWS.d/next/C_API` to specify that the recent 
change affecting the `BlockingIOError` now applies to both `_io.TextIOWrapper.read()` 
and `_pyio.TextIOWrapper.read()`.
@giosiragusa
Align documentation and code with Python style practices
fc02b20 
- Adapted documentation to reflect common Python style practices.
- Refactored code to use a clearer variable name.
@giosiragusa
Fix news message for sphinx-lint
c2d14e3
@giosiragusa
Fix news message for sphinx-lint
a735fff
@giosiragusa
Fix news message for sphinx-lint
5f1cbae
@giosiragusa
Make notes more consistent
cabdf39
cmaloney
cmaloney approved these changes Aug 17, 2024
View reviewed changes
@giosiragusa giosiragusa changed the title gh-109523: In _io_TextIOWrapper_read_impl, raise BlockingIOError when read() from a non-blocking pipe does not return bytes. gh-109523: Reading text from a non-blocking stream with read may now raise a :exc:BlockingIOError if the operation cannot immediately return bytes. Aug 22, 2024
@giosiragusa giosiragusa changed the title gh-109523: Reading text from a non-blocking stream with read may now raise a :exc:BlockingIOError if the operation cannot immediately return bytes. gh-109523: Reading text from a non-blocking stream with read may now raise a BlockingIOError if the operation cannot immediately return bytes. Aug 22, 2024
@cmaloney cmaloney mentioned this pull request Sep 14, 2024
The io module doesn't support non-blocking files #57531
Open
z764969689
z764969689 reviewed Sep 14, 2024
View reviewed changes
Comment thread Lib/_pyio.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@giosiragusa
Align BlockingIOError message in _pyio.py with the C implementation i…
8286a0a 
…n textio.c
@giosiragusa

giosiragusa commented Nov 27, 2024

Copy link
Copy Markdown
Contributor Author

Just dropping a friendly ping here. This pull request is in good shape and has undergone intensive review. Could a maintainer kindly take a look for final review and, if everything looks good, consider merging it?
All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz commented Nov 27, 2024

Copy link
Copy Markdown
Member

cc @serhiy-storchaka
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz reviewed Nov 27, 2024
View reviewed changes
Comment thread Doc/library/io.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/_pyio.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/_pyio.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/_pyio.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@giosiragusa
Add blank line after note.
8690397
gpshead
gpshead reviewed Nov 27, 2024
View reviewed changes
Comment thread Modules/_io/textio.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_io/textio.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/io.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
giosiragusa added 5 commits November 27, 2024 11:38
@giosiragusa
Remove unnecessary blank lines.
2f7fbb2
@giosiragusa
Keep the error message simple.
dbd807d
@giosiragusa
Release the reference before throwing the exception.
1ac9096
@giosiragusa
Add blank line after notes.
7d05cc3
@giosiragusa
Remove unnecessary blank lines.
9f1c5f4
@giosiragusa

giosiragusa commented Nov 27, 2024

Copy link
Copy Markdown
Contributor Author

Also a trivial question from my side. Would this change qualify for an entry in Misc/ACKS? I'd be happy to add it if appropriate.
All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz commented Nov 27, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Simce it's a user facing change I'd say it warrants both a NEWS and a What's New entry to be on the safe side.

You can add yourself to ACKS I think (sorry I only read the Misc part and not the file...). Technically, every contributor is free to add themselves there (or am I wrong? I know I've never added myself due to laziness).
All reactions

Sorry, something went wrong.

Uh oh!

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

giosiragusa commented Nov 28, 2024

Copy link
Copy Markdown
Contributor Author

Simce it's a user facing change I'd say it warrants both a NEWS and a What's New entry to be on the safe side.

You can add yourself to ACKS I think (sorry I only read the Misc part and not the file...). Technically, every contributor is free to add themselves there (or am I wrong? I know I've never added myself due to laziness).

Thanks for the feedback.
I was referring to the "Crediting" section in here: https://devguide.python.org/getting-started/pull-request-lifecycle/
Since this is my first pull request I was not sure how to proceed.

I have the news entry, but I'm not sure how to handle the "What's New entry". How am I supposed to write one?
All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz commented Nov 28, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Look at the 3.14.rst file for example. The What's New entry should looks like

io
--

* Blabla
  (Contributed by YOU in :gh:`ISSUE_NUMBER`.)

The blabla text is usually the same as the one in the blurb.
You can look at how previous features were documented. I'll review any style issue afterwards.
All reactions

Sorry, something went wrong.

Uh oh!

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

giosiragusa commented Nov 29, 2024

Copy link
Copy Markdown
Contributor Author

Look at the 3.14.rst file for example. The What's New entry should looks like

io
--

* Blabla
  (Contributed by YOU in :gh:`ISSUE_NUMBER`.)

The blabla text is usually the same as the one in the blurb. You can look at how previous features were documented. I'll review any style issue afterwards.

Hi Benedikt,
I'm still unsure which file name should I use?

Doc/wathsnew/3.X.rst
or
Doc/whatsnew/next.rts?
All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz commented Nov 29, 2024

Copy link
Copy Markdown
Member

The 3.14.rst file. The next.rst file is for the full changelog and is managed automatically.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@giosiragusa
Added what's new entry.
317d402 
Added entry in ACKS.
picnixz
picnixz reviewed Nov 29, 2024
View reviewed changes
Comment thread Doc/whatsnew/3.14.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@giosiragusa @picnixz
Update Doc/whatsnew/3.14.rst
8e4e661 
Fix missing indents and other syntax issues.

Co-authored-by: Bénédikt Tran <[email protected]>
@encukou
encukou merged commit 31f16e4 into python:main Dec 2, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label Dec 2, 2024
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
@giosiragusa @srinivasreddy
pythongh-109523: Raise a BlockingIOError if reading text from a non-b…
a50e6d8 
…locking stream cannot immediately return bytes. (pythonGH-122933)
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@giosiragusa @ebonnal
pythongh-109523: Raise a BlockingIOError if reading text from a non-b…
3828a71 
…locking stream cannot immediately return bytes. (pythonGH-122933)
@coderabbitai coderabbitai Bot mentioned this pull request May 6, 2026
build: enable strict ruff linting and CI enforcement trustyai-explainability/trustyai-service#122
Merged
11 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead left review comments
@picnixz picnixz picnixz left review comments
@cmaloney cmaloney cmaloney approved these changes
+1 more reviewer
@z764969689 z764969689 z764969689 left review comments
Reviewers whose approvals may not affect merge requirements

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.

6 participants

@giosiragusa @picnixz @gpshead @cmaloney @z764969689 @encukou
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.