-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-129107: make bytearrayiter free-threading safe #130096
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-129107: make bytearrayiter free-threading safe #130096
Conversation
This PR is split off of #129108. Ping @kumaraditya303 |
EDIT: Removed, unnecessary critical sections in |
@kumaraditya303 I thought you would do the iterator first and backport it so removed an unnecessary critical section here if static PyObject *
bytearrayiter_next(PyObject *self)
{
bytesiterobject *it = _bytesiterobject_CAST(self);
int val;
assert(it != NULL);
Py_ssize_t index = FT_ATOMIC_LOAD_SSIZE_RELAXED(it->it_index);
if (index < 0) {
return NULL;
}
PyByteArrayObject *seq = it->it_seq;
assert(PyByteArray_Check(seq));
Py_BEGIN_CRITICAL_SECTION(seq);
if (index < PyByteArray_GET_SIZE(seq)) {
val = (unsigned char)PyByteArray_AS_STRING(seq)[index];
}
else {
val = -1;
}
Py_END_CRITICAL_SECTION();
if (val == -1) {
FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, -1);
#ifndef Py_GIL_DISABLED
it->it_seq = NULL;
Py_DECREF(seq);
#endif
return NULL;
}
FT_ATOMIC_STORE_SSIZE_RELAXED(it->it_index, index + 1);
return _PyLong_FromUnsignedChar((unsigned char)val);
} Though can also just use |
I don't intend to backport the thread safety of bytearray as change is large and there isn't much point in backporting the iterator while bytearray isn't thread safe. @colesbury Do you think we should backport the thread safety of Yes, it is not safe to read data without acquiring critical section of the seq, you need to add back the critical section, the change to use atomic makes sense. |
I probably wouldn't backport the changes. |
Misc/NEWS.d/next/Core_and_Builtins/2025-02-13-20-42-53.gh-issue-129107._olg-L.rst
Outdated
Show resolved
Hide resolved
…e-129107._olg-L.rst Co-authored-by: Kumar Aditya <kumaraditya@python.org>
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.
LGTM but I would like a review from @colesbury as well.
This is the bytearray iterator made safe under free-threading. The behavior is copied from the list iterator, it is not synchronized but it won't crash.