Skip to content

gh-151640: avoid sharing BytesIO buffer in free-threaded builds#151651

Open
pedramkarimii wants to merge 4 commits into
python:mainfrom
pedramkarimii:gh-151640-bytesio-ft-race
Open

gh-151640: avoid sharing BytesIO buffer in free-threaded builds#151651
pedramkarimii wants to merge 4 commits into
python:mainfrom
pedramkarimii:gh-151640-bytesio-ft-race

Conversation

@pedramkarimii

@pedramkarimii pedramkarimii commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown

Fixes gh-151640.

In free-threaded builds, whole-buffer BytesIO.read() and BytesIO.peek() could return self->buf directly via Py_NewRef(). BytesIO.getvalue() could also return the internal buffer directly. A concurrent writer could then resize that bytes object while another thread decrefs a returned reference, producing a TSAN-reported race between _PyBytes_Resize() and _Py_DecRefShared().

This change avoids exposing the internal BytesIO buffer in Py_GIL_DISABLED builds. Whole-buffer read(), peek(), and getvalue() now return a copy in free-threaded builds, while preserving the existing fast path for regular GIL builds.

A focused free-threading regression test covers concurrent whole-buffer read(), peek(), and getvalue() calls with buffer-resizing writes.

Tests run:

  • ./python -m test -v test_free_threading.test_io -m test_concurrent_whole_buffer_read_and_resize
  • ./python -m test -v test_free_threading.test_io
  • ./python -m test -v test_io
  • ./python -m test -v test_free_threading
  • ./python -m test -v -R 3:3 test_free_threading.test_io -m test_concurrent_whole_buffer_read_and_resize
  • make patchcheck

@bedevere-app

bedevere-app Bot commented Jun 18, 2026

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.

@python-cla-bot

python-cla-bot Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app bedevere-app Bot mentioned this pull request Jun 18, 2026
Open
@bedevere-app

bedevere-app Bot commented Jun 18, 2026

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.

@pedramkarimii

pedramkarimii commented Jun 18, 2026

Copy link
Copy Markdown
Author

Additional context:

This PR is intended to fix the specific _io.BytesIO free-threaded race reported in gh-151640. The issue report focuses on the whole-buffer BytesIO.read() fast path returning self->buf directly via Py_NewRef(), while a concurrent writer may later resize that same internal bytes object.

While investigating the code path, I also found that BytesIO.getvalue() can expose the same internal buffer in a similar way. For that reason, this PR handles both whole-buffer read() and getvalue() in Py_GIL_DISABLED builds.

The chosen fix is intentionally conservative: in free-threaded builds, avoid returning the internal BytesIO buffer directly to user code, while keeping the existing fast path unchanged for regular GIL builds. This avoids changing behavior/performance for non-free-threaded builds and keeps the patch localized to _io.BytesIO.

I also considered fixing this lower in the resize/shared-buffer ownership path, but that seemed more invasive because it would require changing the interaction between SHARED_BUF(), _PyBytes_Resize(), and buffer ownership/copy-on-write behavior. If maintainers prefer preserving this optimization in free-threaded builds, I’m happy to rework the patch in that direction.

ZeroIntensity
ZeroIntensity reviewed Jun 18, 2026
View reviewed changes
Comment thread Modules/_io/bytesio.c Outdated
#ifndef Py_GIL_DISABLED
if (size > 1 &&
self->pos == 0 && size == PyBytes_GET_SIZE(self->buf) &&
FT_ATOMIC_LOAD_SSIZE_RELAXED(self->exports) == 0) {

@ZeroIntensity ZeroIntensity Jun 18, 2026

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.

FT_ATOMIC calls inside a block where Py_GIL_DISABLED is never defined are useless. Let's remove them.

@pedramkarimii pedramkarimii Jun 19, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done, thanks. I simplified the exports checks in the non-Py_GIL_DISABLED paths.

@pedramkarimii pedramkarimii force-pushed the gh-151640-bytesio-ft-race branch from 15cf615 to 0359497 Compare June 27, 2026 09:03
@pedramkarimii pedramkarimii force-pushed the gh-151640-bytesio-ft-race branch 2 times, most recently from 82e72df to b26fbb8 Compare June 27, 2026 10:35
@pedramkarimii pedramkarimii force-pushed the gh-151640-bytesio-ft-race branch from b26fbb8 to 33bc6c8 Compare June 27, 2026 11:06
@nascheme

nascheme commented Jul 3, 2026

Copy link
Copy Markdown
Member

I think always copying buf is going to cause too much performance regression. In some cases the buffer is going to be large and so this copy will be significant. Tracking if the buf has been shared seems like a better solution. That sounds like your "more invasive" approach, which I think is worth the extra work and complexity. I prototyped this change here: https://github.com/nascheme/cpython/tree/gh-151640-bytesio-shared-flag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity left review comments

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Data race between resize_buffer_lock_held and _Py_DecRefShared on a shared BytesIO buffer

3 participants

@pedramkarimii @nascheme @ZeroIntensity