Skip to content

Commit 00267a2

Browse files
committed
Use mutex on GIL-enabled builds too.
This race is not free-threading specific.
1 parent d8ebb2d commit 00267a2

4 files changed

Lines changed: 87 additions & 97 deletions

File tree

Lib/test/test_free_threading/test_os.py

Lines changed: 0 additions & 76 deletions
This file was deleted.

Lib/test/test_os/test_os.py

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import sysconfig
2525
import tempfile
2626
import textwrap
27+
import threading
2728
import time
2829
import types
2930
import unittest
@@ -35,6 +36,7 @@
3536
from test.support import infinite_recursion
3637
from test.support import requires_root_user
3738
from test.support import requires_non_root_user
39+
from test.support import threading_helper
3840
from test.support import warnings_helper
3941
from platform import win32_is_iot
4042
from .utils import create_file
@@ -5351,6 +5353,72 @@ def test_resource_warning(self):
53515353
del iterator
53525354

53535355

5356+
@threading_helper.requires_working_threading()
5357+
class ScandirThreadingTest(unittest.TestCase):
5358+
# gh-152754: an os.scandir() iterator shared between threads must not crash.
5359+
5360+
if support.check_sanitizer(thread=True):
5361+
SCANDIR_NUMITEMS = 200
5362+
SCANDIR_N_NEXT = 2
5363+
SCANDIR_N_CLOSE = 2
5364+
SCANDIR_REPEAT = 10
5365+
else:
5366+
SCANDIR_NUMITEMS = 1000
5367+
SCANDIR_N_NEXT = 6
5368+
SCANDIR_N_CLOSE = 3
5369+
SCANDIR_REPEAT = 20
5370+
5371+
def setUp(self):
5372+
self.dir = os.path.realpath(os_helper.TESTFN)
5373+
self.addCleanup(os_helper.rmtree, self.dir)
5374+
os.mkdir(self.dir)
5375+
self.names = set()
5376+
for i in range(self.SCANDIR_NUMITEMS):
5377+
name = f"f{i}"
5378+
create_file(os.path.join(self.dir, name))
5379+
self.names.add(name)
5380+
5381+
def test_close_racing_next(self):
5382+
# One thread's next() racing another's close() must not crash.
5383+
def nexter():
5384+
for _ in self.it:
5385+
pass
5386+
5387+
def closer():
5388+
self.it.close()
5389+
5390+
funcs = [nexter] * self.SCANDIR_N_NEXT + [closer] * self.SCANDIR_N_CLOSE
5391+
for _ in range(self.SCANDIR_REPEAT):
5392+
self.it = os.scandir(self.dir)
5393+
try:
5394+
threading_helper.run_concurrently(funcs)
5395+
finally:
5396+
self.it.close()
5397+
5398+
def test_shared_next(self):
5399+
# Threads sharing one iterator must not crash or lose entries: every
5400+
# entry must be handed to exactly one thread.
5401+
expected = sorted(self.names)
5402+
nthreads = self.SCANDIR_N_NEXT + self.SCANDIR_N_CLOSE
5403+
5404+
for _ in range(self.SCANDIR_REPEAT):
5405+
self.it = os.scandir(self.dir)
5406+
results = []
5407+
results_lock = threading.Lock()
5408+
5409+
def worker():
5410+
local = [entry.name for entry in self.it]
5411+
with results_lock:
5412+
results.extend(local)
5413+
5414+
try:
5415+
threading_helper.run_concurrently([worker] * nthreads)
5416+
finally:
5417+
self.it.close()
5418+
5419+
self.assertEqual(sorted(results), expected)
5420+
5421+
53545422
class TestPEP519(unittest.TestCase):
53555423

53565424
# Abstracted so it can be overridden to test pure Python implementation
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,4 @@
11
Fix a crash when the same :func:`os.scandir` iterator is used concurrently
2-
from multiple threads on the :term:`free-threaded <free threading>` build.
2+
from multiple threads. Iterating and closing the iterator are now serialized
3+
with a per-iterator lock, so that closing it no longer releases the
4+
directory handle while another thread is still reading from it.

Modules/posixmodule.c

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@
2424
#include "pycore_long.h" // _PyLong_IsNegative()
2525
#include "pycore_moduleobject.h" // _PyModule_GetState()
2626
#include "pycore_object.h" // _PyObject_LookupSpecial()
27+
#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_INT_RELAXED()
2728
#include "pycore_pylifecycle.h" // _PyOS_URandom()
2829
#include "pycore_pystate.h" // _PyInterpreterState_GET()
29-
#include "pycore_pyatomic_ft_wrappers.h" // FT_MUTEX_LOCK()
3030
#include "pycore_signal.h" // Py_NSIG
3131
#include "pycore_time.h" // _PyLong_FromTime_t()
3232
#include "pycore_tuple.h" // _PyTuple_FromPairSteal
@@ -16941,11 +16941,6 @@ DirEntry_from_posix_info(PyObject *module, path_t *path, const char *name,
1694116941

1694216942
typedef struct {
1694316943
PyObject_HEAD
16944-
#ifdef Py_GIL_DISABLED
16945-
// Protects scandir iterator state when a os.scandir() iterator is used
16946-
// from multiple threads.
16947-
PyMutex mutex;
16948-
#endif
1694916944
path_t path;
1695016945
#ifdef MS_WINDOWS
1695116946
HANDLE handle;
@@ -16957,6 +16952,9 @@ typedef struct {
1695716952
#ifdef HAVE_FDOPENDIR
1695816953
int fd;
1695916954
#endif
16955+
// Protects the iterator state when an os.scandir() iterator is used from
16956+
// multiple threads.
16957+
PyMutex mutex;
1696016958
} ScandirIterator;
1696116959

1696216960
#define ScandirIterator_CAST(op) ((ScandirIterator *)(op))
@@ -16966,19 +16964,19 @@ typedef struct {
1696616964
static int
1696716965
ScandirIterator_is_closed(ScandirIterator *iterator)
1696816966
{
16969-
FT_MUTEX_LOCK(&iterator->mutex);
16967+
PyMutex_Lock(&iterator->mutex);
1697016968
int closed = iterator->handle == INVALID_HANDLE_VALUE;
16971-
FT_MUTEX_UNLOCK(&iterator->mutex);
16969+
PyMutex_Unlock(&iterator->mutex);
1697216970
return closed;
1697316971
}
1697416972

1697516973
static void
1697616974
ScandirIterator_closedir(ScandirIterator *iterator)
1697716975
{
16978-
FT_MUTEX_LOCK(&iterator->mutex);
16976+
PyMutex_Lock(&iterator->mutex);
1697916977
HANDLE handle = iterator->handle;
1698016978
iterator->handle = INVALID_HANDLE_VALUE;
16981-
FT_MUTEX_UNLOCK(&iterator->mutex);
16979+
PyMutex_Unlock(&iterator->mutex);
1698216980

1698316981
if (handle != INVALID_HANDLE_VALUE) {
1698416982
Py_BEGIN_ALLOW_THREADS
@@ -16996,7 +16994,7 @@ ScandirIterator_iternext(PyObject *op)
1699616994
DWORD error = ERROR_SUCCESS;
1699716995
int found = 0;
1699816996

16999-
FT_MUTEX_LOCK(&iterator->mutex);
16997+
PyMutex_Lock(&iterator->mutex);
1700016998
/* Happens if the iterator is iterated twice, or closed explicitly */
1700116999
while (iterator->handle != INVALID_HANDLE_VALUE) {
1700217000
if (!iterator->first_time) {
@@ -17023,7 +17021,7 @@ ScandirIterator_iternext(PyObject *op)
1702317021

1702417022
/* Loop till we get a non-dot directory or finish iterating */
1702517023
}
17026-
FT_MUTEX_UNLOCK(&iterator->mutex);
17024+
PyMutex_Unlock(&iterator->mutex);
1702717025

1702817026
if (found) {
1702917027
PyObject *module = PyType_GetModule(Py_TYPE(iterator));
@@ -17047,19 +17045,19 @@ ScandirIterator_iternext(PyObject *op)
1704717045
static int
1704817046
ScandirIterator_is_closed(ScandirIterator *iterator)
1704917047
{
17050-
FT_MUTEX_LOCK(&iterator->mutex);
17048+
PyMutex_Lock(&iterator->mutex);
1705117049
int closed = iterator->dirp == NULL;
17052-
FT_MUTEX_UNLOCK(&iterator->mutex);
17050+
PyMutex_Unlock(&iterator->mutex);
1705317051
return closed;
1705417052
}
1705517053

1705617054
static void
1705717055
ScandirIterator_closedir(ScandirIterator *iterator)
1705817056
{
17059-
FT_MUTEX_LOCK(&iterator->mutex);
17057+
PyMutex_Lock(&iterator->mutex);
1706017058
DIR *dirp = iterator->dirp;
1706117059
iterator->dirp = NULL;
17062-
FT_MUTEX_UNLOCK(&iterator->mutex);
17060+
PyMutex_Unlock(&iterator->mutex);
1706317061

1706417062
if (dirp != NULL) {
1706517063
Py_BEGIN_ALLOW_THREADS
@@ -17089,7 +17087,7 @@ ScandirIterator_iternext(PyObject *op)
1708917087
unsigned char d_type = 0;
1709017088
#endif
1709117089

17092-
FT_MUTEX_LOCK(&iterator->mutex);
17090+
PyMutex_Lock(&iterator->mutex);
1709317091
/* Happens if the iterator is iterated twice, or closed explicitly */
1709417092
while (iterator->dirp != NULL) {
1709517093
Py_BEGIN_ALLOW_THREADS
@@ -17126,7 +17124,7 @@ ScandirIterator_iternext(PyObject *op)
1712617124

1712717125
/* Loop till we get a non-dot directory or finish iterating */
1712817126
}
17129-
FT_MUTEX_UNLOCK(&iterator->mutex);
17127+
PyMutex_Unlock(&iterator->mutex);
1713017128

1713117129
if (found) {
1713217130
PyObject *module = PyType_GetModule(Py_TYPE(iterator));
@@ -17286,9 +17284,7 @@ os_scandir_impl(PyObject *module, path_t *path)
1728617284
if (!iterator)
1728717285
return NULL;
1728817286

17289-
#ifdef Py_GIL_DISABLED
1729017287
iterator->mutex = (PyMutex){0};
17291-
#endif
1729217288
#ifdef MS_WINDOWS
1729317289
iterator->handle = INVALID_HANDLE_VALUE;
1729417290
#else

0 commit comments

Comments
 (0)