Skip to content

gh-129967: Fix race condition in repr(set) #129978

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.

Sign up for GitHub

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 6 commits into from
Feb 11, 2025
Merged

gh-129967: Fix race condition in repr(set) #129978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b652577
gh-129967: Fix race condition in `repr(set)`
colesbury Feb 10, 2025
2f04291
Update blurb
colesbury Feb 10, 2025
d12d8b6
Apply suggestions from code review
colesbury Feb 11, 2025
1efd4e5
Revert changes to test_set.py
colesbury Feb 11, 2025
ddb87fe
Add reference to GH issue
colesbury Feb 11, 2025
bfdc05a
Merge branch 'main' into gh-129967-set-repr-clear
colesbury Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions Lib/test/test_free_threading/test_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import unittest

from threading import Thread, Barrier
from unittest import TestCase

from test.support import threading_helper


@threading_helper.requires_working_threading()
class TestSet(TestCase):
def test_repr_clear(self):
"""Test repr() of a set while another thread is calling clear()"""
NUM_ITERS = 10
NUM_REPR_THREADS = 10
barrier = Barrier(NUM_REPR_THREADS + 1)
s = {1, 2, 3, 4, 5, 6, 7, 8}

def clear_set():
barrier.wait()
s.clear()

def repr_set():
barrier.wait()
set_reprs.append(repr(s))

for _ in range(NUM_ITERS):
set_reprs = []
threads = [Thread(target=clear_set)]
for _ in range(NUM_REPR_THREADS):
threads.append(Thread(target=repr_set))
for t in threads:
t.start()
for t in threads:
t.join()

for set_repr in set_reprs:
self.assertIn(set_repr, ("set()", "{1, 2, 3, 4, 5, 6, 7, 8}"))
Comment on lines +36 to +37
Copy link
Member

Choose a reason for hiding this comment

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

... this would report all failing set reprs, not just the first one, and avoid hardcoding the repr we're expecting. (4/4)

Suggested change
for set_repr in set_reprs:
self.assertIn(set_repr, ("set()", "{1, 2, 3, 4, 5, 6, 7, 8}"))
self.assertEqual(set_reprs, expected)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This assertion ended up not working:

expected is { "set()", "{1, 2, 3, 4, 5, 6, 7, 8}" }

set_reprs may be any of the three cases:

  • { "set()", "{1, 2, 3, 4, 5, 6, 7, 8}" }
  • { "{1, 2, 3, 4, 5, 6, 7, 8}" }
  • { "set()" }

So we could do self.assertTrue(set_reprs.issubset(expected)) or something similar, but at that point I'd prefer the original assertion failure message.



if __name__ == "__main__":
unittest.main()
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-02-10-20-01-56.gh-issue-129967.J60tEl.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a race condition in the :term:`free threading` build when ``repr(set)``
is called concurrently with ``set.clear()``.
13 changes: 11 additions & 2 deletions Objects/setobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,9 +535,18 @@ set_repr_lock_held(PySetObject *so)
return PyUnicode_FromFormat("%s()", Py_TYPE(so)->tp_name);
}

keys = PySequence_List((PyObject *)so);
if (keys == NULL)
// gh-129967: avoid PySequence_List because it might re-lock the object
// lock or the GIL and allow something to clear the set from underneath us.
keys = PyList_New(so->used);
if (keys == NULL) {
goto done;
}

Py_ssize_t pos = 0, idx = 0;
setentry *entry;
while (set_next(so, &pos, &entry)) {
PyList_SET_ITEM(keys, idx++, Py_NewRef(entry->key));
}

/* repr(keys)[1:-1] */
listrepr = PyObject_Repr(keys);
Expand Down
Loading