-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
gh-149965: Improve thread-safety in ElementObjects within element_ass_subscr
#149966
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| WIthin ``Modules/_elementtree.c``, improve thread-safety in ElementObjects within ``element_ass_subscr`` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1863,6 +1863,35 @@ element_subscr(PyObject *op, PyObject *item) | |
| } | ||
| } | ||
|
|
||
| // Pointer-by-pointer memmove for PyObject** arrays that is safe | ||
| // for shared ElementObjects in Py_GIL_DISABLED builds. | ||
| static void | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we should duplicate Could you please link to the related C-API Working group issue in this PR, and cross-reference it in the original issue?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the function is not exposed as part of the public API, I don't think linking a C-API Working Group issue is needed here. Anyway, it's inspired by #149936
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add a link to capi-workgroup/decisions#106 for traceability. |
||
| ptr_wise_atomic_memmove(ElementObject *a, PyObject **dest, PyObject **src, Py_ssize_t n) | ||
| { | ||
| #ifndef Py_GIL_DISABLED | ||
| memmove(dest, src, n * sizeof(PyObject *)); | ||
| #else | ||
| // XXX: maybe a critical section isn't needed for ElementObject? | ||
| // _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(a); | ||
| if (_Py_IsOwnedByCurrentThread((PyObject *)a) && !_PyObject_GC_IS_SHARED(a)) { | ||
| // No other threads can read this list concurrently | ||
| memmove(dest, src, n * sizeof(PyObject *)); | ||
| return; | ||
| } | ||
| if (dest < src) { | ||
| for (Py_ssize_t i = 0; i != n; i++) { | ||
| _Py_atomic_store_ptr_release(&dest[i], src[i]); | ||
| } | ||
| } | ||
| else { | ||
| // copy backwards to avoid overwriting src before it's read | ||
| for (Py_ssize_t i = n; i != 0; i--) { | ||
| _Py_atomic_store_ptr_release(&dest[i - 1], src[i - 1]); | ||
| } | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| static int | ||
| element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) | ||
| { | ||
|
|
@@ -1938,19 +1967,21 @@ element_ass_subscr(PyObject *op, PyObject *item, PyObject *value) | |
|
|
||
| PyList_SET_ITEM(recycle, i, self->extra->children[cur]); | ||
|
|
||
| memmove( | ||
| ptr_wise_atomic_memmove( | ||
| self, | ||
| self->extra->children + cur - i, | ||
| self->extra->children + cur + 1, | ||
| num_moved * sizeof(PyObject *)); | ||
| num_moved); | ||
| } | ||
|
|
||
| /* Leftover "tail" after the last removed child */ | ||
| cur = start + (size_t)slicelen * step; | ||
| if (cur < (size_t)self->extra->length) { | ||
| memmove( | ||
| ptr_wise_atomic_memmove( | ||
| self, | ||
| self->extra->children + cur - slicelen, | ||
| self->extra->children + cur, | ||
| (self->extra->length - cur) * sizeof(PyObject *)); | ||
| self->extra->length - cur); | ||
| } | ||
|
|
||
| self->extra->length -= slicelen; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.