X Tutup
Skip to content

gh-129732: Fix race in on shared->array in qsbr code under free-threa…#129738

Merged
colesbury merged 2 commits intopython:mainfrom
hawkinsp:qsbr
Feb 6, 2025
Merged

gh-129732: Fix race in on shared->array in qsbr code under free-threa…#129738
colesbury merged 2 commits intopython:mainfrom
hawkinsp:qsbr

Conversation

@hawkinsp
Copy link
Contributor

@hawkinsp hawkinsp commented Feb 6, 2025 •
edited by bedevere-app bot
Loading

…ding.

The read of shared->array should happen under the lock to avoid a race.

Fixes #129732

@bedevere-app bedevere-app bot mentioned this pull request Feb 6, 2025
Closed
@colesbury colesbury self-requested a review February 6, 2025 17:15
@colesbury colesbury added the needs backport to 3.13 bugs and security fixes label Feb 6, 2025
@colesbury
Copy link
Contributor

colesbury commented Feb 6, 2025

This LGTM, just needs a NEWS blurb

@hawkinsp
pythongh-129732: Fix race in on shared->array in qsbr code under free…
c15a0ba 
…-threading.

The read of shared->array should happen under the lock to avoid a race.

Fixes python#129732
@hawkinsp
Copy link
Contributor Author

hawkinsp commented Feb 6, 2025

This LGTM, just needs a NEWS blurb

Done. I'm not sure how these normally look, let me know if you need something different.

colesbury
colesbury approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

Thanks!

@colesbury colesbury enabled auto-merge (squash) February 6, 2025 18:26
@colesbury colesbury merged commit b4ff8b2 into python:main Feb 6, 2025
42 checks passed
@miss-islington-app
Copy link

miss-islington-app bot commented Feb 6, 2025

Thanks @hawkinsp for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

miss-islington-app bot commented Feb 6, 2025

Sorry, @hawkinsp and @colesbury, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker b4ff8b22b3066b814c3758f87eaddfa923e657ed 3.13

@bedevere-app
Copy link

bedevere-app bot commented Feb 6, 2025

GH-129747 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Feb 6, 2025
colesbury pushed a commit to colesbury/cpython that referenced this pull request Feb 6, 2025
@hawkinsp @colesbury
[3.13] pythongh-129732: Fix race on shared->array in qsbr code unde…
61d6e0c 
…r free-threading (pythongh-129738)

The read of `shared->array` should happen under the lock to avoid a race.
(cherry picked from commit b4ff8b2)

Co-authored-by: Peter Hawkins <phawkins@google.com>
colesbury added a commit that referenced this pull request Feb 6, 2025
@colesbury @hawkinsp
[3.13] gh-129732: Fix race on shared->array in qsbr code under free…
f7cc862 
…-threading (gh-129738) (gh-129747)

The read of `shared->array` should happen under the lock to avoid a race.
(cherry picked from commit b4ff8b2)

Co-authored-by: Peter Hawkins <phawkins@google.com>
@bedevere-bot
Copy link

bedevere-bot commented Feb 6, 2025

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 RHEL8 LTO 3.13 has failed when building commit f7cc862.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1393/builds/515) and take a look at the build logs.
  4. Check if the failure is related to this commit (f7cc862) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1393/builds/515

Summary of the results of the build (if available):

==

Click to see traceback logs 
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.13.cstratak-RHEL8-aarch64.lto/build/Lib/threading.py", line 1041, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.13.cstratak-RHEL8-aarch64.lto/build/Lib/threading.py", line 992, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.13.cstratak-RHEL8-aarch64.lto/build/Lib/test/test_interpreters/test_stress.py", line 47, in run
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.13.cstratak-RHEL8-aarch64.lto/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.13.cstratak-RHEL8-aarch64.lto/build/Lib/threading.py", line 1041, in _bootstrap_inner
    self.run()
    ~~~~~~~~^^
  File "/home/buildbot/buildarea/3.13.cstratak-RHEL8-aarch64.lto/build/Lib/threading.py", line 992, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.13.cstratak-RHEL8-aarch64.lto/build/Lib/test/test_interpreters/test_stress.py", line 30, in task
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.13.cstratak-RHEL8-aarch64.lto/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k

@colesbury
Copy link
Contributor

colesbury commented Feb 6, 2025 •
edited
Loading

The buildbot failure looks unrelated. The modified file is only used in the free threading build and the failed buildbot is for the default (GIL-enabled) build.

EDIT: See #128381

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025
@hawkinsp @srinivasreddy
pythongh-129732: Fix race on shared->array in qsbr code under free-…
8b80c41 
…threading (pythongh-129738)

The read of `shared->array` should happen under the lock to avoid a race.
cmaloney pushed a commit to cmaloney/cpython that referenced this pull request Feb 8, 2025
@hawkinsp @cmaloney
pythongh-129732: Fix race on shared->array in qsbr code under free-…
696d0c3 
…threading (pythongh-129738)

The read of `shared->array` should happen under the lock to avoid a race.
@colesbury colesbury removed their assignment Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Race between grow_thread_array and _Py_qsbr_reserve under free threading

3 participants

@hawkinsp @colesbury @bedevere-bot
X Tutup