GeezerVison is showing github.com — gh-144446: Fix thread safety of gi_frame, cr_frame and ag_frame in free-threading by kumaraditya303 · Pull Request #156037 · python/cpython
Conversation
Fixes thread safety of reading gi_frame, cr_frame and ag_frame while the generator is running or finishing in another thread. Previously the reader could create a frame object for an interpreter frame that was concurrently being cleared, or two threads could each create a frame object for the same frame.
The frame object is now created within the generator's critical section, re-checking the frame state after acquiring it so a finished generator returns None instead of a frame for a cleared iframe. Clearing the generator's frame in gen_clear_frame() and clear_gen_frame() now also holds the critical section so it cannot race with the getter. frame->frame_obj is now set with a compare exchange as the running thread can create it without holding the generator's critical section (e.g. through sys._getframe()) — the losing thread discards its frame object and uses the existing one — and it is cleared with an atomic exchange and read with an acquire load.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
… in free-threading build
Uh oh!
There was an error while loading. Please reload this page.
mpage reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This looks correct to me. I'm a little worried about the performance impact of adding a _Py_atomic_compare_exchange_ptr on the return path of every Python function, though. I kicked off a benchmarking run to see what the impact is on the benchmark suite.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Benchmark results look like a small regression (~1%).
I don't think we need to use an exchange in _PyFrame_ClearExceptCode, though, because access to frame->frame_obj is always mediated by the generator's critical section when _PyFrame_ClearExceptCode is executing. Assuming that's correct, I think the change to _PyFrame_ClearExceptCode can just be:
PyFrameObject *f = FT_ATOMIC_LOAD_PTR_RELAXED(frame->frame_obj);
if (f != NULL) {
FT_ATOMIC_STORE_PTR_RELAXED(frame->frame_obj, NULL);
if (!_PyObject_IsUniquelyReferenced((PyObject *)f)) {
take_ownership(f, frame);
Py_DECREF(f);
return;
}
Py_DECREF(f);
} |
kumaraditya303 reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️
Hi! The buildbot AMD64 CentOS9 NoGIL 3.x (tier-1) has failed when building commit db92901.
What do you need to do:
- Don't panic.
- Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
- Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1609/builds/6887) and take a look at the build logs.
- Check if the failure is related to this commit (db92901) or if it is a false positive.
- 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/1609/builds/6887
Failed tests:
Failed subtests:
- test_build - test.test_cppext.TestInteralCAPI.test_build
Summary of the results of the build (if available):
==
Click to see traceback logs
Traceback (most recent call last):
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/test_cppext/__init__.py", line 146, in test_build
self.check_build('_testcppext_internal', **kwargs)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/test_cppext/__init__.py", line 37, in check_build
self._check_build(extension_name, python_exe,
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
std=std, limited=limited,
^^^^^^^^^^^^^^^^^^^^^^^^^
extra_cflags=extra_cflags)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/test_cppext/__init__.py", line 82, in _check_build
run_cmd('Install', cmd)
~~~~~~~^^^^^^^^^^^^^^^^
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/test_cppext/__init__.py", line 63, in run_cmd
subprocess.run(cmd, check=True, env=env)
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/subprocess.py", line 692, in run
raise CalledProcessError(retcode, process.args,
output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command ['/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/build/test_python_508950æ/tempcwd/env/bin/python', '-X', 'dev', '-m', 'pip', 'install', '--no-build-isolation', '/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/build/test_python_508950æ/tempcwd/pkg', '-v'] returned non-zero exit status 1.
Traceback (most recent call last):
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/test_cppext/__init__.py", line 146, in test_build
self.check_build('_testcppext_internal', **kwargs)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/test_cppext/__init__.py", line 37, in check_build
self._check_build(extension_name, python_exe,
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
std=std, limited=limited,
^^^^^^^^^^^^^^^^^^^^^^^^^
extra_cflags=extra_cflags)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/test_cppext/__init__.py", line 82, in _check_build
run_cmd('Install', cmd)
~~~~~~~^^^^^^^^^^^^^^^^
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/test/test_cppext/__init__.py", line 63, in run_cmd
subprocess.run(cmd, check=True, env=env)
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Lib/subprocess.py", line 692, in run
raise CalledProcessError(retcode, process.args,
output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command ['/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/build/test_python_554276æ/tempcwd/env/bin/python', '-X', 'dev', '-m', 'pip', 'install', '--no-build-isolation', '/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/build/test_python_554276æ/tempcwd/pkg', '-v'] returned non-zero exit status 1.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️
Hi! The buildbot ARM64 MacOS M1 NoGIL 3.x (tier-2) has failed when building commit db92901.
What do you need to do:
- Don't panic.
- Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
- Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1270/builds/8482) and take a look at the build logs.
- Check if the failure is related to this commit (db92901) or if it is a false positive.
- 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/1270/builds/8482
Failed tests:
Failed subtests:
- test_build - test.test_cppext.TestInteralCAPI.test_build
Summary of the results of the build (if available):
==
Click to see traceback logs
Traceback (most recent call last):
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/test/test_cppext/__init__.py", line 146, in test_build
self.check_build('_testcppext_internal', **kwargs)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/test/test_cppext/__init__.py", line 37, in check_build
self._check_build(extension_name, python_exe,
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
std=std, limited=limited,
^^^^^^^^^^^^^^^^^^^^^^^^^
extra_cflags=extra_cflags)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/test/test_cppext/__init__.py", line 82, in _check_build
run_cmd('Install', cmd)
~~~~~~~^^^^^^^^^^^^^^^^
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/test/test_cppext/__init__.py", line 63, in run_cmd
subprocess.run(cmd, check=True, env=env)
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/subprocess.py", line 692, in run
raise CalledProcessError(retcode, process.args,
output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command ['/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/build/test_python_39839æ/tempcwd/env/bin/python.exe', '-X', 'dev', '-m', 'pip', 'install', '--no-build-isolation', '/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/build/test_python_39839æ/tempcwd/pkg', '-v'] returned non-zero exit status 1.
Traceback (most recent call last):
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/test/test_cppext/__init__.py", line 146, in test_build
self.check_build('_testcppext_internal', **kwargs)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/test/test_cppext/__init__.py", line 37, in check_build
self._check_build(extension_name, python_exe,
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
std=std, limited=limited,
^^^^^^^^^^^^^^^^^^^^^^^^^
extra_cflags=extra_cflags)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/test/test_cppext/__init__.py", line 82, in _check_build
run_cmd('Install', cmd)
~~~~~~~^^^^^^^^^^^^^^^^
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/test/test_cppext/__init__.py", line 63, in run_cmd
subprocess.run(cmd, check=True, env=env)
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/Lib/subprocess.py", line 692, in run
raise CalledProcessError(retcode, process.args,
output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command ['/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/build/test_python_49042æ/tempcwd/env/bin/python.exe', '-X', 'dev', '-m', 'pip', 'install', '--no-build-isolation', '/Users/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.nogil/build/build/test_python_49042æ/tempcwd/pkg', '-v'] returned non-zero exit status 1.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️
Hi! The buildbot AMD64 Alpine Linux NoGIL 3.x (tierless) has failed when building commit db92901.
What do you need to do:
- Don't panic.
- Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
- Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1819/builds/2983) and take a look at the build logs.
- Check if the failure is related to this commit (db92901) or if it is a false positive.
- 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/1819/builds/2983
Summary of the results of the build (if available):
==
Click to see traceback logs
Traceback (most recent call last):
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/test/test_cppext/__init__.py"�[0m, line �[35m146�[0m, in �[35mtest_build�[0m
�[31mself.check_build�[0m�[1;31m('_testcppext_internal', **kwargs)�[0m
�[31m~~~~~~~~~~~~~~~~�[0m�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/test/test_cppext/__init__.py"�[0m, line �[35m37�[0m, in �[35mcheck_build�[0m
�[31mself._check_build�[0m�[1;31m(extension_name, python_exe,�[0m
�[31m~~~~~~~~~~~~~~~~~�[0m�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
�[1;31mstd=std, limited=limited,�[0m
�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
�[1;31mextra_cflags=extra_cflags)�[0m
�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/test/test_cppext/__init__.py"�[0m, line �[35m82�[0m, in �[35m_check_build�[0m
�[31mrun_cmd�[0m�[1;31m('Install', cmd)�[0m
�[31m~~~~~~~�[0m�[1;31m^^^^^^^^^^^^^^^^�[0m
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/test/test_cppext/__init__.py"�[0m, line �[35m63�[0m, in �[35mrun_cmd�[0m
�[31msubprocess.run�[0m�[1;31m(cmd, check=True, env=env)�[0m
�[31m~~~~~~~~~~~~~~�[0m�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/subprocess.py"�[0m, line �[35m692�[0m, in �[35mrun�[0m
raise CalledProcessError(retcode, process.args,
output=stdout, stderr=stderr)
�[1;35msubprocess.CalledProcessError�[0m: �[35mCommand ['/buildbot/buildarea/3.x.ware-alpine.nogil/build/build/test_python_3353076æ/tempcwd/env/bin/python', '-X', 'dev', '-m', 'pip', 'install', '--no-build-isolation', '/buildbot/buildarea/3.x.ware-alpine.nogil/build/build/test_python_3353076æ/tempcwd/pkg', '-v'] returned non-zero exit status 1.�[0m
Traceback (most recent call last):
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/test/test_cppext/__init__.py"�[0m, line �[35m146�[0m, in �[35mtest_build�[0m
�[31mself.check_build�[0m�[1;31m('_testcppext_internal', **kwargs)�[0m
�[31m~~~~~~~~~~~~~~~~�[0m�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/test/test_cppext/__init__.py"�[0m, line �[35m37�[0m, in �[35mcheck_build�[0m
�[31mself._check_build�[0m�[1;31m(extension_name, python_exe,�[0m
�[31m~~~~~~~~~~~~~~~~~�[0m�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
�[1;31mstd=std, limited=limited,�[0m
�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
�[1;31mextra_cflags=extra_cflags)�[0m
�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/test/test_cppext/__init__.py"�[0m, line �[35m82�[0m, in �[35m_check_build�[0m
�[31mrun_cmd�[0m�[1;31m('Install', cmd)�[0m
�[31m~~~~~~~�[0m�[1;31m^^^^^^^^^^^^^^^^�[0m
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/test/test_cppext/__init__.py"�[0m, line �[35m63�[0m, in �[35mrun_cmd�[0m
�[31msubprocess.run�[0m�[1;31m(cmd, check=True, env=env)�[0m
�[31m~~~~~~~~~~~~~~�[0m�[1;31m^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m
File �[35m"/buildbot/buildarea/3.x.ware-alpine.nogil/build/Lib/subprocess.py"�[0m, line �[35m692�[0m, in �[35mrun�[0m
raise CalledProcessError(retcode, process.args,
output=stdout, stderr=stderr)
�[1;35msubprocess.CalledProcessError�[0m: �[35mCommand ['/buildbot/buildarea/3.x.ware-alpine.nogil/build/build/test_python_3389655æ/tempcwd/env/bin/python', '-X', 'dev', '-m', 'pip', 'install', '--no-build-isolation', '/buildbot/buildarea/3.x.ware-alpine.nogil/build/build/test_python_3389655æ/tempcwd/pkg', '-v'] returned non-zero exit status 1.�[0m
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This fails on some buildbots with:
/usr/bin/g++ -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fno-strict-overflow -Wsign-compare -g -Og -Wall -fPIC -I/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/build/test_python_2302880æ/tempcwd/env/include -I/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Include -I/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build -c extension.cpp -o build/temp.linux-x86_64-cpython-316t-pydebug/extension.o -Werror -DMODULE_NAME=_testcppext_internal -DTEST_INTERNAL_C_API=1
In file included from /home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Include/internal/pycore_object.h:13,
from /home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Include/internal/pycore_cell.h:5,
from extension.cpp:29:
/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Include/internal/pycore_interpframe.h: In function ‘PyFrameObject* _PyFrame_GetFrameObject(_PyInterpreterFrame*)’:
/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Include/internal/pycore_pyatomic_ft_wrappers.h:33:32: error: invalid conversion from ‘void*’ to ‘PyFrameObject*’ {aka ‘_frame*’} [-fpermissive]
33 | _Py_atomic_load_ptr_acquire(&value)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
| |
| void*
/home/buildbot/buildarea/3.x.itamaro-centos-aws.nogil/build/Include/internal/pycore_interpframe.h:348:26: note: in expansion of macro ‘FT_ATOMIC_LOAD_PTR_ACQUIRE’
348 | PyFrameObject *res = FT_ATOMIC_LOAD_PTR_ACQUIRE(frame->frame_obj);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
mpage
mpage approved these changes
markshannon
Awaiting requested review from markshannon
markshannon is a code owner
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
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.
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.
|
