Skip to content

Commit 75aa17f

Browse files
hawkinsphawkinsp
committed
gh-132641: Fix race in lru_cache due to inconsistent critical section use.
The bounded_lru_cache code was using a critical section on the lru cache object to protect dictionary accesses in some code paths, but using the critical section on the dictionary itself to protect accesses in other code paths. This led to races since not all threads agreed on which lock they needed to be holding. Instead, always use a critical section on the underlying dictionary, rather than the lru cache object itself. Fixes #132641
1 parent 27ed645 commit 75aa17f

File tree

5 files changed

+94
-5
lines changed

5 files changed

+94
-5
lines changed

‎Include/internal/pycore_dict.h

+2
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,8 @@ extern int _PyDict_Pop_KnownHash(
150150
Py_hash_t hash,
151151
PyObject **result);
152152

153+
extern void _PyDict_Clear_LockHeld(PyObject *op);
154+
153155
#ifdef Py_GIL_DISABLED
154156
PyAPI_FUNC(void) _PyDict_EnsureSharedOnRead(PyDictObject *mp);
155157
#endif

‎Lib/test/test_free_threading/test_functools.py

+75
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
import random
2+
import unittest
3+
4+
from functools import lru_cache
5+
from threading import Barrier, Thread
6+
7+
from test.support import threading_helper
8+
9+
@threading_helper.requires_working_threading()
10+
class TestLruCache(unittest.TestCase):
11+
12+
def _test_concurrent_operations(self, maxsize):
13+
num_threads = 10
14+
b = Barrier(num_threads)
15+
@lru_cache(maxsize=maxsize)
16+
def func(arg=0):
17+
return object()
18+
19+
20+
def thread_func():
21+
b.wait()
22+
for i in range(1000):
23+
r = random.randint(0, 1000)
24+
if i < 800:
25+
func(i)
26+
elif i < 900:
27+
func.cache_info()
28+
else:
29+
func.cache_clear()
30+
31+
threads = []
32+
for i in range(num_threads):
33+
t = Thread(target=thread_func)
34+
threads.append(t)
35+
36+
with threading_helper.start_threads(threads):
37+
pass
38+
39+
def test_concurrent_operations_unbounded(self):
40+
self._test_concurrent_operations(maxsize=None)
41+
42+
def test_concurrent_operations_bounded(self):
43+
self._test_concurrent_operations(maxsize=128)
44+
45+
def _test_reentrant_cache_clear(self, maxsize):
46+
num_threads = 10
47+
b = Barrier(num_threads)
48+
@lru_cache(maxsize=maxsize)
49+
def func(arg=0):
50+
func.cache_clear()
51+
return object()
52+
53+
54+
def thread_func():
55+
b.wait()
56+
for i in range(1000):
57+
func(random.randint(0, 10000))
58+
59+
threads = []
60+
for i in range(num_threads):
61+
t = Thread(target=thread_func)
62+
threads.append(t)
63+
64+
with threading_helper.start_threads(threads):
65+
pass
66+
67+
def test_reentrant_cache_clear_unbounded(self):
68+
self._test_reentrant_cache_clear(maxsize=None)
69+
70+
def test_reentrant_cache_clear_bounded(self):
71+
self._test_reentrant_cache_clear(maxsize=128)
72+
73+
74+
if __name__ == "__main__":
75+
unittest.main()

‎Misc/NEWS.d/next/Library/2025-05-09-20-59-24.gh-issue-132641.3qTw44.rst

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fixed a race in :func:`functools.lru_cache` under free-threading.

‎Modules/_functoolsmodule.c

+11-5
Original file line numberDiff line numberDiff line change
@@ -1383,8 +1383,8 @@ bounded_lru_cache_update_lock_held(lru_cache_object *self,
13831383
this same key, then this setitem call will update the cache dict
13841384
with this new link, leaving the old link as an orphan (i.e. not
13851385
having a cache dict entry that refers to it). */
1386-
if (_PyDict_SetItem_KnownHash(self->cache, key, (PyObject *)link,
1387-
hash) < 0) {
1386+
if (_PyDict_SetItem_KnownHash_LockHeld((PyDictObject *)self->cache, key,
1387+
(PyObject *)link, hash) < 0) {
13881388
Py_DECREF(link);
13891389
return NULL;
13901390
}
@@ -1453,8 +1453,8 @@ bounded_lru_cache_update_lock_held(lru_cache_object *self,
14531453
for successful insertion in the cache dict before adding the
14541454
link to the linked list. Otherwise, the potentially reentrant
14551455
__eq__ call could cause the then orphan link to be visited. */
1456-
if (_PyDict_SetItem_KnownHash(self->cache, key, (PyObject *)link,
1457-
hash) < 0) {
1456+
if (_PyDict_SetItem_KnownHash_LockHeld((PyDictObject *)self->cache, key,
1457+
(PyObject *)link, hash) < 0) {
14581458
/* Somehow the cache dict update failed. We no longer can
14591459
restore the old link. Let the error propagate upward and
14601460
leave the cache short one link. */
@@ -1689,7 +1689,13 @@ _functools__lru_cache_wrapper_cache_clear_impl(PyObject *self)
16891689
lru_list_elem *list = lru_cache_unlink_list(_self);
16901690
FT_ATOMIC_STORE_SSIZE_RELAXED(_self->hits, 0);
16911691
FT_ATOMIC_STORE_SSIZE_RELAXED(_self->misses, 0);
1692-
PyDict_Clear(_self->cache);
1692+
if (_self->wrapper == bounded_lru_cache_wrapper) {
1693+
/* The critical section on the lru cache itself protects the dictionary
1694+
for bounded_lru_cache instances. */
1695+
_PyDict_Clear_LockHeld(_self->cache);
1696+
} else {
1697+
PyDict_Clear(_self->cache);
1698+
}
16931699
lru_cache_clear_list(list);
16941700
Py_RETURN_NONE;
16951701
}

‎Objects/dictobject.c

+5
Original file line numberDiff line numberDiff line change
@@ -2915,6 +2915,11 @@ clear_lock_held(PyObject *op)
29152915
ASSERT_CONSISTENT(mp);
29162916
}
29172917

2918+
void
2919+
_PyDict_Clear_LockHeld(PyObject *op) {
2920+
clear_lock_held(op);
2921+
}
2922+
29182923
void
29192924
PyDict_Clear(PyObject *op)
29202925
{

0 commit comments

Comments
 (0)