diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index eb4083bc..a5e033e9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -15,7 +15,7 @@ jobs: strategy: fail-fast: false matrix: - env: ["3.10", 3.11, 3.12, 3.13, 3.14] + env: ["3.10", 3.11, 3.12, 3.13, 3.14, "3.13t", "3.14t"] os: [ubuntu-latest, ubuntu-24.04-arm, macos-latest, windows-latest] steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/HISTORY.rst b/HISTORY.rst index ae86d464..a791dcc8 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -25,6 +25,10 @@ History Reinitializing a ``Metadata`` changes nothing. * Fixed a ``RuntimeWarning`` or ``RuntimeError`` on free-threaded Python on macOS when a ``Reader`` failed to open or was used without ``__init__``. + * Fixed a deadlock on free-threaded Python when a ``Reader`` was closed + during iteration, from another thread or from a signal handler. + * Fixed a crash on free-threaded Python when two threads advanced the same + iterator. 3.2.0 (2026-09-10) ++++++++++++++++++ diff --git a/extension/maxminddb.c b/extension/maxminddb.c index 96679f17..90cbeda0 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -135,6 +135,7 @@ static inline maxminddb_state *get_maxminddb_state_from_self(PyObject *self) { static void reader_close_database(Reader_obj *reader); static bool can_read(const char *path); static int get_record(PyObject *self, PyObject *args, PyObject **record); +static PyObject *reader_iter_next(PyObject *self); static bool format_sockaddr(struct sockaddr *addr, char *dst); static PyObject *from_entry_data_list(maxminddb_state *state, MMDB_entry_data_list_s **entry_data_list); @@ -202,6 +203,13 @@ static void reader_lock_destroy(reader_rwlock_t *lock) { #endif } +// No Python code may run while a thread holds the read lock. On free-threaded +// Python, close() and __init__ wait for the write lock while they stay +// attached to the interpreter. If Python code under the read lock started a +// GC, the stop-the-world pause would wait for the writer, and the writer would +// wait for the read lock, so both would hang. Waiting detached instead lets a +// thread take the lock during a stop-the-world pause, which can hang a forked +// child. static int reader_acquire_read_lock(Reader_obj *reader) { #ifdef MAXMINDDB_USE_WINDOWS_LOCKS AcquireSRWLockShared(&(reader->rwlock)); @@ -814,6 +822,21 @@ static bool is_ipv6(char ip[16]) { } static PyObject *ReaderIter_next(PyObject *self) { + PyObject *result; +#ifdef Py_GIL_DISABLED + // The iterator's list of pending records is not thread-safe, so let only + // one thread at a time advance an iterator. The read lock is shared, so + // it does not do this. + Py_BEGIN_CRITICAL_SECTION(self); +#endif + result = reader_iter_next(self); +#ifdef Py_GIL_DISABLED + Py_END_CRITICAL_SECTION(); +#endif + return result; +} + +static PyObject *reader_iter_next(PyObject *self) { maxminddb_state *state = get_maxminddb_state_from_self((PyObject *)self); if (state == NULL) { return NULL; @@ -907,6 +930,9 @@ static PyObject *ReaderIter_next(PyObject *self) { case MMDB_RECORD_TYPE_EMPTY: break; case MMDB_RECORD_TYPE_DATA: { + // Read this before any Python code runs, which could close + // the reader. + uint16_t const depth = ri->reader->mmdb->depth; MMDB_entry_data_list_s *entry_data_list = NULL; int status = MMDB_get_entry_data_list(&cur->entry, &entry_data_list); @@ -926,15 +952,19 @@ static PyObject *ReaderIter_next(PyObject *self) { PyObject *record = from_entry_data_list(state, &entry_data_list); MMDB_free_entry_data_list(original_entry_data_list); + + // The rest uses only cur, which this call owns. Release the + // lock before ip_network runs Python code, which could close + // the reader on this thread. + reader_release_read_lock(ri->reader); if (record == NULL) { - reader_release_read_lock(ri->reader); free(cur); return NULL; } int ip_start = 0; Py_ssize_t ip_length = 4; - if (ri->reader->mmdb->depth == 128) { + if (depth == 128) { if (is_ipv6(cur->ip_packed)) { // IPv6 address ip_length = 16; @@ -948,37 +978,28 @@ static PyObject *ReaderIter_next(PyObject *self) { &(cur->ip_packed[ip_start]), ip_length, cur->depth - ip_start * 8); + free(cur); if (network_tuple == NULL) { - reader_release_read_lock(ri->reader); Py_DECREF(record); - free(cur); return NULL; } PyObject *args = PyTuple_Pack(1, network_tuple); Py_DECREF(network_tuple); if (args == NULL) { - reader_release_read_lock(ri->reader); Py_DECREF(record); - free(cur); return NULL; } PyObject *network = PyObject_CallObject(state->ipaddress_ip_network, args); Py_DECREF(args); if (network == NULL) { - reader_release_read_lock(ri->reader); Py_DECREF(record); - free(cur); return NULL; } PyObject *rv = PyTuple_Pack(2, network, record); Py_DECREF(network); Py_DECREF(record); - - reader_release_read_lock(ri->reader); - - free(cur); return rv; } default: diff --git a/pyproject.toml b/pyproject.toml index fa1b12b2..c51facb6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -108,6 +108,8 @@ env_list = [ "3.12", "3.13", "3.14", + "3.13t", + "3.14t", "lint", ] skip_missing_interpreters = false @@ -143,6 +145,8 @@ commands = [ ] [tool.tox.gh.python] +"3.14t" = ["3.14t"] +"3.13t" = ["3.13t"] "3.14" = ["3.14", "lint"] "3.13" = ["3.13"] "3.12" = ["3.12"] diff --git a/tests/reader_test.py b/tests/reader_test.py index da91173f..22a82c9e 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -1192,18 +1192,17 @@ def __del__(self): ) self._run_program(program) - def _run_program(self, program: str) -> None: + def _run_program(self, program: str, path: str = _DECODER_DB) -> None: # Put this process's maxminddb first, and keep the harness's paths. paths = [str(pathlib.Path(maxminddb.__file__).parent.parent)] if os.environ.get("PYTHONPATH"): paths.append(os.environ["PYTHONPATH"]) env = {**os.environ, "PYTHONPATH": os.pathsep.join(paths)} - path = pathlib.Path(_DECODER_DB).resolve() with tempfile.TemporaryDirectory() as directory: # Run from an empty directory so the child imports the same # maxminddb as this process, not a source tree in the cwd. result = subprocess.run( # noqa: S603 - [sys.executable, "-c", program, str(path)], + [sys.executable, "-c", program, str(pathlib.Path(path).resolve())], capture_output=True, text=True, check=False, @@ -1271,6 +1270,142 @@ def test_freed_objects_release_their_type(self) -> None: iter(reader) self.assertEqual([sys.getrefcount(c) for c in classes], before) + def test_close_from_ip_network_during_iteration(self) -> None: + # The iterator calls ipaddress.ip_network, which can run Python code + # that closes the reader. If the iterator still held the read lock, + # close() would wait for it forever on free-threaded Python. Run in a + # subprocess with a timeout, and patch ip_network before the extension + # caches it. + program = textwrap.dedent( + """ + import ipaddress + import sys + + real_ip_network = ipaddress.ip_network + + def ip_network(*args, **kwargs): + reader.close() + return real_ip_network(*args, **kwargs) + + ipaddress.ip_network = ip_network + + from maxminddb.extension import Reader + + reader = Reader(sys.argv[1]) + iterator = iter(reader) + # The record was decoded before the close, so this call finishes. + next(iterator) + try: + next(iterator) + except ValueError: + pass + else: + sys.exit("next() after close() did not raise ValueError") + print("ok") + """, + ) + self._run_program(program) + + def test_close_from_another_thread_during_ip_network(self) -> None: + # Thread B calls close() and waits for the write lock while thread A + # is in ip_network. ip_network then starts a GC, which on free-threaded + # Python waits for every thread, B included. If A still held the read + # lock, B would never get the lock, and both would wait forever. + program = textwrap.dedent( + """ + import gc + import ipaddress + import sys + import threading + import time + + real_ip_network = ipaddress.ip_network + closing = threading.Event() + + def ip_network(*args, **kwargs): + if not closing.is_set(): + closing.set() + # Give the other thread time to wait for the write lock. + time.sleep(0.2) + gc.collect() + return real_ip_network(*args, **kwargs) + + ipaddress.ip_network = ip_network + + from maxminddb.extension import Reader + + reader = Reader(sys.argv[1]) + + def close(): + closing.wait() + reader.close() + + closer = threading.Thread(target=close) + closer.start() + next(iter(reader)) + closer.join() + print("ok") + """, + ) + self._run_program(program) + + @unittest.skipIf( + getattr(sys, "_is_gil_enabled", lambda: True)(), + "needs free-threaded Python", + ) + def test_threads_can_share_an_iterator(self) -> None: + # Run in a subprocess, so heap corruption or a hang fails only this + # test. + program = textwrap.dedent( + """ + import sys + import threading + + from maxminddb.extension import Reader + + reader = Reader(sys.argv[1]) + expected = sorted(str(network) for network, _ in reader) + + def collect(iterator, barrier, networks, errors): + try: + barrier.wait() + # Each thread appends to its own list. With one shared + # list, list.extend would hold the list's critical + # section, and only one thread would call next() at a + # time. + for network, _ in iterator: + networks.append(str(network)) + except BaseException as e: + errors.append(e) + + # A race corrupts the heap only some of the time, so repeat. + for _ in range(5): + iterator = iter(reader) + barrier = threading.Barrier(8, timeout=60) + results = [[] for _ in range(8)] + errors = [] + threads = [ + threading.Thread( + target=collect, + args=(iterator, barrier, networks, errors), + ) + for networks in results + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + if errors: + sys.exit(f"a thread failed: {errors!r}") + # Each network comes out once, with none lost or repeated. + networks = sorted(n for result in results for n in result) + if networks != expected: + sys.exit("networks were lost or repeated") + print("ok") + """, + ) + self._run_program(program, f"{_TEST_DATA_DIR}/GeoIP2-City-Test.mmdb") + def test_iterator_type_is_not_instantiable(self) -> None: with maxminddb.extension.Reader(_DECODER_DB) as reader: iterator_class = type(iter(reader))