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-60107: avoid `io.RawIOBase.read` from reading into a temporary bytearray by taskevich · Pull Request #138616 · python/cpython

Conversation

@taskevich

@taskevich taskevich commented Sep 7, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown

implements: https://github.com/python/cpython/issues/60107

  • Issue: Remove copy from bytearray to bytes in rawiobase_read() implementation #60107

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@taskevich
allocate a bytes object directly instead and manually construct a wri…
974cc01 
…table memoryview pointing to it.
@python-cla-bot

python-cla-bot Bot commented Sep 7, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting review label Sep 7, 2025
@bedevere-app

This comment was marked as resolved.

Sign in to view
@bedevere-app bedevere-app Bot mentioned this pull request Sep 7, 2025
Remove copy from bytearray to bytes in rawiobase_read() implementation #60107
Closed
@blurb-it
📜🤖 Added by blurb_it.
b44bec9
picnixz
picnixz reviewed Sep 7, 2025
View reviewed changes
Comment thread Modules/_io/iobase.c Resolved

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/C_API/2025-09-07-10-52-09.gh-issue-60107.dhcb2Y.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@picnixz picnixz changed the title gh-60107: Read directly to bytes object gh-60107: avoid io.RawIOBase.read from reading into a temporary bytearray Sep 7, 2025
@picnixz

picnixz commented Sep 7, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Can you indicate how efficient this actually becomes by using pyperf? there was not clear consensus on the issue considering what should be done.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
taskevich added 2 commits September 7, 2025 14:17
@taskevich
pythongh-60107: pythongh-138616: Minor code changes
897e813
@taskevich
Merge branch 'direct-read-bytes' of https://github.com/taskevich/cpython
77e3875 
 into direct-read-bytes
@picnixz

picnixz commented Sep 7, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

While I think this addition is fine, I'm worried that it could change performances a bit. So I would appreciate if we had numbers indicating whether this changed or not the performance (you can look at https://github.com/python/pyperformance for inspiration of how to write benchmarks).

By the way, avoid merging main into your branch unless it's meant to fix CI tests. And remove the NEWS entry for now. We'll add it later if needed. If possible, it would be good if we also had tests where we have a class inheriting from RawIOBase and which tries to mess with the buffer passed to readinto
taskevich 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.
@picnixz picnixz added the skip news label Sep 7, 2025
@taskevich
pythongh-60107: Change blurb_it
2351f34
picnixz
picnixz reviewed Sep 7, 2025
View reviewed changes
Comment thread Misc/NEWS.d/next/C_API/2025-09-07-10-52-09.gh-issue-60107.dhcb2Y.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
taskevich and others added 2 commits September 7, 2025 14:28
@taskevich @picnixz
Update Misc/NEWS.d/next/C_API/2025-09-07-10-52-09.gh-issue-60107.dhcb…
c671122 
…2Y.rst

Co-authored-by: Bénédikt Tran <[email protected]>
@blurb-it
📜🤖 Added by blurb_it.
5396b1c
@picnixz picnixz removed the skip news label Sep 7, 2025
picnixz
picnixz reviewed Sep 7, 2025
View reviewed changes
Comment thread Misc/NEWS.d/next/C_API/2025-09-07-10-52-09.gh-issue-60107.dhcb2Y.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@taskevich
Deleted incorrect news from C_API
23cd0ff
auvipy
auvipy reviewed Sep 7, 2025
View reviewed changes

@auvipy auvipy left a comment

Copy link
Copy Markdown

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.

should we also consider adding some extra unit tests for this change?

Sorry, something went wrong.

Uh oh!

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

picnixz commented Sep 7, 2025

Copy link
Copy Markdown
Member

This was already mentioned:

If possible, it would be good if we also had tests where we have a class inheriting from RawIOBase and which tries to mess with the buffer passed to readinto
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@taskevich taskevich closed this Sep 7, 2025
@taskevich taskevich reopened this Sep 7, 2025
@taskevich
pythongh-60107: Tests for readinto
905b3ec
picnixz
picnixz reviewed Sep 7, 2025
View reviewed changes
Comment thread Lib/test/test_io/test_general.py
self.assertEqual(rawio.read(2), b"")

def test_exact_RawIOBase(self):
rawio = self.MockRawIOWithoutRead((b"ab", b"cd"))

@picnixz picnixz Sep 7, 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.

Maybe I wasn't precise enough. RawIOBase.read() is implemented in terms of readinto. So, what we want to do is provide a readinto method that does something bad as it's given the internal buffer we constructed in C. I don't know if it's possible to cause a segfault on main with that approach though.

Stated otherwise, we want some class:

class EvilReadInto(MockRawIOWithoutRead):
    def readinto(self, buf):
        # do something bad with 'buf'

And then

r = EvilReadInto(something_here)
r.read()  # on main, this call should crash, but not with this patch

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@picnixz
picnixz marked this pull request as draft September 7, 2025 14:19
@bedevere-app bedevere-app Bot removed the awaiting review label Sep 7, 2025
@picnixz

picnixz commented Sep 7, 2025

Copy link
Copy Markdown
Member

I'm going to convert it into a draft until a regression can be found between main and this PR. IOW, we want to be able to do something evil with the buffer we are given in readinto when using main, but we want that evil thing to be impossible with this patch.
All reactions

Sorry, something went wrong.

Uh oh!

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

cmaloney commented Nov 20, 2025

Copy link
Copy Markdown
Contributor

Thanks for the pull request @taskevich ! A slightly different fix for this has landed in #141532 so going to close this one.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cmaloney cmaloney closed this Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments
@cmaloney cmaloney Awaiting requested review from cmaloney cmaloney will be requested when the pull request is marked ready for review cmaloney is a code owner
+1 more reviewer
@auvipy auvipy auvipy 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.

4 participants

@taskevich @picnixz @cmaloney @auvipy
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.