Skip to content

gh-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe #114843

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

Merged
merged 13 commits into from
Feb 14, 2024
27 changes: 15 additions & 12 deletions Lib/test/support/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1727,19 +1727,22 @@ def _check_tracemalloc():


def check_free_after_iterating(test, iter, cls, args=()):
class A(cls):
def __del__(self):
nonlocal done
done = True
try:
next(it)
except StopIteration:
pass

done = False
it = iter(A(*args))
# Issue 26494: Shouldn't crash
test.assertRaises(StopIteration, next, it)
def wrapper():
class A(cls):
def __del__(self):
nonlocal done
done = True
try:
next(it)
except StopIteration:
pass

it = iter(A(*args))
# Issue 26494: Shouldn't crash
test.assertRaises(StopIteration, next, it)

wrapper()
# The sequence should be deallocated just after the end of iterating
gc_collect()
test.assertTrue(done)
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_iter.py
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ def __eq__(self, other):
# listiter_reduce_general
self.assertEqual(
run("reversed", orig["reversed"](list(range(8)))),
(iter, ([],))
(reversed, ([],))
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was added from #101769, not sure that it will be okay to change the implementation detail.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is OK, see my other comment.

)

for case in types:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
For an empty reverse iterator for list will be reduced to :func:`reversed`.
Patch by Donghee Na
102 changes: 52 additions & 50 deletions Objects/listobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ class list "PyListObject *" "&PyList_Type"

_Py_DECLARE_STR(list_err, "list index out of range");

#ifdef Py_GIL_DISABLED
# define LOAD_SSIZE(value) _Py_atomic_load_ssize_relaxed(&value)
# define STORE_SSIZE(value, new_value) _Py_atomic_store_ssize_relaxed(&value, new_value)
#else
# define LOAD_SSIZE(value) value
# define STORE_SSIZE(value, new_value) value = new_value
#endif

#ifdef WITH_FREELISTS
static struct _Py_list_state *
get_list_state(void)
Expand Down Expand Up @@ -2981,7 +2989,8 @@ list___sizeof___impl(PyListObject *self)
/*[clinic end generated code: output=3417541f95f9a53e input=b8030a5d5ce8a187]*/
{
size_t res = _PyObject_SIZE(Py_TYPE(self));
res += (size_t)self->allocated * sizeof(void*);
Py_ssize_t allocated = LOAD_SSIZE(self->allocated);
res += (size_t)allocated * sizeof(void*);
return PyLong_FromSize_t(res);
}

Expand Down Expand Up @@ -3383,33 +3392,34 @@ static PyObject *
listiter_next(PyObject *self)
{
_PyListIterObject *it = (_PyListIterObject *)self;
PyListObject *seq;
PyObject *item;

assert(it != NULL);
seq = it->it_seq;
if (seq == NULL)
Py_ssize_t index = LOAD_SSIZE(it->it_index);
if (index < 0) {
return NULL;
assert(PyList_Check(seq));

if (it->it_index < PyList_GET_SIZE(seq)) {
item = PyList_GET_ITEM(seq, it->it_index);
++it->it_index;
return Py_NewRef(item);
}

it->it_seq = NULL;
Py_DECREF(seq);
return NULL;
PyObject *item = list_get_item_ref(it->it_seq, index);
if (item == NULL) {
// out-of-bounds
STORE_SSIZE(it->it_index, -1);
#ifndef Py_GIL_DISABLED
PyListObject *seq = it->it_seq;
it->it_seq = NULL;
Py_DECREF(seq);
#endif
return NULL;
}
STORE_SSIZE(it->it_index, index + 1);
return item;
}

static PyObject *
listiter_len(PyObject *self, PyObject *Py_UNUSED(ignored))
{
assert(self != NULL);
_PyListIterObject *it = (_PyListIterObject *)self;
Py_ssize_t len;
if (it->it_seq) {
len = PyList_GET_SIZE(it->it_seq) - it->it_index;
Py_ssize_t index = LOAD_SSIZE(it->it_index);
if (index >= 0) {
Py_ssize_t len = PyList_GET_SIZE(it->it_seq) - index;
if (len >= 0)
return PyLong_FromSsize_t(len);
}
Expand All @@ -3430,8 +3440,8 @@ listiter_setstate(PyObject *self, PyObject *state)
if (index == -1 && PyErr_Occurred())
return NULL;
if (it->it_seq != NULL) {
if (index < 0)
index = 0;
if (index < -1)
index = -1;
else if (index > PyList_GET_SIZE(it->it_seq))
index = PyList_GET_SIZE(it->it_seq); /* iterator exhausted */
it->it_index = index;
Expand Down Expand Up @@ -3536,34 +3546,33 @@ static PyObject *
listreviter_next(PyObject *self)
{
listreviterobject *it = (listreviterobject *)self;
PyObject *item;
Py_ssize_t index;
PyListObject *seq;

assert(it != NULL);
seq = it->it_seq;
if (seq == NULL) {
return NULL;
}
PyListObject *seq = it->it_seq;
assert(PyList_Check(seq));

index = it->it_index;
if (index>=0 && index < PyList_GET_SIZE(seq)) {
item = PyList_GET_ITEM(seq, index);
it->it_index--;
return Py_NewRef(item);
Py_ssize_t index = LOAD_SSIZE(it->it_index);
if (index < 0) {
return NULL;
}
PyObject *item = list_get_item_ref(seq, index);
if (item != NULL) {
STORE_SSIZE(it->it_index, index - 1);
return item;
}
it->it_index = -1;
STORE_SSIZE(it->it_index, -1);
#ifndef Py_GIL_DISABLED
it->it_seq = NULL;
Py_DECREF(seq);
#endif
return NULL;
}

static PyObject *
listreviter_len(PyObject *self, PyObject *Py_UNUSED(ignored))
{
listreviterobject *it = (listreviterobject *)self;
Py_ssize_t len = it->it_index + 1;
Py_ssize_t index = LOAD_SSIZE(it->it_index);
Py_ssize_t len = index + 1;
if (it->it_seq == NULL || PyList_GET_SIZE(it->it_seq) < len)
len = 0;
return PyLong_FromSsize_t(len);
Expand Down Expand Up @@ -3598,36 +3607,29 @@ static PyObject *
listiter_reduce_general(void *_it, int forward)
{
PyObject *list;
PyObject *iter;

/* _PyEval_GetBuiltin can invoke arbitrary code,
* call must be before access of iterator pointers.
* see issue #101765 */

/* the objects are not the same, index is of different types! */
if (forward) {
PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
if (!iter) {
return NULL;
}
iter = _PyEval_GetBuiltin(&_Py_ID(iter));
_PyListIterObject *it = (_PyListIterObject *)_it;
if (it->it_seq) {
if (it->it_index >= 0) {
return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
}
Py_DECREF(iter);
} else {
PyObject *reversed = _PyEval_GetBuiltin(&_Py_ID(reversed));
if (!reversed) {
return NULL;
}
iter = _PyEval_GetBuiltin(&_Py_ID(reversed));
listreviterobject *it = (listreviterobject *)_it;
if (it->it_seq) {
return Py_BuildValue("N(O)n", reversed, it->it_seq, it->it_index);
if (it->it_index >= 0) {
return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
}
Py_DECREF(reversed);
}
/* empty iterator, create an empty list */
list = PyList_New(0);
if (list == NULL)
return NULL;
return Py_BuildValue("N(N)", _PyEval_GetBuiltin(&_Py_ID(iter)), list);
return Py_BuildValue("N(N)", iter, list);
Copy link
Member

Choose a reason for hiding this comment

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

This is the change causing the behavior difference in the test. Previously, an empty iterator would always reduce to the iter builtin, now it will reduce to reversed if it was a reverse iterator. IMO the new behavior is more correct (consistent with the non-empty behavior) and we can consider this a bugfix.

Copy link
Member Author

@corona10 corona10 Feb 9, 2024

Choose a reason for hiding this comment

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

Okay in that case, we need to create backport patches for this.

}
8 changes: 5 additions & 3 deletions Python/bytecodes.c
Original file line number Diff line number Diff line change
Expand Up @@ -2614,11 +2614,14 @@ dummy_func(
assert(Py_TYPE(iter) == &PyListIter_Type);
STAT_INC(FOR_ITER, hit);
PyListObject *seq = it->it_seq;
if (seq == NULL || it->it_index >= PyList_GET_SIZE(seq)) {
if ((size_t)it->it_index >= (size_t)PyList_GET_SIZE(seq)) {
it->it_index = -1;
#ifndef Py_GIL_DISABLED
if (seq != NULL) {
it->it_seq = NULL;
Py_DECREF(seq);
}
#endif
Py_DECREF(iter);
STACK_SHRINK(1);
/* Jump forward oparg, then skip following END_FOR and POP_TOP instructions */
Expand All @@ -2632,8 +2635,7 @@ dummy_func(
_PyListIterObject *it = (_PyListIterObject *)iter;
assert(Py_TYPE(iter) == &PyListIter_Type);
PyListObject *seq = it->it_seq;
DEOPT_IF(seq == NULL);
DEOPT_IF(it->it_index >= PyList_GET_SIZE(seq));
DEOPT_IF((size_t)it->it_index >= (size_t)PyList_GET_SIZE(seq));
}

op(_ITER_NEXT_LIST, (iter -- iter, next)) {
Expand Down
3 changes: 1 addition & 2 deletions Python/executor_cases.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion Python/generated_cases.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.