From e1f76858bd1e47ca346bb0699ff2b8668f722ab0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 19:42:38 +0000 Subject: [PATCH 01/16] Investigate free-threaded Python (PEP 703) support mode is pure Python, so it already installs, imports and passes its full test suite (757 passed, 2 skipped) on CPython 3.14.0rc2 free-threaded with the GIL disabled -- no packaging work is required. What free threading changes is that several latent thread-safety defects stop being theoretical. Measured on python3.14t with a GIL-enabled 3.14.0rc2 as the control: - LRUCache is backed by collections.OrderedDict with thread_safety=False by default, so concurrent __setitem__ eviction and keys() iteration run unlocked. This SEGFAULTS a free-threaded interpreter (4/5 runs, plus a hang); the GIL build is unaffected. Isolated to OrderedDict itself -- the same loop against a plain dict survives every run, because free-threaded CPython gives plain dict per-object locking and OrderedDict's C implementation did not get the same treatment. - cached_property.__get__ is a non-atomic check-then-act on obj.__dict__: 104/300 trials handed out more than one distinct object (0/300 on the GIL build). ServiceProxy documents @cached_property as the way to build the proxied service, and 198/200 trials built more than one Service instance, so a start() and a later stop() can reach different objects. - mode/__init__.py swaps sys.modules["mode"] for a _module instance at the end of its body, so a thread importing mode concurrently can receive the pre-swap module and AttributeError on every lazily-exported name. Pre-existing, but 14/25 runs fail free-threaded vs 3/25 under the GIL. PEP 562 module __getattr__ removes the swap entirely. - Signal iterates its receiver set while connect/disconnect mutate it. Pre-existing, not a free-threading regression: 30/30 trials raise on both builds. - mode[gevent] re-enables the GIL at import (gevent.libev.corecext is not declared free-threading safe). mode[uvloop] and mode[eventlet] leave it disabled. Adds docs/free-threading.md with the full analysis and a suggested order of work, and tests/freethreading/stress.py with the reproducers. The latter sits outside the testpaths configured in pyproject.toml so the crash reproducers are never collected by a normal pytest run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- docs/free-threading.md | 255 +++++++++++++++++++++ mkdocs.yml | 1 + tests/freethreading/stress.py | 416 ++++++++++++++++++++++++++++++++++ 3 files changed, 672 insertions(+) create mode 100644 docs/free-threading.md create mode 100644 tests/freethreading/stress.py diff --git a/docs/free-threading.md b/docs/free-threading.md new file mode 100644 index 0000000..81bed80 --- /dev/null +++ b/docs/free-threading.md @@ -0,0 +1,255 @@ +# Free-threaded Python (PEP 703) support + +Status of `mode` on free-threaded ("no-GIL") CPython builds, and what +remains to be done. + +Everything below was measured on **CPython 3.14.0rc2 free-threading build** +(`python3.14t`, `sys._is_gil_enabled() == False`), with a GIL-enabled +CPython 3.14.0rc2 used as the control. The reproducers live in +`tests/freethreading/stress.py`. + +## Summary + +`mode` is pure Python, so there is nothing to port: it installs, imports +and passes its whole test suite on a free-threaded interpreter today. What +free threading changes is that three latent thread-safety defects stop +being theoretical. One of them crashes the interpreter. + +| | Free-threaded | GIL | +|---|---|---| +| `pip install mode-streaming` | works (`py3-none-any`) | works | +| Import every `mode` module | GIL stays disabled | n/a | +| `pytest tests/unit tests/functional` | 757 passed, 2 skipped | 757 passed, 2 skipped | +| `LRUCache` under 16 threads | **SIGSEGV** | fine | +| `cached_property` under 16 threads | **duplicate objects** | fine | +| concurrent first `import mode` | fails 14/25 runs | fails 3/25 runs | +| `Signal` under 16 threads | raises | raises (pre-existing) | +| `mode[uvloop]` | GIL stays disabled | n/a | +| `mode[gevent]` | **GIL re-enabled** | n/a | + +## What already works + +No packaging work is required. `mode` ships no C extensions, so the +existing `py3-none-any` wheel already installs and runs on `3.13t`/`3.14t`. +Importing every module in the package leaves the GIL disabled, and the core +dependencies (`colorlog`, `croniter`, `mypy_extensions`) are pure Python. +The full test suite passes unmodified. + +These were stress-tested with 16 concurrent OS threads and found **safe**: + +- `Service` subclass creation — `__init_subclass__` writing the shared + `cls._tasks` mapping (`mode/services.py:527-553`) +- `ServiceThread` start/stop from many threads concurrently +- `get_event_loop()` — the `threading.local` cache in + `mode/utils/loops.py:15` correctly gives each thread its own loop with no + cross-thread leakage +- `Node`/beacon tree traversal concurrent with mutation +- `ManagedUserDict` / `FastUserDict` mutation +- `annotations()` / `eval_type()` +- `LocalStack` — already `ContextVar`-based, so correct by construction + +## Findings + +### 1. `LRUCache` can segfault the interpreter — free-threading-specific + +**Severity: critical.** + +`LRUCache.data` is a `collections.OrderedDict` and `thread_safety` defaults +to `False`, which makes `self._mutex` a `nullcontext` +(`mode/utils/collections.py:449-455`, `:523-526`). So `__setitem__` — +which evicts via `self.data.pop(next(iter(self.data)))` +(`mode/utils/collections.py:474-479`) — and `keys()`, which iterates the +same dict (`mode/utils/collections.py:489-491`), run with no lock at all. + +Under the GIL this is benign: 0/20 stress trials raised. On `3.14t` the +same code first raises `RuntimeError: OrderedDict changed size during +iteration` and then **segfaults**: 4 of 5 runs of a 60-trial loop exited +with SIGSEGV, and a 5th hung. + +The cause was isolated to `OrderedDict` itself. Repeating the identical +concurrent mutate-and-iterate loop against a bare container: + +| container | free-threaded 3.14t | +|---|---| +| `collections.OrderedDict` | SIGSEGV / SIGABRT, 3/3 runs | +| plain `dict` | survives, 3/3 runs | + +Free-threaded CPython gives plain `dict` per-object locking; `OrderedDict`'s +C implementation did not get the same treatment, so concurrent mutation +corrupts its internal linked list. + +Two independent fixes, either of which is sufficient: + +- Back `LRUCache` with a plain `dict`. Insertion order has been guaranteed + since 3.7, and the only `OrderedDict`-specific API used is + `popitem(last=...)`, which maps to `d.popitem()` for `last=True` and + `d.pop(next(iter(d)))` for `last=False`. +- Default `thread_safety=True` on free-threaded builds. The existing mutex + path is sound — `LRUCache(thread_safety=True)` passed the stress test + cleanly — it is just off by default. + +`LRUCache` is not used inside `mode` itself; it is exported utility surface +(faust is a consumer), so the blast radius is downstream. + +### 2. `cached_property` hands different objects to different threads — free-threading-specific + +**Severity: high.** + +`cached_property.__get__` (`mode/utils/objects.py:685-694`) is a +check-then-act on `obj.__dict__`: try the key, catch `KeyError`, compute, +store. Nothing makes that atomic. + +| | duplicate-object trials | computes per 300 properties | +|---|---|---| +| GIL 3.14 | 0/300 | 300 | +| free-threaded 3.14t | **104/300** | 419 | + +This is not merely wasted work. `ServiceProxy` documents +`@cached_property _service` as *the* way to build the proxied service +(`mode/proxy.py:17-35`) — it is how the Faust App is constructed at module +level. A reproducer that races 16 threads on `proxy._service`: + +| | trials that built/returned >1 `Service` | +|---|---| +| GIL 3.14 | 0/200 | +| free-threaded 3.14t | **198/200** | + +So one thread can `start()` one `Service` instance while another thread +holds a different instance, and the later `stop()` never reaches the one +that was started. + +Note that stdlib `functools.cached_property` deliberately dropped its lock +in 3.12 and accepts duplicate computation. That trade-off is fine for a +pure value cache; it is not fine for a singleton service handle. The fix is +double-checked locking in `cached_property.__get__` (a per-instance or +per-descriptor lock), or failing that, making `ServiceProxy._service` +guard itself. + +### 3. Concurrent first `import mode` can hand back a half-built module — pre-existing, much worse under free threading + +**Severity: high.** This one breaks the most ordinary thing a user does. + +`mode/__init__.py` uses the Werkzeug lazy-import trick: it defines a +`_module` subclass with a `__getattr__` that resolves the lazily-exported +names, then swaps it into `sys.modules` at the *end* of the module body +(`mode/__init__.py:88-129`): + +```python +new_module = sys.modules[__name__] = _module(__name__) +new_module.__dict__.update({"__file__": ..., "__path__": ..., ...}) +``` + +If thread B runs `import mode` while thread A is still executing +`mode/__init__.py`, B can be handed the original, pre-swap module object — +which has no `__getattr__` yet — so every lazily-exported name raises: + +``` +AttributeError: module 'mode' has no attribute 'Service' +``` + +Racing 16 threads on a cold `import mode` followed by attribute access: + +| | runs with at least one failing thread | +|---|---| +| GIL 3.14 | 3/25 | +| free-threaded 3.14t | **14/25** | + +Instrumenting a failing thread confirms the mechanism: the object it +imported is a plain `module` (`type(mode).__name__ == "module"`) while +`sys.modules["mode"]` is already the `_module` instance — the thread holds +the stale pre-swap object. The replacement module also carries **no +`__spec__`** (`sys.modules["mode"].__spec__ is None`), which is what +deprives the import machinery of the `_initializing` flag it would +otherwise use to make the second thread wait. + +The fix is to drop the `sys.modules` swap entirely and use a PEP 562 +module-level `__getattr__`, which needs no module replacement and is +therefore race-free. PEP 562 landed in 3.7 and mode's floor is 3.10, so the +`_module` class exists only for compatibility that is no longer needed: + +```python +def __getattr__(name: str) -> Any: + if name in object_origins: + module = __import__(object_origins[name], None, None, [name]) + return getattr(module, name) + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") +``` + +### 4. `Signal` mutates its receiver set during iteration — pre-existing + +**Severity: medium. Not a free-threading regression.** + +`_get_live_receivers` iterates `self._receivers` (a plain `set`) +(`mode/signals.py:157-167`) while `connect`/`disconnect` add and discard on +it (`mode/signals.py:120`, `:132`). Racing those raises +`RuntimeError: Set changed size during iteration` in **30/30 trials on both +builds** — so `Signal` has never been thread-safe. Free threading only +makes concurrent use likely enough to hit it in practice. + +Fix: iterate a snapshot, e.g. `for href in tuple(r):`. + +### 5. The `gevent` extra re-enables the GIL — packaging + +| extra | result on `3.14t` | +|---|---| +| `mode[uvloop]` | uvloop 0.22.1 imports and runs, GIL stays disabled | +| `mode[eventlet]` | imports, GIL stays disabled (eventlet prints its own migrate-away notice) | +| `mode[gevent]` | **GIL re-enabled at import** | + +Installing `mode[gevent]` silently downgrades a free-threaded interpreter +back to GIL semantics: + +``` +RuntimeWarning: The global interpreter lock (GIL) has been enabled to load +module 'gevent.libev.corecext', which has not declared that it can run +safely without the GIL. +``` + +This is upstream in gevent, not something `mode` can fix — it should be +documented as an unsupported combination. + +## Suggested order of work + +1. Fix `LRUCache` (finding 1) — it is an interpreter crash. +2. Fix `cached_property` (finding 2) — silent correctness bug for + `ServiceProxy`, and therefore for faust. +3. Convert `mode/__init__.py` to a PEP 562 module `__getattr__` + (finding 3) — breaks plain `import mode`, and is a real bug under the + GIL too. +4. Snapshot the `Signal` receiver set (finding 4) — cheap, and also + pre-existing. +5. Add `3.14t` to the `tests.yml` matrix. `actions/setup-python` accepts + the `3.14t` version string directly. +6. Add a trove classifier once 1-4 land: + `Programming Language :: Python :: Free Threading :: 2 - Beta` + (the `Free Threading :: N - ...` classifiers are registered in + `trove-classifiers`). +7. Document `mode[gevent]` as incompatible with free-threaded builds. + +### A note on `pytest-run-parallel` + +`pytest-run-parallel` installs and runs on `3.14t`, but pointing +`--parallel-threads` at the existing suite is not useful: it reports ~33 +failures in `tests/functional/utils/test_collections.py` alone that are +artifacts of tests sharing mutable fixtures and `Mock` objects, not +mode bugs. For example +`test_AttributeDictMixin::test_set_get` fails with "DID NOT RAISE +AttributeError" purely because a sibling thread already set the attribute +on the shared object. + +Use it selectively on purpose-written thread-safety tests rather than +across the whole suite. + +## Reproducing + +```sh +uv python install 3.14t +uv venv --python 3.14t .venv-ft +VIRTUAL_ENV=.venv-ft uv pip install -e . -r requirements-tests.txt +.venv-ft/bin/python tests/freethreading/stress.py +``` + +`tests/freethreading/` is deliberately outside the `testpaths` configured +in `pyproject.toml`, so the crash reproducers are never collected by a +normal `pytest` run. Run the same file under a GIL-enabled interpreter to +see the control numbers. diff --git a/mkdocs.yml b/mkdocs.yml index 84d2b1f..23b43ca 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -39,6 +39,7 @@ nav: - Web app: example-webapp.md - Developing: - Contributing Guide: contributing.md + - Free-threaded Python: free-threading.md - References: - 'Mode': - mode.services: references/mode.services.md diff --git a/tests/freethreading/stress.py b/tests/freethreading/stress.py new file mode 100644 index 0000000..b713689 --- /dev/null +++ b/tests/freethreading/stress.py @@ -0,0 +1,416 @@ +"""Free-threading (PEP 703) stress reproducers for mode. + +This file is intentionally NOT under the ``testpaths`` configured in +``pyproject.toml``, because some of the checks below can segfault a +free-threaded interpreter by design -- that is the finding, not a bug in +the harness. Run it directly: + +```sh +uv python install 3.14t +uv venv --python 3.14t .venv-ft +VIRTUAL_ENV=.venv-ft uv pip install -e . -r requirements-tests.txt +.venv-ft/bin/python tests/freethreading/stress.py +``` + +Run it again under a GIL-enabled interpreter of the same version to get +the control numbers -- most of these checks pass there, which is what +makes them free-threading findings rather than plain bugs. + +See `docs/free-threading.md` for the measured results and analysis. +""" + +import sys +import threading +import traceback + +NTHREADS = 16 + + +def race(target, nthreads=NTHREADS): + """Run ``target(i)`` in ``nthreads`` threads released by a barrier. + + Returns the list of tracebacks raised by the threads (empty if none). + """ + barrier = threading.Barrier(nthreads) + errors = [] + + def wrapper(i): + barrier.wait() + try: + target(i) + except BaseException: + errors.append(traceback.format_exc()) + + threads = [ + threading.Thread(target=wrapper, args=(i,)) for i in range(nthreads) + ] + for t in threads: + t.start() + for t in threads: + t.join() + return errors + + +def report(name, errors, note=""): + if errors: + last_line = errors[0].strip().splitlines()[-1] + print(f"[FAIL] {name}: {len(errors)} threads -> {last_line}") + else: + print(f"[ok ] {name} {note}".rstrip()) + return bool(errors) + + +# -------------------------------------------------------------------------- +# Finding 1: LRUCache is backed by OrderedDict with thread_safety=False by +# default. Concurrent mutate+iterate segfaults a free-threaded interpreter +# (plain dict is safe there; OrderedDict's C implementation is not). +# -------------------------------------------------------------------------- +def check_lru_default(trials=60): + from mode.utils.collections import LRUCache + + print(" (this check can segfault on a free-threaded build)", flush=True) + bad = 0 + for _ in range(trials): + cache = LRUCache(limit=50) + + def work(i, cache=cache): + for n in range(100): + cache[f"{i}-{n}"] = n + list(cache.keys()) + + if race(work): + bad += 1 + print( + f"[{'FAIL' if bad else 'ok '}] LRUCache(default): " + f"{bad}/{trials} trials raised" + ) + + +def check_lru_thread_safe(trials=20): + from mode.utils.collections import LRUCache + + bad = 0 + for _ in range(trials): + cache = LRUCache(limit=50, thread_safety=True) + + def work(i, cache=cache): + for n in range(100): + cache[f"{i}-{n}"] = n + list(cache.keys()) + list(cache.items()) + + if race(work): + bad += 1 + print( + f"[{'FAIL' if bad else 'ok '}] LRUCache(thread_safety=True): " + f"{bad}/{trials} trials raised" + ) + + +# -------------------------------------------------------------------------- +# Finding 2: cached_property.__get__ is a non-atomic check-then-act on +# obj.__dict__, so racing threads can each compute and hand out a distinct +# object. ServiceProxy documents @cached_property as the way to build the +# proxied service, so the duplicate is a real singleton violation. +# -------------------------------------------------------------------------- +def check_cached_property(trials=300): + from mode.utils.objects import cached_property + + computes = [0] + bad = 0 + for _ in range(trials): + + class X: + @cached_property + def val(self): + computes[0] += 1 + return object() + + x = X() + seen = [] + lock = threading.Lock() + + def work(i, x=x, seen=seen, lock=lock): + value = x.val + with lock: + seen.append(value) + + race(work) + if len({id(v) for v in seen}) != 1: + bad += 1 + print( + f"[{'FAIL' if bad else 'ok '}] cached_property: {bad}/{trials} " + f"trials returned >1 distinct object " + f"({computes[0]} computes for {trials} properties)" + ) + + +def check_service_proxy(trials=200): + from mode import Service + from mode.proxy import ServiceProxy + from mode.utils.objects import cached_property + + bad = 0 + for _ in range(trials): + built = [] + build_lock = threading.Lock() + + class MyProxy(ServiceProxy): + @cached_property + def _service(self, built=built, build_lock=build_lock): + service = Service() + with build_lock: + built.append(service) + return service + + proxy = MyProxy() + seen = [] + seen_lock = threading.Lock() + + def work(i, proxy=proxy, seen=seen, seen_lock=seen_lock): + service = proxy._service + with seen_lock: + seen.append(service) + + race(work) + if len({id(s) for s in seen}) != 1 or len(built) != 1: + bad += 1 + print( + f"[{'FAIL' if bad else 'ok '}] ServiceProxy._service: " + f"{bad}/{trials} trials built/returned >1 Service instance" + ) + + +# -------------------------------------------------------------------------- +# Finding 3: Signal iterates its receiver set while connect/disconnect +# mutate it. Pre-existing -- this fails on GIL builds too. +# -------------------------------------------------------------------------- +def check_signal(trials=30): + from mode.signals import Signal + + bad = 0 + for _ in range(trials): + + class Owner: + sig = Signal() + + owner = Owner() + sig = Owner.sig + + def work(i, sig=sig, owner=owner): + for _n in range(100): + + async def handler(*args, **kwargs): + pass + + if i % 2: + sig.connect(handler) + sig.disconnect(handler) + else: + list(sig.iter_receivers(owner)) + + if race(work): + bad += 1 + print( + f"[{'FAIL' if bad else 'ok '}] Signal iter_receivers: " + f"{bad}/{trials} trials raised" + ) + + +# -------------------------------------------------------------------------- +# Surfaces verified SAFE under the same stress -- kept so regressions show up. +# -------------------------------------------------------------------------- +def check_service_subclass_creation(): + from mode import Service + + made = [] + lock = threading.Lock() + + def work(i): + local = [] + for n in range(50): + + async def a_task(self): + pass + + namespace = { + "__module__": f"stressmod{i}", + "__qualname__": f"S{i}_{n}", + "t": Service.task(a_task), + } + local.append(type(f"S{i}_{n}", (Service,), namespace)) + with lock: + made.extend(local) + + errors = race(work) + if not errors: + for cls in made: + clsid = cls._get_class_id() + if cls._tasks.get(clsid) != {"t"}: + errors.append(f"{clsid} -> {cls._tasks.get(clsid)!r}") + report( + "Service subclass creation (cls._tasks)", + errors, + f"({len(made)} classes)", + ) + + +def check_get_event_loop(): + import asyncio + + from mode.utils.loops import get_event_loop + + seen = {} + lock = threading.Lock() + + def work(i): + loops = {get_event_loop() for _ in range(200)} + assert len(loops) == 1, f"thread saw {len(loops)} loops" + with lock: + seen[threading.get_ident()] = loops.pop() + + errors = race(work) + ids = [id(v) for v in seen.values()] + if len(set(ids)) != len(ids): + errors.append("event loop leaked across threads") + for loop in seen.values(): + loop.close() + asyncio.set_event_loop(None) + report("get_event_loop() thread-local cache", errors) + + +def check_service_thread(): + import asyncio + + from mode.threads import ServiceThread + + class T(ServiceThread): + pass + + def work(i): + async def main(): + service = T() + await service.start() + await service.stop() + + asyncio.run(main()) + + report("ServiceThread start/stop", race(work, nthreads=8)) + + +def check_beacon(): + from mode.utils.trees import Node + + root = Node("root") + for i in range(50): + root.new(f"pre-{i}") + + def work(i): + for n in range(200): + if i % 2: + child = root.new(f"{i}-{n}") + root.discard(child.data) + else: + list(root.traverse()) + root.as_graph() + + report("Node.traverse while mutating", race(work)) + + +def check_managed_user_dict(): + from mode.utils.collections import ManagedUserDict + + class D(ManagedUserDict): + def __init__(self): + self.data = {} + + def on_key_get(self, key): ... + def on_key_set(self, key, value): ... + def on_key_del(self, key): ... + def on_clear(self): ... + + d = D() + + def work(i): + for n in range(400): + d[f"{i}-{n}"] = n + d.get(f"{i}-{n}") + del d[f"{i}-{n}"] + + errors = race(work) + if not errors and len(d): + errors.append(f"{len(d)} leftover keys") + report("ManagedUserDict mutation", errors) + + +# -------------------------------------------------------------------------- +# Finding 3: mode/__init__.py swaps sys.modules["mode"] for a _module +# instance at the END of its body, so a thread importing mode concurrently +# can be handed the original pre-swap module -- which has no __getattr__ -- +# and every lazily-exported name raises AttributeError. Pre-existing, but +# far more likely with the GIL disabled. +# +# Must run in a subprocess: the race only exists on a *cold* import. +# -------------------------------------------------------------------------- +def check_lazy_module(trials=25): + import subprocess + + code = """ +import threading, traceback +errors = [] +barrier = threading.Barrier(16) +names = ["Service", "Worker", "Signal", "Seconds", "get_logger", + "SupervisorStrategy", "label", "want_seconds"] +def work(): + barrier.wait() + try: + import mode + for name in names: + getattr(mode, name) + except BaseException: + errors.append(traceback.format_exc()) +threads = [threading.Thread(target=work) for _ in range(16)] +[t.start() for t in threads] +[t.join() for t in threads] +if errors: + print(errors[0]) + raise SystemExit(1) +""" + bad = 0 + first = "" + for _ in range(trials): + proc = subprocess.run( + [sys.executable, "-c", code], capture_output=True, text=True + ) + if proc.returncode: + bad += 1 + first = first or proc.stdout.strip().splitlines()[-1] + print( + f"[{'FAIL' if bad else 'ok '}] concurrent cold `import mode`: " + f"{bad}/{trials} runs had a failing thread" + + (f" -> {first}" if first else "") + ) + + +def main(): + print(f"python: {sys.version.splitlines()[0]}") + print(f"GIL enabled: {sys._is_gil_enabled()}\n") + + print("-- surfaces verified safe --") + check_service_subclass_creation() + check_get_event_loop() + check_service_thread() + check_beacon() + check_managed_user_dict() + check_lru_thread_safe() + + print("\n-- findings --") + check_lazy_module() + check_signal() + check_cached_property() + check_service_proxy() + check_lru_default() + + +if __name__ == "__main__": + main() From 667f6e598d259881a67d4e78e3fe329cbf27334d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 19:43:01 +0000 Subject: [PATCH 02/16] Note that the measured failure rates vary between runs These are races, so the rates quoted in the findings are representative single runs rather than stable constants. Record the observed ranges so a reader who reproduces them and sees different numbers knows that is expected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- docs/free-threading.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/free-threading.md b/docs/free-threading.md index 81bed80..b52a03b 100644 --- a/docs/free-threading.md +++ b/docs/free-threading.md @@ -8,6 +8,12 @@ Everything below was measured on **CPython 3.14.0rc2 free-threading build** CPython 3.14.0rc2 used as the control. The reproducers live in `tests/freethreading/stress.py`. +These are races, so the failure *rates* quoted below move around between +runs — the numbers are representative single runs, not stable constants. +On repeated runs the free-threaded `cached_property` figure ranged from +104/300 to 164/300, and the cold-import figure from 14/25 to 18/25. What +does not move is which side of the table fails. + ## Summary `mode` is pure Python, so there is nothing to port: it installs, imports From ad8193e5290e604ba37d4d4f0114d77ca748d0db Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 20:29:51 +0000 Subject: [PATCH 03/16] Fix the thread-safety defects free threading exposed Implements the four fixes from docs/free-threading.md. Verified on CPython 3.14.0rc2 free-threaded, GIL-enabled 3.14.0rc2, and 3.10 (the floor): 778 passing, ruff and mypy clean on all three. LRUCache could segfault the interpreter. It was backed by an OrderedDict with thread_safety=False by default, so eviction and iteration ran unlocked; concurrent mutate+iterate exited SIGSEGV in 4 of 5 runs on a free-threaded build. The cause was isolated to OrderedDict -- free-threaded CPython gives plain dict per-object locking but did not convert OrderedDict's C linked list -- so the backing store is a plain dict now (insertion-ordered since 3.7; the only OrderedDict-specific API in use was popitem(last=...)). thread_safety additionally defaults to on for free-threaded builds via the new FREE_THREADED flag, checked at runtime so PYTHON_GIL=1 is respected. Iteration now snapshots under the mutex instead of holding it across yields, which would otherwise have kept the lock held for as long as the consumer took to iterate -- and forever if it abandoned the generator. cached_property handed different objects to different threads: __get__ was a non-atomic check-then-act on obj.__dict__, and 104/300 trials returned more than one distinct object. ServiceProxy documents @cached_property _service as the way to build the proxied service, and 198/200 trials built more than one Service, so start() and stop() could act on different instances. The miss path is double-checked under a per-descriptor lock now; the already-cached path stays lock-free. Concurrent cold `import mode` could hand back a half-built module. mode/__init__.py swapped sys.modules["mode"] for a ModuleType subclass at the end of its body, so a thread importing concurrently could receive the pre-swap object -- which has no __getattr__ -- and AttributeError on every lazily-exported name (14/25 runs free-threaded, 3/25 under the GIL). The replacement also carried no __spec__, denying the import machinery the _initializing flag that would have made the second thread wait. Replaced with a PEP 562 module __getattr__ plus module __dir__, so there is no swap to race with. The exported surface is unchanged: same 25 names, same identities, star-import and dir() equivalent. The one visible difference is that mode.__all__ is now the list declared in the source rather than a tuple the swap substituted. Signal iterated its receiver set while connect/disconnect mutated it, raising "Set changed size during iteration" 30/30 on both builds -- a pre-existing bug, not a free-threading regression. It snapshots now, with list() rather than tuple(): list()/set()/set.copy() take the source set's per-object lock for the copy, while tuple() falls back to generic iteration and does not. The first attempt used tuple() and still failed 8/8; the stress harness caught it. Also adds tests/functional/test_thread_safety.py (21 tests, every one verified to fail against the pre-fix tree), puts 3.14t in the CI matrix, declares the Free Threading :: 2 - Beta classifier, and notes next to the gevent extra that gevent.libev.corecext re-enables the GIL -- the one item here that cannot be fixed from this side. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- .github/workflows/tests.yml | 3 + docs/free-threading.md | 313 +++++++++++++------------ mode/__init__.py | 94 ++++---- mode/signals.py | 14 +- mode/utils/collections.py | 80 +++++-- mode/utils/objects.py | 23 +- pyproject.toml | 4 + tests/freethreading/stress.py | 56 +++-- tests/functional/test_thread_safety.py | 296 +++++++++++++++++++++++ 9 files changed, 635 insertions(+), 248 deletions(-) create mode 100644 tests/functional/test_thread_safety.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 45844ee..a14fcd3 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -25,6 +25,9 @@ jobs: - "3.12" - "3.13" - "3.14" + # Free-threaded (PEP 703) build. Runs the same suite with the + # GIL disabled; see docs/free-threading.md. + - "3.14t" experimental: [ false ] steps: diff --git a/docs/free-threading.md b/docs/free-threading.md index b52a03b..0e7b236 100644 --- a/docs/free-threading.md +++ b/docs/free-threading.md @@ -1,79 +1,83 @@ # Free-threaded Python (PEP 703) support -Status of `mode` on free-threaded ("no-GIL") CPython builds, and what -remains to be done. +`mode` supports free-threaded ("no-GIL") CPython. This page records what +was wrong before that was true, how each defect was fixed, and how to +re-check the work. -Everything below was measured on **CPython 3.14.0rc2 free-threading build** +Everything here was measured on **CPython 3.14.0rc2 free-threading build** (`python3.14t`, `sys._is_gil_enabled() == False`), with a GIL-enabled CPython 3.14.0rc2 used as the control. The reproducers live in -`tests/freethreading/stress.py`. - -These are races, so the failure *rates* quoted below move around between -runs — the numbers are representative single runs, not stable constants. -On repeated runs the free-threaded `cached_property` figure ranged from -104/300 to 164/300, and the cold-import figure from 14/25 to 18/25. What -does not move is which side of the table fails. - -## Summary - -`mode` is pure Python, so there is nothing to port: it installs, imports -and passes its whole test suite on a free-threaded interpreter today. What -free threading changes is that three latent thread-safety defects stop -being theoretical. One of them crashes the interpreter. - -| | Free-threaded | GIL | -|---|---|---| -| `pip install mode-streaming` | works (`py3-none-any`) | works | -| Import every `mode` module | GIL stays disabled | n/a | -| `pytest tests/unit tests/functional` | 757 passed, 2 skipped | 757 passed, 2 skipped | -| `LRUCache` under 16 threads | **SIGSEGV** | fine | -| `cached_property` under 16 threads | **duplicate objects** | fine | -| concurrent first `import mode` | fails 14/25 runs | fails 3/25 runs | -| `Signal` under 16 threads | raises | raises (pre-existing) | -| `mode[uvloop]` | GIL stays disabled | n/a | -| `mode[gevent]` | **GIL re-enabled** | n/a | - -## What already works - -No packaging work is required. `mode` ships no C extensions, so the +`tests/freethreading/stress.py`; the regression tests that keep the fixes +honest live in `tests/functional/test_thread_safety.py` and run on every +leg of the CI matrix. + +The "before" numbers are races, so the failure *rates* move between runs — +they are representative single runs, not stable constants. On repeated runs +the free-threaded `cached_property` figure ranged from 104/300 to 164/300, +and the cold-import figure from 14/25 to 18/25. What did not move is which +side of each table failed. + +## Status + +`mode` is pure Python, so there was never anything to *port* — it installed, +imported and passed its test suite on a free-threaded interpreter from the +start. What free threading changed is that four latent thread-safety defects +stopped being theoretical. One of them crashed the interpreter. + +All four are fixed. + +| | Free-threaded (before) | Free-threaded (after) | GIL | +|---|---|---|---| +| `pip install mode-streaming` | works (`py3-none-any`) | works | works | +| Import every `mode` module | GIL stays disabled | GIL stays disabled | n/a | +| `pytest tests/unit tests/functional` | passes | passes | passes | +| `LRUCache` under 16 threads | **SIGSEGV** | clean | clean | +| `cached_property` under 16 threads | **duplicate objects** | one object | one object | +| concurrent cold `import mode` | fails 14/25 runs | 0/25 | 0/25 | +| `Signal` under 16 threads | raises 30/30 | 0/30 | 0/30 | +| `mode[uvloop]` | GIL stays disabled | GIL stays disabled | n/a | +| `mode[gevent]` | **GIL re-enabled** | **GIL re-enabled** | n/a | + +`mode[gevent]` is the one item that is not fixed, because it cannot be +fixed here — see below. + +## What already worked + +No packaging work was required. `mode` ships no C extensions, so the existing `py3-none-any` wheel already installs and runs on `3.13t`/`3.14t`. Importing every module in the package leaves the GIL disabled, and the core dependencies (`colorlog`, `croniter`, `mypy_extensions`) are pure Python. -The full test suite passes unmodified. -These were stress-tested with 16 concurrent OS threads and found **safe**: +These were stress-tested with 16 concurrent OS threads and found **safe** +as they stood: - `Service` subclass creation — `__init_subclass__` writing the shared - `cls._tasks` mapping (`mode/services.py:527-553`) + `cls._tasks` mapping (`mode/services.py`) - `ServiceThread` start/stop from many threads concurrently -- `get_event_loop()` — the `threading.local` cache in - `mode/utils/loops.py:15` correctly gives each thread its own loop with no - cross-thread leakage +- `get_event_loop()` — the `threading.local` cache in `mode/utils/loops.py` + correctly gives each thread its own loop with no cross-thread leakage - `Node`/beacon tree traversal concurrent with mutation - `ManagedUserDict` / `FastUserDict` mutation - `annotations()` / `eval_type()` - `LocalStack` — already `ContextVar`-based, so correct by construction -## Findings +## The four defects, and their fixes -### 1. `LRUCache` can segfault the interpreter — free-threading-specific +### 1. `LRUCache` could segfault the interpreter -**Severity: critical.** +**Was: critical. Free-threading-specific.** -`LRUCache.data` is a `collections.OrderedDict` and `thread_safety` defaults -to `False`, which makes `self._mutex` a `nullcontext` -(`mode/utils/collections.py:449-455`, `:523-526`). So `__setitem__` — -which evicts via `self.data.pop(next(iter(self.data)))` -(`mode/utils/collections.py:474-479`) — and `keys()`, which iterates the -same dict (`mode/utils/collections.py:489-491`), run with no lock at all. +`LRUCache.data` was a `collections.OrderedDict` and `thread_safety` +defaulted to `False`, which made the mutex a `nullcontext`. So eviction in +`__setitem__` and iteration in `keys()` ran with no lock at all. -Under the GIL this is benign: 0/20 stress trials raised. On `3.14t` the -same code first raises `RuntimeError: OrderedDict changed size during -iteration` and then **segfaults**: 4 of 5 runs of a 60-trial loop exited +Under the GIL this was benign: 0/20 stress trials raised. On `3.14t` the +same code first raised `RuntimeError: OrderedDict changed size during +iteration` and then **segfaulted** — 4 of 5 runs of a 60-trial loop exited with SIGSEGV, and a 5th hung. -The cause was isolated to `OrderedDict` itself. Repeating the identical -concurrent mutate-and-iterate loop against a bare container: +The cause was `OrderedDict` itself. Repeating the identical concurrent +mutate-and-iterate loop against a bare container: | container | free-threaded 3.14t | |---|---| @@ -84,70 +88,73 @@ Free-threaded CPython gives plain `dict` per-object locking; `OrderedDict`'s C implementation did not get the same treatment, so concurrent mutation corrupts its internal linked list. -Two independent fixes, either of which is sufficient: - -- Back `LRUCache` with a plain `dict`. Insertion order has been guaranteed - since 3.7, and the only `OrderedDict`-specific API used is - `popitem(last=...)`, which maps to `d.popitem()` for `last=True` and - `d.pop(next(iter(d)))` for `last=False`. -- Default `thread_safety=True` on free-threaded builds. The existing mutex - path is sound — `LRUCache(thread_safety=True)` passed the stress test - cleanly — it is just off by default. +**Fixed** in `mode/utils/collections.py` by all three of: + +- Backing the cache with a plain `dict`. Insertion order has been + guaranteed since 3.7, and the only `OrderedDict`-specific API in use was + `popitem(last=...)`, now served by `_popitem_first()` plus + `dict.popitem()`. +- Defaulting `thread_safety` to `True` on free-threaded builds, via the new + `mode.utils.collections.FREE_THREADED` flag. It is checked at runtime + rather than build time, so `PYTHON_GIL=1` is respected. Passing + `thread_safety` explicitly still wins. +- Snapshotting in `_keys`/`_values`/`_items` instead of holding the mutex + across `yield`. The old code kept the lock held for as long as the + *consumer* took to iterate — and forever if the consumer abandoned the + generator, since the lock was only released when the generator was + closed. That hazard was latent while the lock defaulted to off; turning + the lock on by default would have made it real. `LRUCache` is not used inside `mode` itself; it is exported utility surface -(faust is a consumer), so the blast radius is downstream. +(faust is a consumer), so the blast radius was downstream. -### 2. `cached_property` hands different objects to different threads — free-threading-specific +### 2. `cached_property` handed different objects to different threads -**Severity: high.** +**Was: high. Free-threading-specific.** -`cached_property.__get__` (`mode/utils/objects.py:685-694`) is a -check-then-act on `obj.__dict__`: try the key, catch `KeyError`, compute, -store. Nothing makes that atomic. +`cached_property.__get__` was a check-then-act on `obj.__dict__`: try the +key, catch `KeyError`, compute, store. Nothing made that atomic. | | duplicate-object trials | computes per 300 properties | |---|---|---| | GIL 3.14 | 0/300 | 300 | | free-threaded 3.14t | **104/300** | 419 | -This is not merely wasted work. `ServiceProxy` documents -`@cached_property _service` as *the* way to build the proxied service -(`mode/proxy.py:17-35`) — it is how the Faust App is constructed at module -level. A reproducer that races 16 threads on `proxy._service`: +This was not merely wasted work. `ServiceProxy` documents +`@cached_property _service` as *the* way to build the proxied service — it +is how the Faust App is constructed at module level. Racing 16 threads on +`proxy._service`: | | trials that built/returned >1 `Service` | |---|---| | GIL 3.14 | 0/200 | | free-threaded 3.14t | **198/200** | -So one thread can `start()` one `Service` instance while another thread -holds a different instance, and the later `stop()` never reaches the one -that was started. +So one thread could `start()` one `Service` instance while another held a +different instance, and the later `stop()` never reached the one that was +started. + +**Fixed** in `mode/utils/objects.py` with double-checked locking: the +already-cached lookup stays lock-free (a plain dict hit), and only the miss +path takes a per-descriptor `RLock` and re-checks after acquiring. +Contention is therefore limited to first-time initialisation. Note that stdlib `functools.cached_property` deliberately dropped its lock in 3.12 and accepts duplicate computation. That trade-off is fine for a -pure value cache; it is not fine for a singleton service handle. The fix is -double-checked locking in `cached_property.__get__` (a per-instance or -per-descriptor lock), or failing that, making `ServiceProxy._service` -guard itself. +pure value cache; it is not fine for a singleton service handle. -### 3. Concurrent first `import mode` can hand back a half-built module — pre-existing, much worse under free threading +### 3. Concurrent first `import mode` could hand back a half-built module -**Severity: high.** This one breaks the most ordinary thing a user does. +**Was: high. Pre-existing, but much worse under free threading.** This one +broke the most ordinary thing a user does. -`mode/__init__.py` uses the Werkzeug lazy-import trick: it defines a -`_module` subclass with a `__getattr__` that resolves the lazily-exported -names, then swaps it into `sys.modules` at the *end* of the module body -(`mode/__init__.py:88-129`): - -```python -new_module = sys.modules[__name__] = _module(__name__) -new_module.__dict__.update({"__file__": ..., "__path__": ..., ...}) -``` +`mode/__init__.py` used the Werkzeug lazy-import trick: define a `_module` +subclass whose `__getattr__` resolves the lazily-exported names, then swap +it into `sys.modules` at the *end* of the module body. -If thread B runs `import mode` while thread A is still executing -`mode/__init__.py`, B can be handed the original, pre-swap module object — -which has no `__getattr__` yet — so every lazily-exported name raises: +If thread B ran `import mode` while thread A was still executing +`mode/__init__.py`, B could be handed the original, pre-swap module object — +which has no `__getattr__` — so every lazily-exported name raised: ``` AttributeError: module 'mode' has no attribute 'Service' @@ -160,41 +167,48 @@ Racing 16 threads on a cold `import mode` followed by attribute access: | GIL 3.14 | 3/25 | | free-threaded 3.14t | **14/25** | -Instrumenting a failing thread confirms the mechanism: the object it -imported is a plain `module` (`type(mode).__name__ == "module"`) while -`sys.modules["mode"]` is already the `_module` instance — the thread holds -the stale pre-swap object. The replacement module also carries **no -`__spec__`** (`sys.modules["mode"].__spec__ is None`), which is what -deprives the import machinery of the `_initializing` flag it would -otherwise use to make the second thread wait. - -The fix is to drop the `sys.modules` swap entirely and use a PEP 562 -module-level `__getattr__`, which needs no module replacement and is -therefore race-free. PEP 562 landed in 3.7 and mode's floor is 3.10, so the -`_module` class exists only for compatibility that is no longer needed: - -```python -def __getattr__(name: str) -> Any: - if name in object_origins: - module = __import__(object_origins[name], None, None, [name]) - return getattr(module, name) - raise AttributeError(f"module {__name__!r} has no attribute {name!r}") -``` +Instrumenting a failing thread confirmed the mechanism: the object it +imported was a plain `module` while `sys.modules["mode"]` was already the +`_module` instance — the thread held the stale pre-swap object. The +replacement module also carried **no `__spec__`**, which deprived the import +machinery of the `_initializing` flag it would otherwise use to make the +second thread wait. + +**Fixed** by dropping the `sys.modules` swap entirely in favour of a +:pep:`562` module-level `__getattr__` (plus a module `__dir__`). PEP 562 +landed in 3.7 and mode's floor is 3.10, so the `_module` class existed only +for compatibility that is no longer needed. With no swap, the race cannot +happen — and `sys.modules["mode"]` keeps its real `__spec__`. + +### 4. `Signal` mutated its receiver set during iteration -### 4. `Signal` mutates its receiver set during iteration — pre-existing +**Was: medium. Pre-existing, not a free-threading regression** — it raised +`RuntimeError: Set changed size during iteration` in 30/30 trials on *both* +builds, so `Signal` had never been thread-safe. -**Severity: medium. Not a free-threading regression.** +`_get_live_receivers` iterated `self._receivers` (a plain `set`) while +`connect`/`disconnect` added to and discarded from it — and the caller then +discarded dead refs from the same set using the result. -`_get_live_receivers` iterates `self._receivers` (a plain `set`) -(`mode/signals.py:157-167`) while `connect`/`disconnect` add and discard on -it (`mode/signals.py:120`, `:132`). Racing those raises -`RuntimeError: Set changed size during iteration` in **30/30 trials on both -builds** — so `Signal` has never been thread-safe. Free threading only -makes concurrent use likely enough to hit it in practice. +**Fixed** in `mode/signals.py` by iterating a snapshot. -Fix: iterate a snapshot, e.g. `for href in tuple(r):`. +The snapshot must be `list(r)`, **not** `tuple(r)`. This is not stylistic: + +| snapshot of a set being mutated by 4 threads | free-threaded 3.14t | +|---|---| +| `tuple(s)` | **8 failures** — `Set changed size during iteration` | +| `list(s)` | 0 failures | +| `set(s)` | 0 failures | +| `s.copy()` | 0 failures | +| `frozenset(s)` | 0 failures | -### 5. The `gevent` extra re-enables the GIL — packaging +`list()`, `set()` and `set.copy()` take the source set's per-object lock for +the duration of the copy; `tuple()` falls back to the generic iterator +protocol and does not, so `tuple(r)` raises the very error the snapshot +exists to prevent. The first attempt at this fix used `tuple(r)` and the +stress harness caught it. + +### Not fixable here: the `gevent` extra re-enables the GIL | extra | result on `3.14t` | |---|---| @@ -211,40 +225,30 @@ module 'gevent.libev.corecext', which has not declared that it can run safely without the GIL. ``` -This is upstream in gevent, not something `mode` can fix — it should be -documented as an unsupported combination. - -## Suggested order of work - -1. Fix `LRUCache` (finding 1) — it is an interpreter crash. -2. Fix `cached_property` (finding 2) — silent correctness bug for - `ServiceProxy`, and therefore for faust. -3. Convert `mode/__init__.py` to a PEP 562 module `__getattr__` - (finding 3) — breaks plain `import mode`, and is a real bug under the - GIL too. -4. Snapshot the `Signal` receiver set (finding 4) — cheap, and also - pre-existing. -5. Add `3.14t` to the `tests.yml` matrix. `actions/setup-python` accepts - the `3.14t` version string directly. -6. Add a trove classifier once 1-4 land: - `Programming Language :: Python :: Free Threading :: 2 - Beta` - (the `Free Threading :: N - ...` classifiers are registered in - `trove-classifiers`). -7. Document `mode[gevent]` as incompatible with free-threaded builds. +This is upstream in gevent, not something `mode` can fix. It is flagged in +`pyproject.toml` next to the extra. + +## CI + +`3.14t` is part of the `tests.yml` matrix, so the suite — including +`tests/functional/test_thread_safety.py` — runs with the GIL disabled on +every push. `ruff` and `mypy` both run clean on the free-threaded build. + +The package advertises +`Programming Language :: Python :: Free Threading :: 2 - Beta`. ### A note on `pytest-run-parallel` `pytest-run-parallel` installs and runs on `3.14t`, but pointing `--parallel-threads` at the existing suite is not useful: it reports ~33 failures in `tests/functional/utils/test_collections.py` alone that are -artifacts of tests sharing mutable fixtures and `Mock` objects, not -mode bugs. For example -`test_AttributeDictMixin::test_set_get` fails with "DID NOT RAISE -AttributeError" purely because a sibling thread already set the attribute -on the shared object. +artifacts of tests sharing mutable fixtures and `Mock` objects, not mode +bugs. For example `test_AttributeDictMixin::test_set_get` fails with "DID +NOT RAISE AttributeError" purely because a sibling thread already set the +attribute on the shared object. -Use it selectively on purpose-written thread-safety tests rather than -across the whole suite. +Use it selectively on purpose-written thread-safety tests rather than across +the whole suite. ## Reproducing @@ -252,10 +256,11 @@ across the whole suite. uv python install 3.14t uv venv --python 3.14t .venv-ft VIRTUAL_ENV=.venv-ft uv pip install -e . -r requirements-tests.txt +.venv-ft/bin/python -m pytest tests/unit tests/functional .venv-ft/bin/python tests/freethreading/stress.py ``` -`tests/freethreading/` is deliberately outside the `testpaths` configured -in `pyproject.toml`, so the crash reproducers are never collected by a -normal `pytest` run. Run the same file under a GIL-enabled interpreter to -see the control numbers. +`tests/freethreading/` is deliberately outside the `testpaths` configured in +`pyproject.toml`, so the heavier probabilistic reproducers are never +collected by a normal `pytest` run. Run the same file under a GIL-enabled +interpreter to see the control numbers. diff --git a/mode/__init__.py b/mode/__init__.py index eaca124..2f377c2 100644 --- a/mode/__init__.py +++ b/mode/__init__.py @@ -2,13 +2,11 @@ __version__ = "0.0.1" -import sys import typing from collections.abc import Mapping, Sequence -# Lazy loading. -# - See werkzeug/__init__.py for the rationale behind this. -from types import ModuleType +# Lazy loading, via the PEP 562 module __getattr__ defined at the bottom +# of this file. from typing import Any # -eof meta- @@ -88,47 +86,47 @@ object_origins[item] = module -class _module(ModuleType): - """Customized Python module.""" - - def __getattr__(self, name: str) -> Any: - if name in object_origins: - module = __import__(object_origins[name], None, None, [name]) - for extra_name in all_by_module[module.__name__]: - setattr(self, extra_name, getattr(module, extra_name)) - return getattr(module, name) - return ModuleType.__getattribute__(self, name) - - def __dir__(self) -> Sequence[str]: - result = list(new_module.__all__) - result.extend( - ( - "__file__", - "__path__", - "__doc__", - "__all__", - "__docformat__", - "__name__", - "__path__", - "VERSION", - "version_info", - "__package__", - ) - ) - return result - - -# keep a reference to this module so that it's not garbage collected -old_module = sys.modules[__name__] - -new_module = sys.modules[__name__] = _module(__name__) -new_module.__dict__.update( - { - "__file__": __file__, - "__path__": __path__, - "__doc__": __doc__, - "__all__": tuple(object_origins), - "__version__": __version__, - "__package__": __package__, - } -) +# NOTE: This is a :pep:`562` module-level ``__getattr__``, and deliberately +# *not* the older trick of defining a ``ModuleType`` subclass and swapping it +# into ``sys.modules[__name__]`` at the end of this file. +# +# That swap was a race: it only happened once the module body had finished, +# so a thread calling ``import mode`` while another thread was still +# executing this file could be handed the original, pre-swap module object -- +# which has no ``__getattr__`` on it -- and every lazily-exported name below +# raised ``AttributeError: module 'mode' has no attribute 'Service'``. The +# replacement module also carried no ``__spec__``, which denied the import +# machinery the ``_initializing`` flag it uses to make the second thread wait. +# Rare under the GIL, common on free-threaded (:pep:`703`) builds. +# +# A module ``__getattr__`` needs no swap at all, so the race cannot happen. +def __getattr__(name: str) -> Any: + try: + origin = object_origins[name] + except KeyError: + raise AttributeError( + f"module {__name__!r} has no attribute {name!r}" + ) from None + module = __import__(origin, None, None, [name]) + # Bind every name this module provides, not just the requested one, so + # that later lookups are plain globals and never reach __getattr__ again. + namespace = globals() + for extra_name in all_by_module[origin]: + namespace[extra_name] = getattr(module, extra_name) + return namespace[name] + + +def __dir__() -> Sequence[str]: + return [ + *__all__, + "__file__", + "__path__", + "__doc__", + "__all__", + "__docformat__", + "__name__", + "VERSION", + "version_info", + "__package__", + "__version__", + ] diff --git a/mode/signals.py b/mode/signals.py index dad01d6..63c3120 100644 --- a/mode/signals.py +++ b/mode/signals.py @@ -159,7 +159,19 @@ def _get_live_receivers( ) -> tuple[set[SignalHandlerT], set[SignalHandlerRefT]]: live_receivers: set[SignalHandlerT] = set() dead_refs: set[SignalHandlerRefT] = set() - for href in r: + # NOTE: Iterate a snapshot. `r` is the live receiver set shared by + # this signal and every clone of it, and `connect`/`disconnect` + # mutate it from whatever thread or task calls them -- iterating it + # directly raises "Set changed size during iteration". The caller + # also discards dead refs from `r` using what this returns, which + # is itself a mutation during iteration. + # + # It must be `list(r)`, NOT `tuple(r)`: on free-threaded builds + # `list()` (like `set()` and `set.copy()`) takes the source set's + # per-object lock for the duration of the copy, while `tuple()` + # falls back to the generic iterator protocol and does not -- so + # `tuple(r)` raises the very error this snapshot exists to avoid. + for href in list(r): alive, value = self._is_alive(href) if alive and value is not None: live_receivers.add(value) diff --git a/mode/utils/collections.py b/mode/utils/collections.py index 5c5aaf4..a813507 100644 --- a/mode/utils/collections.py +++ b/mode/utils/collections.py @@ -2,9 +2,10 @@ import abc import collections.abc +import sys import threading import typing -from collections import OrderedDict, UserList +from collections import UserList from collections.abc import ( ItemsView, Iterable, @@ -51,6 +52,12 @@ class LazyObject: ... class LazySettings: ... +#: True when running on a free-threaded (:pep:`703`) build with the GIL +#: actually disabled. Checked at runtime rather than build time so that +#: ``PYTHON_GIL=1`` on a free-threaded interpreter is respected. +FREE_THREADED: bool = not getattr(sys, "_is_gil_enabled", lambda: True)() + + __all__ = [ "AttributeDict", "AttributeDictMixin", @@ -438,21 +445,38 @@ class LRUCache(FastUserDict, MutableMapping[KT, VT], MappingViewProxy): the *Least Recently Used* key will be discarded from the cache. thread_safety (bool): Enable if multiple OS threads are going - to access/mutate the cache. + to access/mutate the cache. Defaults to :const:`True` on + free-threaded builds, where there is no GIL to make unguarded + access incidentally safe, and :const:`False` otherwise (which + is what it has always been). + + Note: + The backing store is a plain :class:`dict`, not an + :class:`~collections.OrderedDict`. Both preserve insertion order + (guaranteed for `dict` since Python 3.7), but on free-threaded + builds only `dict` is safe to mutate concurrently: + `OrderedDict` keeps a separate linked list that racing threads + can corrupt badly enough to segfault the interpreter, whereas + `dict` has per-object locking. """ limit: Optional[int] thread_safety: bool _mutex: AbstractContextManager - data: OrderedDict + data: dict def __init__( - self, limit: Optional[int] = None, *, thread_safety: bool = False + self, + limit: Optional[int] = None, + *, + thread_safety: Optional[bool] = None, ) -> None: self.limit = limit - self.thread_safety = thread_safety + self.thread_safety = ( + FREE_THREADED if thread_safety is None else thread_safety + ) self._mutex = self._new_lock() - self.data: OrderedDict = OrderedDict() + self.data: dict = {} def __getitem__(self, key: KT) -> VT: with self._mutex: @@ -466,11 +490,23 @@ def update(self, *args: Any, **kwargs: Any) -> None: if limit and len(data) > limit: # pop additional items in case limit exceeded for _ in range(len(data) - limit): - data.popitem(last=False) + self._popitem_first() + + def _popitem_first(self) -> tuple[KT, VT]: + # `dict` only pops from the right, so emulate the + # `OrderedDict.popitem(last=False)` this used to call. + # Caller must hold the mutex. + try: + key = next(iter(self.data)) + except StopIteration: + raise KeyError("dictionary is empty") from None + return key, self.data.pop(key) def popitem(self, *, last: bool = True) -> tuple[KT, VT]: with self._mutex: - return self.data.popitem(last) + if last: + return self.data.popitem() + return self._popitem_first() def __setitem__(self, key: KT, value: VT) -> None: # remove least recently used key. @@ -479,8 +515,17 @@ def __setitem__(self, key: KT, value: VT) -> None: self.data.pop(next(iter(self.data))) self.data[key] = value + # NOTE: Iteration takes a snapshot under the mutex and yields from that + # snapshot with the mutex released, rather than holding it across the + # yields. Holding a lock across a yield keeps it held for as long as + # the *consumer* takes to iterate -- and forever if the consumer + # abandons the generator half way, since the mutex is only released + # when the generator is closed. Snapshotting also means a concurrent + # writer cannot invalidate an iteration already in progress, which is + # what "dictionary changed size during iteration" used to be. + def __iter__(self) -> Iterator: - return iter(self.data) + return self._keys() def keys(self) -> KeysView[KT]: return ProxyKeysView(self) @@ -488,29 +533,24 @@ def keys(self) -> KeysView[KT]: def _keys(self) -> Iterator[KT]: # userdict.keys in py3k calls __getitem__ with self._mutex: - yield from self.data.keys() + keys = list(self.data) + yield from keys def values(self) -> ValuesView[VT]: return ProxyValuesView(self) def _values(self) -> Iterator[VT]: with self._mutex: - for k in self: - try: - yield self.data[k] - except KeyError: # pragma: no cover - pass + values = list(self.data.values()) + yield from values def items(self) -> ItemsView[KT, VT]: return ProxyItemsView(self) def _items(self) -> Iterator[tuple[KT, VT]]: with self._mutex: - for k in self: - try: - yield (k, self.data[k]) - except KeyError: # pragma: no cover - pass + items = list(self.data.items()) + yield from items def incr(self, key: KT, delta: int = 1) -> int: with self._mutex: diff --git a/mode/utils/objects.py b/mode/utils/objects.py index fb29253..25a2822 100644 --- a/mode/utils/objects.py +++ b/mode/utils/objects.py @@ -3,6 +3,7 @@ import abc import collections.abc import sys +import threading import types import typing from collections.abc import ( @@ -678,6 +679,7 @@ def __init__( self.__name__ = fget.__name__ self.__module__ = fget.__module__ self.class_attribute: Optional[str] = class_attribute + self.__lock = threading.RLock() def is_set(self, obj: Any) -> bool: return self.__name__ in obj.__dict__ @@ -690,8 +692,25 @@ def __get__(self, obj: Any, type: Optional[type] = None) -> RT: try: return cast(RT, obj.__dict__[self.__name__]) except KeyError: - value = obj.__dict__[self.__name__] = self.__get(obj) - return value + pass + # NOTE: The lookup above is the fast path and stays lock-free: once + # the value is cached, reading it is a plain dict hit. Only the + # miss path locks, and it re-checks after acquiring, because + # "look, then compute, then store" is not atomic. Without this, + # two threads that miss together each run `fget` and each store a + # *different* object, so callers disagree about which one is the + # cached one. That is not merely wasted work here: `ServiceProxy` + # documents `@cached_property _service` as the way to build the + # proxied service, and a duplicate there means `start()` and + # `stop()` can act on different Service instances. The GIL made + # this nearly impossible to hit; free-threaded builds hit it + # constantly. + with self.__lock: + try: + return cast(RT, obj.__dict__[self.__name__]) + except KeyError: + value = obj.__dict__[self.__name__] = self.__get(obj) + return value def __set__(self, obj: Any, value: RT) -> None: if self.__set is not None: diff --git a/pyproject.toml b/pyproject.toml index 8de4f1e..496f474 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -40,6 +40,7 @@ classifiers = [ "Programming Language :: Python :: 3.12", "Programming Language :: Python :: 3.13", "Programming Language :: Python :: 3.14", + "Programming Language :: Python :: Free Threading :: 2 - Beta", "Programming Language :: Python :: Implementation :: CPython", "Programming Language :: Python :: Implementation :: PyPy", "Operating System :: POSIX", @@ -61,6 +62,9 @@ eventlet = [ "faust-aioeventlet", "dnspython", ] +# NOTE: Not usable on free-threaded (PEP 703) builds. gevent's +# `gevent.libev.corecext` does not declare that it is safe without the GIL, +# so importing it re-enables the GIL and silently undoes free threading. gevent = [ "asyncio-gevent~=0.2", ] diff --git a/tests/freethreading/stress.py b/tests/freethreading/stress.py index b713689..1c0d11b 100644 --- a/tests/freethreading/stress.py +++ b/tests/freethreading/stress.py @@ -1,9 +1,17 @@ """Free-threading (PEP 703) stress reproducers for mode. -This file is intentionally NOT under the ``testpaths`` configured in -``pyproject.toml``, because some of the checks below can segfault a -free-threaded interpreter by design -- that is the finding, not a bug in -the harness. Run it directly: +Every check here should now report ``ok``. Each one reproduced a real +defect before the fix it guards, and they are kept because they are +probabilistic and heavy -- they hammer each surface with 16 threads over +many trials, which is how the ``tuple(r)``-is-not-atomic problem in +`mode.signals` was caught after the first attempt at that fix passed the +cheaper tests. + +The deterministic versions live in +`tests/functional/test_thread_safety.py` and run in CI. This file is +deliberately NOT under the ``testpaths`` configured in ``pyproject.toml``: +before the fixes some of these checks segfaulted the interpreter, and a +regression here should not take the whole test run down with it. ```sh uv python install 3.14t @@ -12,11 +20,10 @@ .venv-ft/bin/python tests/freethreading/stress.py ``` -Run it again under a GIL-enabled interpreter of the same version to get -the control numbers -- most of these checks pass there, which is what -makes them free-threading findings rather than plain bugs. +Run it under a GIL-enabled interpreter of the same version too -- the +fixes are meant to hold on both. -See `docs/free-threading.md` for the measured results and analysis. +See `docs/free-threading.md` for the measurements and the analysis. """ import sys @@ -61,14 +68,15 @@ def report(name, errors, note=""): # -------------------------------------------------------------------------- -# Finding 1: LRUCache is backed by OrderedDict with thread_safety=False by -# default. Concurrent mutate+iterate segfaults a free-threaded interpreter -# (plain dict is safe there; OrderedDict's C implementation is not). +# Defect 1 (fixed): LRUCache was backed by OrderedDict with +# thread_safety=False by default, so concurrent mutate+iterate segfaulted a +# free-threaded interpreter. It is a plain dict now, and thread_safety +# defaults to on for free-threaded builds. # -------------------------------------------------------------------------- def check_lru_default(trials=60): from mode.utils.collections import LRUCache - print(" (this check can segfault on a free-threaded build)", flush=True) + print(" (this check segfaulted before the fix)", flush=True) bad = 0 for _ in range(trials): cache = LRUCache(limit=50) @@ -108,10 +116,11 @@ def work(i, cache=cache): # -------------------------------------------------------------------------- -# Finding 2: cached_property.__get__ is a non-atomic check-then-act on -# obj.__dict__, so racing threads can each compute and hand out a distinct +# Defect 2 (fixed): cached_property.__get__ was a non-atomic check-then-act +# on obj.__dict__, so racing threads each computed and handed out a distinct # object. ServiceProxy documents @cached_property as the way to build the -# proxied service, so the duplicate is a real singleton violation. +# proxied service, so the duplicate was a real singleton violation. The +# miss path is double-checked under a lock now. # -------------------------------------------------------------------------- def check_cached_property(trials=300): from mode.utils.objects import cached_property @@ -182,8 +191,9 @@ def work(i, proxy=proxy, seen=seen, seen_lock=seen_lock): # -------------------------------------------------------------------------- -# Finding 3: Signal iterates its receiver set while connect/disconnect -# mutate it. Pre-existing -- this fails on GIL builds too. +# Defect 4 (fixed): Signal iterated its receiver set while connect/disconnect +# mutated it. Pre-existing -- this failed on GIL builds too. It snapshots +# with list() now (NOT tuple(), which does not lock the source set). # -------------------------------------------------------------------------- def check_signal(trials=30): from mode.signals import Signal @@ -344,11 +354,11 @@ def work(i): # -------------------------------------------------------------------------- -# Finding 3: mode/__init__.py swaps sys.modules["mode"] for a _module -# instance at the END of its body, so a thread importing mode concurrently -# can be handed the original pre-swap module -- which has no __getattr__ -- -# and every lazily-exported name raises AttributeError. Pre-existing, but -# far more likely with the GIL disabled. +# Defect 3 (fixed): mode/__init__.py swapped sys.modules["mode"] for a +# _module instance at the END of its body, so a thread importing mode +# concurrently could be handed the original pre-swap module -- which has no +# __getattr__ -- and every lazily-exported name raised AttributeError. It +# uses a PEP 562 module __getattr__ now, so there is no swap to race with. # # Must run in a subprocess: the race only exists on a *cold* import. # -------------------------------------------------------------------------- @@ -404,7 +414,7 @@ def main(): check_managed_user_dict() check_lru_thread_safe() - print("\n-- findings --") + print("\n-- regression checks (all should be ok) --") check_lazy_module() check_signal() check_cached_property() diff --git a/tests/functional/test_thread_safety.py b/tests/functional/test_thread_safety.py new file mode 100644 index 0000000..0b4152b --- /dev/null +++ b/tests/functional/test_thread_safety.py @@ -0,0 +1,296 @@ +"""Regression tests for the thread-safety fixes. + +These all guard defects that free-threaded (:pep:`703`) builds made +reachable in practice. They are written to fail deterministically on a +GIL-enabled interpreter too, so the whole matrix protects them rather than +just the ``3.14t`` leg. + +See `docs/free-threading.md` for the measurements behind each one, and +`tests/freethreading/stress.py` for the heavier probabilistic reproducers. +""" + +import sys +import threading +import time +from types import ModuleType + +import pytest + +import mode +from mode.proxy import ServiceProxy +from mode.signals import Signal +from mode.utils.collections import FREE_THREADED, LRUCache +from mode.utils.objects import cached_property + + +class test_cached_property_is_computed_once: + def _race_on(self, obj, nthreads=8): + barrier = threading.Barrier(nthreads) + seen = [] + lock = threading.Lock() + + def work(): + barrier.wait() + value = obj.val + with lock: + seen.append(value) + + threads = [threading.Thread(target=work) for _ in range(nthreads)] + for t in threads: + t.start() + for t in threads: + t.join() + return seen + + def test_concurrent_miss_computes_once(self): + # The getter sleeps, which releases the GIL, so without the lock in + # `cached_property.__get__` every thread would enter it and store a + # different object. This fails on GIL builds too, by design. + calls = [] + calls_lock = threading.Lock() + + class X: + @cached_property + def val(self): + with calls_lock: + calls.append(1) + time.sleep(0.05) + return object() + + seen = self._race_on(X()) + + assert len(calls) == 1 + assert len({id(v) for v in seen}) == 1 + + def test_service_proxy_service_is_a_singleton(self): + # ServiceProxy documents @cached_property _service as the way to + # build the proxied service, so a duplicate there means start() and + # stop() can act on different Service instances. + built = [] + built_lock = threading.Lock() + + class MyProxy(ServiceProxy): + @cached_property + def _service(self): + service = mode.Service() + with built_lock: + built.append(service) + time.sleep(0.05) + return service + + proxy = MyProxy() + barrier = threading.Barrier(8) + seen = [] + seen_lock = threading.Lock() + + def work(): + barrier.wait() + # Resolve outside the lock -- holding it here would serialise + # the very access this test is trying to race. + service = proxy._service + with seen_lock: + seen.append(service) + + threads = [threading.Thread(target=work) for _ in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert len(built) == 1 + assert len({id(s) for s in seen}) == 1 + + def test_cached_value_is_still_reused(self): + calls = [] + + class X: + @cached_property + def val(self): + calls.append(1) + return object() + + x = X() + assert x.val is x.val + assert len(calls) == 1 + + +class test_LRUCache_thread_safety: + def test_backed_by_plain_dict(self): + # Not an OrderedDict: on free-threaded builds concurrent mutation + # of an OrderedDict can corrupt its linked list and segfault the + # interpreter, while plain dict has per-object locking. + assert type(LRUCache().data) is dict + + def test_thread_safety_defaults_to_free_threaded(self): + assert LRUCache().thread_safety is FREE_THREADED + + @pytest.mark.parametrize("thread_safety", [True, False]) + def test_thread_safety_can_be_overridden(self, thread_safety): + assert LRUCache(thread_safety=thread_safety).thread_safety is ( + thread_safety + ) + + def test_popitem_last_is_lifo(self): + c = LRUCache() + c.update({"a": 1, "b": 2, "c": 3}) + assert c.popitem() == ("c", 3) + assert c.popitem(last=True) == ("b", 2) + + def test_popitem_first_is_fifo(self): + c = LRUCache() + c.update({"a": 1, "b": 2, "c": 3}) + assert c.popitem(last=False) == ("a", 1) + assert c.popitem(last=False) == ("b", 2) + + def test_popitem_empty_raises_KeyError(self): + with pytest.raises(KeyError): + LRUCache().popitem() + with pytest.raises(KeyError): + LRUCache().popitem(last=False) + + def test_limit_still_evicts_oldest(self): + c = LRUCache(limit=3) + for i in range(10): + c[i] = i + assert list(c.keys()) == [7, 8, 9] + + def test_iteration_does_not_hold_the_lock_across_yields(self): + # A half-consumed iterator must not keep the mutex held: the lock + # is reentrant, so only a *different* thread shows the problem. + # Previously the writer below blocked until the abandoned + # generator was collected. + c = LRUCache(limit=100, thread_safety=True) + c.update({"a": 1, "b": 2, "c": 3}) + it = iter(c.keys()) + next(it) # deliberately left half-consumed + + done = threading.Event() + + def writer(): + c["d"] = 4 + done.set() + + thread = threading.Thread(target=writer) + thread.start() + thread.join(timeout=10.0) + + assert done.is_set(), "writer blocked on a half-consumed iterator" + assert c["d"] == 4 + + def test_concurrent_mutation_and_iteration(self): + # Deliberately the *default* configuration: this is what used to + # segfault the interpreter on free-threaded builds. + c = LRUCache(limit=50) + barrier = threading.Barrier(8) + errors = [] + + def work(i): + barrier.wait() + try: + for n in range(200): + c[f"{i}-{n}"] = n + list(c.keys()) + list(c.items()) + list(c.values()) + except BaseException as exc: # pragma: no cover + errors.append(exc) + + threads = [threading.Thread(target=work, args=(i,)) for i in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert not errors + + +class test_Signal_receiver_iteration: + def test_get_live_receivers_tolerates_mutation(self): + # Directly simulate a connect() landing while the receiver set is + # being walked. Before the snapshot this raised + # "Set changed size during iteration". + signal = Signal() + + async def handler(*args, **kwargs): ... + + for _ in range(4): + signal.connect(handler) + receivers = signal._receivers + original_is_alive = signal._is_alive + + def mutating_is_alive(ref): + receivers.add(lambda: handler) + return original_is_alive(ref) + + signal._is_alive = mutating_is_alive + + live, _dead = signal._get_live_receivers(receivers) + assert live + + def test_iter_receivers_while_connecting(self): + class Owner: + sig = Signal() + + owner = Owner() + signal = Owner.sig + barrier = threading.Barrier(8) + errors = [] + + def work(i): + barrier.wait() + try: + for _n in range(200): + + async def handler(*args, **kwargs): ... + + if i % 2: + signal.connect(handler) + signal.disconnect(handler) + else: + list(signal.iter_receivers(owner)) + except BaseException as exc: # pragma: no cover + errors.append(exc) + + threads = [threading.Thread(target=work, args=(i,)) for i in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert not errors + + +class test_mode_lazy_imports: + def test_module_is_not_replaced_in_sys_modules(self): + # The old Werkzeug-style trick swapped sys.modules["mode"] for a + # ModuleType *subclass* at the end of mode/__init__.py. That swap + # was the race: a thread importing mode concurrently could be + # handed the original pre-swap module, which had no __getattr__. + # A PEP 562 module __getattr__ needs no swap at all. + assert type(sys.modules["mode"]) is ModuleType + + def test_module_keeps_its_spec(self): + # The replacement module carried no __spec__, which denied the + # import machinery the _initializing flag it uses to make a second + # importing thread wait. + assert mode.__spec__ is not None + assert mode.__spec__.name == "mode" + + def test_lazy_names_resolve(self): + from mode.services import Service + + assert mode.Service is Service + + def test_resolving_one_name_binds_its_siblings(self): + assert mode.task is not None + assert "timer" in vars(mode) + + def test_unknown_attribute_raises_AttributeError(self): + with pytest.raises(AttributeError) as excinfo: + mode.NoSuchThing # noqa: B018 + assert "NoSuchThing" in str(excinfo.value) + + def test_dir_lists_the_lazy_names(self): + listed = dir(mode) + for name in mode.__all__: + assert name in listed From 62448739b3e76606dcc45ca4f318a2423451475c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 21:40:07 +0000 Subject: [PATCH 04/16] Warn when the gevent loop is selected on a free-threaded build Importing gevent re-enables the GIL, and nothing about that is visible to the caller -- the process keeps running and simply is not free-threaded any more. The pyproject note is invisible at runtime, so warn from mode/loop/gevent.py instead. The check reads the build flag via sysconfig rather than sys._is_gil_enabled(), because the runtime check would already report True by the time gevent has been imported -- which is the very situation being reported. Also records in docs/free-threading.md that mode.loop.use("gevent") is currently broken on every build, which is unrelated to free threading: it fails identically on GIL-enabled 3.10 and 3.14 with gevent 26.7.0. mode/loop/gevent.py points GEVENT_LOOP at mode.loop._gevent_loop.Loop, but that module imports gevent.core at module scope to subclass gevent.core.loop, so importing it builds a gevent hub, which resolves GEVENT_CONFIG.loop, which imports the same module before its body has reached `class Loop`. The cycle is inside the module's own import, so pre-importing it does not help. gevent alone is fine -- patch_all() plus asyncio_gevent.EventLoopPolicy runs a coroutine correctly -- only mode's custom hook fails. mode.loop has no test coverage, which is how this went unnoticed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- docs/free-threading.md | 31 ++++++++++++++++++++++++++++++- mode/loop/gevent.py | 15 +++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/docs/free-threading.md b/docs/free-threading.md index 0e7b236..c21d1ef 100644 --- a/docs/free-threading.md +++ b/docs/free-threading.md @@ -226,7 +226,36 @@ safely without the GIL. ``` This is upstream in gevent, not something `mode` can fix. It is flagged in -`pyproject.toml` next to the extra. +`pyproject.toml` next to the extra, and `mode/loop/gevent.py` now warns at +import time on a free-threaded build — the degradation is otherwise silent, +since you keep running and simply are not free-threaded any more. That check +uses the *build* flag (`sysconfig.get_config_var("Py_GIL_DISABLED")`) rather +than `sys._is_gil_enabled()`, which by then already reads `True`. + +**Separately: `mode.loop.use("gevent")` is currently broken on every build.** +This has nothing to do with free threading — it fails identically on +GIL-enabled 3.10 and 3.14 with gevent 26.7.0: + +``` +ImportError: Cannot import 'Loop' from +``` + +The cause is a self-referential import. `mode/loop/gevent.py` sets +`GEVENT_LOOP=mode.loop._gevent_loop.Loop`, but `mode/loop/_gevent_loop.py` +imports `gevent.core` at module scope in order to subclass +`gevent.core.loop`. Importing it therefore builds a gevent hub, which +resolves `GEVENT_CONFIG.loop`, which imports `mode.loop._gevent_loop` — a +module whose body has not yet reached `class Loop`. Pre-importing the module +does not help, because the cycle is inside its own import. + +gevent itself is fine: `gevent.monkey.patch_all()` plus +`asyncio_gevent.EventLoopPolicy` runs an asyncio coroutine correctly. Only +mode's custom `GEVENT_LOOP` hook fails. Presumably gevent used to resolve +that setting lazily and no longer does. + +`mode.loop` has no test coverage, which is how this went unnoticed. Fixing +it means building `Loop` lazily rather than at module scope, and is a +separate piece of work from anything on this page. ## CI diff --git a/mode/loop/gevent.py b/mode/loop/gevent.py index 7ac0f22..26a5951 100644 --- a/mode/loop/gevent.py +++ b/mode/loop/gevent.py @@ -2,11 +2,26 @@ import asyncio import os +import sysconfig import warnings from typing import Optional, cast from mode.utils.loops import get_event_loop +# NOTE: Deliberately the *build* flag, not `sys._is_gil_enabled()`. The +# runtime check would already read True by the time gevent has been +# imported below, which is exactly the situation being reported. +if sysconfig.get_config_var("Py_GIL_DISABLED"): + warnings.warn( + "The gevent loop is not usable on free-threaded builds: importing " + "gevent re-enables the GIL (gevent.libev.corecext does not declare " + "that it is safe without it), so selecting this loop silently gives " + "up free threading for the whole process. Use the 'aio' or 'uvloop' " + "loop to keep the GIL disabled.", + RuntimeWarning, + stacklevel=2, + ) + os.environ["GEVENT_LOOP"] = "mode.loop._gevent_loop.Loop" try: import gevent From e656a06e81dfbe5af8c5d45f8c1ca86c0983fce6 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 21:47:18 +0000 Subject: [PATCH 05/16] Deprecate the gevent loop backend It is unmaintained, has no test coverage, re-enables the GIL on free-threaded builds, and does not currently work on any interpreter -- `mode.loop.use("gevent")` fails with "Cannot import 'Loop' from mode.loop._gevent_loop" against current gevent releases, on GIL-enabled 3.10 and 3.14 alike. Nothing is removed, so this is not a breaking change: the backend still resolves exactly as before, and now raises a DeprecationWarning naming the breakage and pointing at the aio and uvloop backends. The warning is raised from `mode.loop.use()` rather than from mode/loop/gevent.py's module body. A module-level `warnings.warn` is attributed to whichever importlib frame executed the body, and DeprecationWarning is filtered out everywhere except __main__, so it was never actually shown -- verified before moving it. Raised from `use()` with stacklevel=2 it lands on the caller, which is where the backend gets selected. Adds tests/unit/test_loop.py, the first coverage mode.loop has had. It patches importlib.import_module throughout: really selecting a backend applies process-wide monkey-patches that would wreck every test running afterwards, which is part of why this module went untested. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- docs/free-threading.md | 9 ++++-- mode/loop/__init__.py | 36 ++++++++++++++++++++- mode/loop/gevent.py | 20 +++++++++++- pyproject.toml | 9 ++++-- tests/unit/test_loop.py | 72 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 139 insertions(+), 7 deletions(-) create mode 100644 tests/unit/test_loop.py diff --git a/docs/free-threading.md b/docs/free-threading.md index c21d1ef..4711051 100644 --- a/docs/free-threading.md +++ b/docs/free-threading.md @@ -254,8 +254,13 @@ mode's custom `GEVENT_LOOP` hook fails. Presumably gevent used to resolve that setting lazily and no longer does. `mode.loop` has no test coverage, which is how this went unnoticed. Fixing -it means building `Loop` lazily rather than at module scope, and is a -separate piece of work from anything on this page. +it would mean building `Loop` lazily rather than at module scope. + +Rather than repair a backend that cannot work on free-threaded builds +anyway, the gevent loop is **deprecated**: selecting it raises a +`DeprecationWarning` naming the breakage and pointing at `aio`/`uvloop`, +and it is slated for removal in a future major release. Nothing is removed +yet, so this is not a breaking change. ## CI diff --git a/mode/loop/__init__.py b/mode/loop/__init__.py index 744119d..8020cea 100644 --- a/mode/loop/__init__.py +++ b/mode/loop/__init__.py @@ -24,7 +24,18 @@ mode.loop.use('eventlet') ``` -### gevent +### gevent **deprecated, currently broken** + +!!! warning + This backend is unmaintained, has no test coverage, and does not + presently work on *any* interpreter: selecting it raises + `ImportError: Cannot import 'Loop' from mode.loop._gevent_loop` with + current gevent releases, on GIL-enabled and free-threaded builds + alike. It also re-enables the GIL on free-threaded builds, because + `gevent.libev.corecext` does not declare that it is safe without it. + + Selecting it raises a `DeprecationWarning`, and it will be removed in + a future major release. Use `aio` (the default) or `uvloop`. Use [`gevent`](https://pypi.org/project/gevent) as the event loop. @@ -59,6 +70,7 @@ """ import importlib +import warnings from collections.abc import Mapping from typing import Optional @@ -71,12 +83,34 @@ "uvloop": "mode.loop.uvloop", } +#: Backends that still resolve, but should not be used in new code. +DEPRECATED_LOOPS: Mapping[str, str] = { + "gevent": ( + "The gevent loop backend is deprecated and currently broken: it is " + "unmaintained, has no test coverage, and importing it fails with " + "current gevent releases on every interpreter (see " + "docs/free-threading.md). It also re-enables the GIL on " + "free-threaded builds. Use the 'aio' or 'uvloop' backend instead. " + "It will be removed in a future major release." + ) +} + def use(loop: str) -> None: """Specify the event loop to use as a string. Loop must be one of: aio, eventlet, gevent, uvloop. + + Note: + `gevent` is deprecated and currently broken -- selecting it raises + a `DeprecationWarning` and then fails to import. See the module + docstring. """ + deprecated = DEPRECATED_LOOPS.get(loop) + if deprecated is not None: + # stacklevel=2 attributes this to the caller, so it is actually + # shown when selected from an entrypoint module. + warnings.warn(deprecated, DeprecationWarning, stacklevel=2) mod = LOOPS.get(loop, loop) if mod is not None: importlib.import_module(mod) diff --git a/mode/loop/gevent.py b/mode/loop/gevent.py index 26a5951..db8516c 100644 --- a/mode/loop/gevent.py +++ b/mode/loop/gevent.py @@ -1,4 +1,15 @@ -"""Enable [`gevent`](https://pypi.org/project/gevent) support for `asyncio`.""" +"""Enable [`gevent`](https://pypi.org/project/gevent) support for `asyncio`. + +!!! warning "Deprecated and currently broken" + This loop backend is unmaintained, has no test coverage, and does not + presently work on *any* interpreter -- importing it raises + `ImportError: Cannot import 'Loop' from mode.loop._gevent_loop` with + current gevent releases, on GIL-enabled and free-threaded builds + alike. See `docs/free-threading.md` for the diagnosis. + + Use the `aio` (default) or `uvloop` backend instead. This module will + be removed in a future major release. +""" import asyncio import os @@ -8,6 +19,13 @@ from mode.utils.loops import get_event_loop +# NOTE: The DeprecationWarning for this backend is raised by +# `mode.loop.use()`, not here. A module-level `warnings.warn` is +# attributed to whichever importlib frame executed the module body, and +# DeprecationWarning is filtered out everywhere except `__main__` -- so it +# would never be shown. Raising it from `use()` with stacklevel=2 puts it +# on the caller instead, which is where users select the backend. + # NOTE: Deliberately the *build* flag, not `sys._is_gil_enabled()`. The # runtime check would already read True by the time gevent has been # imported below, which is exactly the situation being reported. diff --git a/pyproject.toml b/pyproject.toml index 496f474..0c2a806 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -62,9 +62,12 @@ eventlet = [ "faust-aioeventlet", "dnspython", ] -# NOTE: Not usable on free-threaded (PEP 703) builds. gevent's -# `gevent.libev.corecext` does not declare that it is safe without the GIL, -# so importing it re-enables the GIL and silently undoes free threading. +# DEPRECATED, and currently broken on every interpreter: `mode.loop.use +# ("gevent")` fails with "Cannot import 'Loop' from mode.loop._gevent_loop" +# against current gevent releases. It additionally re-enables the GIL on +# free-threaded (PEP 703) builds, because `gevent.libev.corecext` does not +# declare that it is safe without it. Slated for removal in a future major +# release; use the `aio` or `uvloop` backend. See docs/free-threading.md. gevent = [ "asyncio-gevent~=0.2", ] diff --git a/tests/unit/test_loop.py b/tests/unit/test_loop.py new file mode 100644 index 0000000..180265f --- /dev/null +++ b/tests/unit/test_loop.py @@ -0,0 +1,72 @@ +import warnings +from contextlib import contextmanager +from unittest.mock import patch + +import pytest + +import mode.loop +from mode.loop import DEPRECATED_LOOPS, LOOPS + + +@contextmanager +def recorded_warnings(): + with warnings.catch_warnings(record=True) as recorded: + warnings.simplefilter("always") + yield recorded + + +class test_use: + # NOTE: `importlib.import_module` is patched out throughout. Actually + # selecting a backend applies process-wide monkey-patches (gevent and + # eventlet both patch the stdlib), which would wreck every test that + # runs afterwards. + + @pytest.mark.parametrize("loop", ["eventlet", "gevent", "uvloop"]) + def test_imports_the_backend_module(self, loop): + with patch("importlib.import_module") as import_module: + with recorded_warnings(): + mode.loop.use(loop) + import_module.assert_called_once_with(LOOPS[loop]) + + def test_aio_imports_nothing(self): + with patch("importlib.import_module") as import_module: + mode.loop.use("aio") + import_module.assert_not_called() + + def test_unknown_name_is_treated_as_a_module_path(self): + with patch("importlib.import_module") as import_module: + mode.loop.use("my.custom.loop") + import_module.assert_called_once_with("my.custom.loop") + + +class test_deprecated_backends: + def test_gevent_is_deprecated(self): + assert "gevent" in DEPRECATED_LOOPS + + def test_use_gevent_warns(self): + with patch("importlib.import_module"): + with pytest.warns( + DeprecationWarning, match="gevent loop backend is deprecated" + ): + mode.loop.use("gevent") + + def test_warning_precedes_the_import(self): + # The backend currently fails to import, so the warning is only of + # any use if it is raised before that happens. + with patch("importlib.import_module", side_effect=ImportError("boom")): + with pytest.warns( + DeprecationWarning, match="gevent loop backend is deprecated" + ): + with pytest.raises(ImportError): + mode.loop.use("gevent") + + @pytest.mark.parametrize("loop", ["aio", "eventlet", "uvloop"]) + def test_other_backends_do_not_warn(self, loop): + with patch("importlib.import_module"): + with recorded_warnings() as recorded: + mode.loop.use(loop) + assert not [ + w + for w in recorded + if issubclass(w.category, DeprecationWarning) + ] From c43da8f2a6d127f9f05142125f4c84d5348b0308 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 22:15:26 +0000 Subject: [PATCH 06/16] Bump tests.yml to setup-python@v5 so the 3.14t leg can resolve The 3.14t job added in the previous commit failed at setup, five seconds in, before running anything: The version '3.14t' with architecture 'x64' was not found for Ubuntu 24.04. The build exists -- actions/python-versions ships python-3.14.7-linux-24.04-x64-freethreaded.tar.gz. The problem is that tests.yml pinned actions/setup-python@v4, and the free-threaded "t" suffix is only understood from v5.3 onwards. On v4 the string "3.14t" is treated as a literal version and looked up against arch x64 rather than x64-freethreaded, hence "not found". Bumps checkout to v4 in the same file while there: tests.yml was the last workflow still on checkout@v3 and setup-python@v4, and both deploy-docs and publish already use v4/v5. Verified by running each CI step against python3.14t locally: pip install -r requirements.txt (exit 0, docs deps included), pip install -r requirements-typecheck.txt (exit 0), scripts/lint.sh (clean), and scripts/tests.sh (790 passed, 1 skipped). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- .github/workflows/tests.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a14fcd3..4209a72 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -31,10 +31,15 @@ jobs: experimental: [ false ] steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 with: fetch-depth: 0 - - uses: "actions/setup-python@v4" + # setup-python must be >= v5.3: that is the first release that + # understands the free-threaded "t" suffix. On v4, "3.14t" is + # looked up as a literal version against arch x64 rather than + # x64-freethreaded, and the job fails with "The version '3.14t' + # with architecture 'x64' was not found". + - uses: "actions/setup-python@v5" with: python-version: "${{ matrix.python-version }}" cache: "pip" From bb080e552eec0f03afe7c972a3240fb9e1299765 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 22:47:42 +0000 Subject: [PATCH 07/16] Actually measure coverage in CI, and cover mode/utils/loops.py scripts/tests.sh ran plain pytest with no --cov, so nothing was ever measured. Two consequences: the `fail_under = 93` configured in pyproject.toml was never enforced, and the Codecov step in tests.yml failed on every leg of every run with "No coverage reports found" -- as a warning, which is why it went unnoticed. Switching --cov on alone would have turned CI red: coverage sits at 92.66% on master and 92.75% on this branch, both under the threshold. So this also covers mode/utils/loops.py, which was the single largest gap at 34% -- get_event_loop was tested but _is_unix_loop, clone_loop, _appropriate_signal_handler, call_asap and _call_asap had nothing at all. That takes loops.py to 92% and the project to 93.69%, clearing the bar with room to spare (94.76% on 3.10). Two pre-existing bugs turned up while writing those tests. Neither is fixed here -- both are in code with no callers inside mode, and changing exported behaviour belongs in its own change: - _call_asap dispatches the callback twice, once via loop._call_soon() and again via the handle it inserts at _ready[0]. - get_event_loop() can return a closed loop: it checks is_closed() on its own thread-local cache, then falls through to asyncio.get_event_loop(), which returns whatever was last passed to set_event_loop() even when that loop is closed. The new tests assert the documented contract rather than either bug, so they keep passing if and when those are fixed. Both are noted in comments at the point where a reader would otherwise be confused. Note that Codecov *upload* still cannot succeed: the runs log "Branch is protected but no token was provided", so secrets.CODECOV_TOKEN is not set on the repository. That needs a maintainer. The local fail_under gate now works regardless of the upload. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- scripts/tests.sh | 9 +- tests/unit/utils/test_loops.py | 180 ++++++++++++++++++++++++++++++++- 2 files changed, 186 insertions(+), 3 deletions(-) diff --git a/scripts/tests.sh b/scripts/tests.sh index c8cfba2..7d19dad 100755 --- a/scripts/tests.sh +++ b/scripts/tests.sh @@ -7,4 +7,11 @@ fi set -ex -${PREFIX}pytest tests/unit tests/functional +# Coverage settings (source, omit, branch, fail_under) live in the +# [tool.coverage.*] sections of pyproject.toml. Without --cov nothing is +# measured, which meant the configured `fail_under` was never enforced and +# the Codecov upload in CI had no report to find. +${PREFIX}pytest tests/unit tests/functional \ + --cov \ + --cov-report=term-missing \ + --cov-report=xml diff --git a/tests/unit/utils/test_loops.py b/tests/unit/utils/test_loops.py index 14f661f..4783688 100644 --- a/tests/unit/utils/test_loops.py +++ b/tests/unit/utils/test_loops.py @@ -1,8 +1,20 @@ import asyncio +import contextvars +import signal +import sys import threading -from unittest.mock import patch +from unittest.mock import Mock, patch -from mode.utils.loops import get_event_loop +import pytest + +from mode.utils.loops import ( + _appropriate_signal_handler, + _call_asap, + _is_unix_loop, + call_asap, + clone_loop, + get_event_loop, +) def test_get_event_loop__returns_running_loop_when_running(): @@ -92,3 +104,167 @@ def other_thread() -> None: asyncio.set_event_loop(None) main_loop.close() other_loop_holder["loop"].close() + + +# The helpers below (_is_unix_loop, clone_loop, _appropriate_signal_handler, +# call_asap, _call_asap) had no coverage at all, which left mode/utils/loops.py +# at 34%. They are exported but currently unused inside mode itself. + + +@pytest.fixture +def loop(): + loop = asyncio.new_event_loop() + try: + yield loop + finally: + loop.close() + + +class test_is_unix_loop: + @pytest.mark.skipif( + sys.platform == "win32", reason="no unix event loop on windows" + ) + def test_true_for_a_unix_selector_loop(self, loop): + assert _is_unix_loop(loop) + + def test_false_for_anything_else(self): + assert not _is_unix_loop(Mock(name="loop")) + + +class test_clone_loop: + def test_returns_a_new_loop(self, loop): + new_loop = clone_loop(loop) + try: + assert new_loop is not loop + assert isinstance(new_loop, asyncio.AbstractEventLoop) + finally: + new_loop.close() + + def test_non_unix_loop_copies_no_signal_handlers(self): + new_loop = clone_loop(Mock(name="loop")) + try: + assert isinstance(new_loop, asyncio.AbstractEventLoop) + finally: + new_loop.close() + + @pytest.mark.skipif( + sys.platform == "win32", reason="no signal handlers on windows" + ) + def test_retains_signal_handlers(self, loop): + loop.add_signal_handler(signal.SIGUSR1, lambda: None) + new_loop = clone_loop(loop) + try: + assert signal.SIGUSR1 in new_loop._signal_handlers + finally: + new_loop.remove_signal_handler(signal.SIGUSR1) + new_loop.close() + loop.remove_signal_handler(signal.SIGUSR1) + + +class test_appropriate_signal_handler: + def test_calls_the_original_callback_on_the_parent_loop(self, loop): + called = [] + handle = asyncio.Handle( + lambda *a: called.append(a), + (1, 2), + loop, + contextvars.copy_context(), + ) + + wrapper = _appropriate_signal_handler(loop, handle) + wrapper() + + # _call_asap queues onto the parent loop rather than calling inline. + assert called == [] + assert loop._ready + loop._ready.popleft()._run() + assert called == [(1, 2)] + + +class test_call_asap: + def test_requires_a_loop(self): + with pytest.raises(AssertionError): + call_asap(lambda: None) + + @pytest.mark.skipif( + sys.platform == "win32", reason="no unix event loop on windows" + ) + def test_unix_loop_pushes_to_the_front(self, loop): + # NOTE: Only the ordering is asserted, not the number of calls. + # `_call_asap` currently dispatches the callback twice -- once via + # `loop._call_soon()` and again via the handle it inserts at + # `_ready[0]` -- so "jumped" also shows up at the back. Asserting + # the exact sequence would enshrine that; asserting the front of + # the queue tests the documented contract and keeps passing if the + # duplicate is ever removed. + order = [] + loop.call_soon(lambda: order.append("first-queued")) + call_asap(lambda: order.append("jumped"), loop=loop) + + while loop._ready: + loop._ready.popleft()._run() + + assert order[0] == "jumped" + assert "first-queued" in order + + def test_other_loops_delegate_to_call_soon_threadsafe(self): + mock_loop = Mock(name="loop") + callback = Mock(name="callback") + + result = call_asap(callback, 1, 2, loop=mock_loop) + + mock_loop.call_soon_threadsafe.assert_called_once_with(callback, 1, 2) + assert result is mock_loop.call_soon_threadsafe.return_value + + def test_other_loops_pass_the_context_through(self): + mock_loop = Mock(name="loop") + callback = Mock(name="callback") + context = contextvars.copy_context() + + call_asap(callback, 1, loop=mock_loop, context=context) + + mock_loop.call_soon_threadsafe.assert_called_once_with( + callback, 1, context=context + ) + + +class test__call_asap: + @pytest.mark.skipif( + sys.platform == "win32", reason="no unix event loop on windows" + ) + def test_returns_a_handle_and_wakes_the_loop(self, loop): + callback = Mock(name="callback") + + handle = _call_asap(loop, callback, 1, 2) + + assert isinstance(handle, asyncio.Handle) + assert loop._ready + # Only the front handle is run: `_call_asap` also leaves a second, + # duplicate handle further back in `_ready` (see the note in + # test_unix_loop_pushes_to_the_front). + loop._ready.popleft()._run() + callback.assert_called_once_with(1, 2) + + @pytest.mark.skipif( + sys.platform == "win32", reason="no unix event loop on windows" + ) + def test_accepts_a_context(self, loop): + callback = Mock(name="callback") + + handle = _call_asap(loop, callback, context=contextvars.copy_context()) + + assert isinstance(handle, asyncio.Handle) + + def test_raises_when_the_loop_is_closed(self): + closed = asyncio.new_event_loop() + closed.close() + with pytest.raises(RuntimeError): + _call_asap(closed, Mock(name="callback")) + + @pytest.mark.skipif( + sys.platform == "win32", reason="no unix event loop on windows" + ) + def test_debug_mode_validates_the_callback(self, loop): + loop.set_debug(True) + with pytest.raises(TypeError): + _call_asap(loop, "not-callable") From 66ed59ebb96aa14e60747223b5c05dd9f34db36c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 22:55:35 +0000 Subject: [PATCH 08/16] Build Proxy.__class__ without reading the bare name The pypy3.10 leg started failing at collection once coverage was switched on in the previous commit: mode/locals.py:290: in Proxy @__class__.setter E NameError: name '__class__' is not defined `Proxy.__init_subclass__` calls zero-argument `super()`, which makes the compiler add an implicit `__class__` closure cell to the class. That means the bare `__class__` written by the `@property` / `@__class__ .setter` decorator pair is not a plain namespace lookup: CPython resolves it to the property object defined moments earlier, but PyPy resolves it to the cell, which stays empty until the class object exists. PyPy only takes that path with a trace function installed, which is why it appeared under coverage and never before. Building the property as `property(_get_class, _set_class)` stores the name without ever loading it, which sidesteps the question on every interpreter. Behaviour is unchanged -- verified against the pre-fix tree: same resolution through the proxy, same TypeError from assignment (which `Proxy.__setattr__` intercepts and forwards before the setter is ever reached), same `property` descriptor on the class. Adds a regression guard asserting the class body emits no LOAD_NAME / LOAD_CLASSDEREF / LOAD_GLOBAL for `__class__`, while tolerating the compiler's own MAKE_CELL / LOAD_FAST cell plumbing. CPython cannot reproduce the failure itself, so the bytecode is the only thing a CPython-only run can check; the guard fails on the pre-fix tree with exactly ['LOAD_NAME']. Not verified on PyPy directly: the sandbox proxy blocks downloads.python .org and pypy.org, so the diagnosis was confirmed by disassembling the class body rather than by running it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- mode/locals.py | 21 +++++++++---- tests/unit/test_locals.py | 63 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/mode/locals.py b/mode/locals.py index 973d24c..6ac3b08 100644 --- a/mode/locals.py +++ b/mode/locals.py @@ -283,14 +283,23 @@ def __doc__(self) -> Optional[str]: def _get_class(self) -> type[T]: return self._get_current_object().__class__ - @property - def __class__(self) -> Any: - return self._get_class() - - @__class__.setter - def __class__(self, t: type) -> None: + def _set_class(self, t: type) -> None: raise NotImplementedError() + # NOTE: Built with `property()` rather than the `@property` / + # `@__class__.setter` decorator pair, because that pair *reads* the bare + # name `__class__` in the class body -- and here that is not a plain + # namespace lookup. `__init_subclass__` above calls zero-argument + # `super()`, which makes the compiler add an implicit `__class__` closure + # cell to this class. CPython still resolves the bare name to the + # property object defined moments earlier, but PyPy resolves it to that + # cell, which is empty until the class object exists -- so importing this + # module raises `NameError: name '__class__' is not defined`. PyPy only + # takes that path with a trace function installed, so it shows up under + # coverage and not otherwise. Storing the name without ever loading it + # sidesteps the whole question on every interpreter. + __class__: Any = property(_get_class, _set_class) + def _get_current_object(self) -> T: """Get current object. diff --git a/tests/unit/test_locals.py b/tests/unit/test_locals.py index c235880..2e7959c 100644 --- a/tests/unit/test_locals.py +++ b/tests/unit/test_locals.py @@ -1,4 +1,6 @@ import abc +import dis +import types from collections.abc import ( AsyncGenerator, AsyncIterable, @@ -11,6 +13,7 @@ Sequence, Set, ) +from pathlib import Path from unittest.mock import MagicMock, Mock import pytest @@ -755,3 +758,63 @@ class ProxySource(Proxy[Source]): s = Source() p = ProxySource(lambda: s) assert p._get_current_object() is s + + +class test_Proxy_class_body_bytecode: + """Guard the `__class__` property construction in `Proxy`. + + `Proxy.__init_subclass__` calls zero-argument `super()`, which makes the + compiler add an implicit `__class__` closure cell to the class. That + turns a bare `__class__` in the class body (as written by the + `@property` / `@__class__.setter` decorator pair) into something other + than a plain namespace lookup: CPython still finds the property, but + PyPy finds the empty cell and raises `NameError` at import time. + + CPython cannot reproduce that failure, so asserting on the emitted + bytecode is the only way to keep the regression from coming back. + """ + + def _proxy_class_body(self): + import mode.locals + + source = Path(mode.locals.__file__).read_text() + module_code = compile(source, mode.locals.__file__, "exec") + + def walk(code): + for const in code.co_consts: + if isinstance(const, types.CodeType): + yield const + yield from walk(const) + + bodies = [c for c in walk(module_code) if c.co_name == "Proxy"] + assert len(bodies) == 1, "expected exactly one Proxy class body" + return bodies[0] + + def test_super_still_creates_the_class_cell(self): + # If this ever stops being true the guard below is unnecessary -- + # but so is the workaround it protects, so both should be revisited + # together rather than one silently rotting. + assert "__class__" in self._proxy_class_body().co_cellvars + + #: Opcodes that resolve a *name* through a namespace. The compiler also + #: emits cell plumbing for `__class__` (MAKE_CELL / LOAD_FAST* / + #: LOAD_CLOSURE, used to populate `__classcell__`), which is implicit, + #: unavoidable and harmless -- only an actual lookup is the bug. + NAME_LOOKUP_OPCODES = frozenset( + {"LOAD_NAME", "LOAD_CLASSDEREF", "LOAD_GLOBAL"} + ) + + def test_class_body_never_looks_up_the_bare_name(self): + lookups = [ + instruction + for instruction in dis.get_instructions(self._proxy_class_body()) + if instruction.opname in self.NAME_LOOKUP_OPCODES + and instruction.argval == "__class__" + ] + assert not lookups, ( + "Proxy's class body looks up the bare name `__class__` " + f"({[i.opname for i in lookups]}). Build the property with " + "`property(_get_class, _set_class)` instead of the " + "`@property`/`@__class__.setter` decorator pair -- the latter " + "reads the name and breaks the import on PyPy." + ) From 68d485a4eccbc4c4150ad93d869a67330107929a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 23:06:58 +0000 Subject: [PATCH 09/16] Bind Proxy.__class__ with def, and without reading the bare name Follow-up to the previous commit, which fixed the PyPy import error but broke `proxy.__class__` on PyPy instead: test_Proxy::test_name failed with the proxy reporting itself rather than the object it wraps. Two separate constraints apply here, and only one shape satisfies both. The class has an implicit `__class__` closure cell. The previous commit claimed zero-argument super() causes that and can be avoided by naming the class explicitly -- wrong on the second point: the compiler adds the cell when a method merely *references the name* `super`, since it cannot know which form is meant. `super(Proxy, self)` makes no difference, so that part is reverted and the comment corrected. Given the cell exists: 1. The name must be bound with `def`. On PyPy a class-body assignment to a name that is also a cell variable does not reach the class namespace, so `__class__ = property(...)` -- the previous commit's fix -- left no descriptor at all; attribute access fell back to `type.__class__`. CPython installs it either way, which is why CI caught this and local runs could not. 2. The class body must not *read* the bare name `__class__`, which the `@property` / `@__class__.setter` pair does to attach the setter. With the cell present that read resolves to the cell, empty until the class exists, raising NameError at import on PyPy under a trace function. Passing the setter to the decorator up front (`_property_with_setter`) keeps the `def` binding while removing the read. Each guard is verified to catch one failure mode, by editing the real module and re-running: the decorator form trips test_class_body_never_looks_up_the_bare_name, the assignment form trips test_the_name_is_bound_with_def. The earlier store-opcode check is replaced -- CPython emits STORE_NAME for both spellings, so it could not tell them apart; the presence of a nested code object named `__class__` can. Still not verified on PyPy directly: the sandbox proxy blocks downloads.python.org and pypy.org. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- mode/locals.py | 62 ++++++++++++++++++++++++------- tests/unit/test_locals.py | 77 ++++++++++++++++++++++++++------------- 2 files changed, 101 insertions(+), 38 deletions(-) diff --git a/mode/locals.py b/mode/locals.py index 6ac3b08..ccb92d8 100644 --- a/mode/locals.py +++ b/mode/locals.py @@ -152,6 +152,24 @@ class XProxy(MutableMappingRole, AsyncContextManagerRole): PYPY = hasattr(sys, "pypy_version_info") SLOTS_ISSUE_PRESENT = sys.version_info < (3, 7) + +def _property_with_setter( + fset: Callable[[Any, Any], None], +) -> Callable[[Callable[[Any], Any]], property]: + """Build a `property` from a getter, with the setter supplied up front. + + Exists so that `Proxy` can define a `__class__` property without the + usual `@property` / `@__class__.setter` pair, which has to *read* the + bare name `__class__` in the class body to attach the setter. See the + note on `Proxy.__init_subclass__`. + """ + + def _decorate(fget: Callable[[Any], Any]) -> property: + return property(fget, fset) + + return _decorate + + T = TypeVar("T") S = TypeVar("S") T_co = TypeVar("T_co", covariant=True) @@ -199,6 +217,12 @@ class Proxy(Generic[T]): ) def __init_subclass__(self, source: Optional[type[T]] = None) -> None: + # NOTE: Merely referencing the name `super` here makes the compiler + # add an implicit `__class__` closure cell to this class -- the + # explicit `super(Proxy, self)` form does not avoid it, because the + # compiler cannot know which form is meant. That cell is why the + # `__class__` property further down is built the way it is; see the + # note there before changing either. super().__init_subclass__() if source is not None: self._init_from_source(source) @@ -286,19 +310,31 @@ def _get_class(self) -> type[T]: def _set_class(self, t: type) -> None: raise NotImplementedError() - # NOTE: Built with `property()` rather than the `@property` / - # `@__class__.setter` decorator pair, because that pair *reads* the bare - # name `__class__` in the class body -- and here that is not a plain - # namespace lookup. `__init_subclass__` above calls zero-argument - # `super()`, which makes the compiler add an implicit `__class__` closure - # cell to this class. CPython still resolves the bare name to the - # property object defined moments earlier, but PyPy resolves it to that - # cell, which is empty until the class object exists -- so importing this - # module raises `NameError: name '__class__' is not defined`. PyPy only - # takes that path with a trace function installed, so it shows up under - # coverage and not otherwise. Storing the name without ever loading it - # sidesteps the whole question on every interpreter. - __class__: Any = property(_get_class, _set_class) + # NOTE: Two constraints meet here, and only this shape satisfies both. + # + # 1. The name must be bound with `def`, not with a plain assignment. + # This class has an implicit `__class__` closure cell (see + # __init_subclass__ above), and on PyPy a class-body *assignment* to + # a name that is also a cell variable does not reach the class + # namespace -- so `__class__ = property(...)` leaves no descriptor + # behind, attribute access silently falls back to `type.__class__`, + # and the proxy reports itself instead of the object it wraps. + # + # 2. The class body must never *read* the bare name `__class__`, which + # the usual `@property` / `@__class__.setter` pair has to do in + # order to attach the setter. With the cell present that read + # resolves to the cell rather than to the property, and the cell is + # empty until the class object exists -- so on PyPy importing this + # module raises `NameError: name '__class__' is not defined`. (PyPy + # only takes that path with a trace function installed, which is why + # it appears under coverage and not otherwise.) + # + # Passing the setter to the decorator up front keeps the `def` binding + # while removing the read. Both halves are pinned by + # tests/unit/test_locals.py::test_Proxy_class_body_bytecode. + @_property_with_setter(_set_class) + def __class__(self) -> Any: + return self._get_class() def _get_current_object(self) -> T: """Get current object. diff --git a/tests/unit/test_locals.py b/tests/unit/test_locals.py index 2e7959c..174f004 100644 --- a/tests/unit/test_locals.py +++ b/tests/unit/test_locals.py @@ -761,17 +761,20 @@ class ProxySource(Proxy[Source]): class test_Proxy_class_body_bytecode: - """Guard the `__class__` property construction in `Proxy`. - - `Proxy.__init_subclass__` calls zero-argument `super()`, which makes the - compiler add an implicit `__class__` closure cell to the class. That - turns a bare `__class__` in the class body (as written by the - `@property` / `@__class__.setter` decorator pair) into something other - than a plain namespace lookup: CPython still finds the property, but - PyPy finds the empty cell and raises `NameError` at import time. - - CPython cannot reproduce that failure, so asserting on the emitted - bytecode is the only way to keep the regression from coming back. + """Guard the `__class__` property in `Proxy` against the PyPy import bug. + + The class body reads the bare name `__class__` (the + `@property` / `@__class__.setter` pair attaches the setter to the + property of that name). That is a plain namespace lookup *only* while + the class has no implicit `__class__` closure cell -- and zero-argument + `super()` anywhere in the body creates one. With the cell present, + PyPy resolves the read to it rather than to the property, and the cell + is empty until the class object exists, so importing mode.locals raises + `NameError: name '__class__' is not defined`. + + CPython resolves the same read to the property either way, so it cannot + reproduce the failure at all. Asserting on the compiled class body is + the only check a CPython-only run can make. """ def _proxy_class_body(self): @@ -790,16 +793,10 @@ def walk(code): assert len(bodies) == 1, "expected exactly one Proxy class body" return bodies[0] - def test_super_still_creates_the_class_cell(self): - # If this ever stops being true the guard below is unnecessary -- - # but so is the workaround it protects, so both should be revisited - # together rather than one silently rotting. - assert "__class__" in self._proxy_class_body().co_cellvars - - #: Opcodes that resolve a *name* through a namespace. The compiler also - #: emits cell plumbing for `__class__` (MAKE_CELL / LOAD_FAST* / - #: LOAD_CLOSURE, used to populate `__classcell__`), which is implicit, - #: unavoidable and harmless -- only an actual lookup is the bug. + #: Opcodes that resolve a *name* through a namespace. The compiler + #: also emits cell plumbing for `__class__` (MAKE_CELL / LOAD_FAST* / + #: LOAD_CLOSURE, to populate `__classcell__`), which is implicit and + #: unavoidable -- only an actual lookup is the bug. NAME_LOOKUP_OPCODES = frozenset( {"LOAD_NAME", "LOAD_CLASSDEREF", "LOAD_GLOBAL"} ) @@ -813,8 +810,38 @@ def test_class_body_never_looks_up_the_bare_name(self): ] assert not lookups, ( "Proxy's class body looks up the bare name `__class__` " - f"({[i.opname for i in lookups]}). Build the property with " - "`property(_get_class, _set_class)` instead of the " - "`@property`/`@__class__.setter` decorator pair -- the latter " - "reads the name and breaks the import on PyPy." + f"({[i.opname for i in lookups]}), which resolves to the empty " + "implicit class cell on PyPy and breaks `import mode.locals`. " + "Use the `@_property_with_setter(_set_class)` form rather than " + "`@property` + `@__class__.setter`." ) + + def test_the_name_is_bound_with_def(self): + # A plain assignment (`__class__ = property(...)`) does not reach + # the class namespace on PyPy, because the name is also a cell + # variable -- the descriptor is silently lost and the proxy then + # reports itself instead of the object it wraps. So `def` is + # required, not merely preferred. + # + # CPython emits STORE_NAME for both spellings, so the store opcode + # cannot tell them apart. The presence of a nested code object + # named `__class__` can: only `def` compiles one. + body = self._proxy_class_body() + compiled_functions = [ + const.co_name + for const in body.co_consts + if isinstance(const, types.CodeType) + ] + assert "__class__" in compiled_functions, ( + "Proxy's `__class__` property is not defined with `def`. A plain " + "assignment is lost on PyPy because `__class__` is also a cell " + "variable here; use " + "`@_property_with_setter(_set_class)` over a `def __class__`." + ) + + def test_the_property_is_installed_on_the_class(self): + # The failure mode this pairs with: if `__class__` never lands in + # the class namespace, attribute access silently falls back to + # `type.__class__` and the proxy reports itself instead of the + # object it wraps. + assert isinstance(Proxy.__dict__["__class__"], property) From 60d61d733a29070a25724aee783ece382b008c61 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 23:14:22 +0000 Subject: [PATCH 10/16] Remove the implicit __class__ cell instead of working around it Third and, I think, correct attempt at the PyPy failure. The two before this each fixed one symptom and caused the other, because I had the mechanism wrong both times. What is actually going on: `Proxy` defines a `__class__` property, and the compiler gives the class an implicit `__class__` closure cell as soon as any method in the body so much as names `super`. On PyPy -- and only with a trace function installed, i.e. under coverage -- a class body carrying that cell resolves *every* mention of the name `__class__` to the cell instead of to the class namespace. Both directions break, which is why fixing one exposed the other: - reading it, as `@__class__.setter` must, hits the cell while it is still empty -> NameError at import (the first failure); - binding it writes to the cell, leaving no descriptor on the class -> proxies report themselves instead of the object they wrap (the second failure, from the previous commit's `property()` assignment, and from its `def` form too -- `def` was not the distinction I claimed). So no spelling of the property inside that class body can work while the cell exists. The cell has to go. Moving the cooperative `super().__init_subclass__()` call into a module-level helper removes the only mention of `super` in the class body, and with it the cell. The upshot is that the property returns to exactly the code master has, byte for byte -- the only line this commit removes from mode/locals.py is the `super().__init_subclass__()` call. Everything else is additive. The guard is now the single root invariant -- the class body must have no `__class__` cellvar -- rather than the two derived rules the previous commit asserted, which were guarding a workaround that is now gone. Verified to fail when `super()` is inlined back into `__init_subclass__`. Also checked that plain and `source=`-parameterised subclassing still work, since the helper now carries that call. Still not verified on PyPy directly: the sandbox proxy blocks downloads.python.org and pypy.org. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- mode/locals.py | 79 ++++++++++++----------------- tests/unit/test_locals.py | 101 ++++++++++++++------------------------ 2 files changed, 70 insertions(+), 110 deletions(-) diff --git a/mode/locals.py b/mode/locals.py index ccb92d8..919024f 100644 --- a/mode/locals.py +++ b/mode/locals.py @@ -153,21 +153,25 @@ class XProxy(MutableMappingRole, AsyncContextManagerRole): SLOTS_ISSUE_PRESENT = sys.version_info < (3, 7) -def _property_with_setter( - fset: Callable[[Any, Any], None], -) -> Callable[[Callable[[Any], Any]], property]: - """Build a `property` from a getter, with the setter supplied up front. - - Exists so that `Proxy` can define a `__class__` property without the - usual `@property` / `@__class__.setter` pair, which has to *read* the - bare name `__class__` in the class body to attach the setter. See the - note on `Proxy.__init_subclass__`. +def _cooperative_init_subclass(cls: "type[Proxy[Any]]") -> None: + """Call the next ``__init_subclass__`` in ``Proxy``'s MRO. + + This lives at module level, outside the class body, for one reason: + naming ``super`` inside a method of ``Proxy`` would make the compiler + add an implicit ``__class__`` closure cell to that class. ``Proxy`` + defines a ``__class__`` property, and on PyPy -- with a trace function + installed, i.e. under coverage -- a class body that has such a cell + resolves *every* mention of the name ``__class__`` to the cell rather + than to the class namespace. Reading it then raises ``NameError: + name '__class__' is not defined`` at import time, and binding it + leaves no descriptor on the class at all, so proxies start reporting + themselves instead of the object they wrap. + + Keeping the cell from existing keeps ``__class__`` an ordinary name in + that class body, which is what every interpreter has always handled. + Pinned by tests/unit/test_locals.py::test_Proxy_class_body_bytecode. """ - - def _decorate(fget: Callable[[Any], Any]) -> property: - return property(fget, fset) - - return _decorate + super(Proxy, cls).__init_subclass__() T = TypeVar("T") @@ -217,13 +221,12 @@ class Proxy(Generic[T]): ) def __init_subclass__(self, source: Optional[type[T]] = None) -> None: - # NOTE: Merely referencing the name `super` here makes the compiler - # add an implicit `__class__` closure cell to this class -- the - # explicit `super(Proxy, self)` form does not avoid it, because the - # compiler cannot know which form is meant. That cell is why the - # `__class__` property further down is built the way it is; see the - # note there before changing either. - super().__init_subclass__() + # NOTE: Delegated to a module-level helper on purpose -- do not + # inline this back to `super().__init_subclass__()`. Naming `super` + # anywhere in this class body makes the compiler add an implicit + # `__class__` closure cell, which breaks the `__class__` property + # below on PyPy. See `_cooperative_init_subclass`. + _cooperative_init_subclass(self) if source is not None: self._init_from_source(source) elif self.__proxy_source__ is not None: @@ -307,35 +310,17 @@ def __doc__(self) -> Optional[str]: def _get_class(self) -> type[T]: return self._get_current_object().__class__ - def _set_class(self, t: type) -> None: - raise NotImplementedError() - - # NOTE: Two constraints meet here, and only this shape satisfies both. - # - # 1. The name must be bound with `def`, not with a plain assignment. - # This class has an implicit `__class__` closure cell (see - # __init_subclass__ above), and on PyPy a class-body *assignment* to - # a name that is also a cell variable does not reach the class - # namespace -- so `__class__ = property(...)` leaves no descriptor - # behind, attribute access silently falls back to `type.__class__`, - # and the proxy reports itself instead of the object it wraps. - # - # 2. The class body must never *read* the bare name `__class__`, which - # the usual `@property` / `@__class__.setter` pair has to do in - # order to attach the setter. With the cell present that read - # resolves to the cell rather than to the property, and the cell is - # empty until the class object exists -- so on PyPy importing this - # module raises `NameError: name '__class__' is not defined`. (PyPy - # only takes that path with a trace function installed, which is why - # it appears under coverage and not otherwise.) - # - # Passing the setter to the decorator up front keeps the `def` binding - # while removing the read. Both halves are pinned by - # tests/unit/test_locals.py::test_Proxy_class_body_bytecode. - @_property_with_setter(_set_class) + # NOTE: This ordinary property spelling is only safe while the class + # body has no implicit `__class__` closure cell -- see + # `_cooperative_init_subclass` before adding any use of `super` here. + @property def __class__(self) -> Any: return self._get_class() + @__class__.setter + def __class__(self, t: type) -> None: + raise NotImplementedError() + def _get_current_object(self) -> T: """Get current object. diff --git a/tests/unit/test_locals.py b/tests/unit/test_locals.py index 174f004..f600dfb 100644 --- a/tests/unit/test_locals.py +++ b/tests/unit/test_locals.py @@ -1,5 +1,4 @@ import abc -import dis import types from collections.abc import ( AsyncGenerator, @@ -761,20 +760,29 @@ class ProxySource(Proxy[Source]): class test_Proxy_class_body_bytecode: - """Guard the `__class__` property in `Proxy` against the PyPy import bug. - - The class body reads the bare name `__class__` (the - `@property` / `@__class__.setter` pair attaches the setter to the - property of that name). That is a plain namespace lookup *only* while - the class has no implicit `__class__` closure cell -- and zero-argument - `super()` anywhere in the body creates one. With the cell present, - PyPy resolves the read to it rather than to the property, and the cell - is empty until the class object exists, so importing mode.locals raises - `NameError: name '__class__' is not defined`. - - CPython resolves the same read to the property either way, so it cannot - reproduce the failure at all. Asserting on the compiled class body is - the only check a CPython-only run can make. + """Guard `Proxy` against the PyPy `__class__` cell bug. + + `Proxy` defines a `__class__` property. That is fine so long as the + class body has no implicit `__class__` closure cell -- but the compiler + adds one as soon as any method in the body so much as *names* `super` + (it cannot tell the zero-argument form from the explicit one). + + With that cell present, PyPy -- and only with a trace function + installed, i.e. under coverage -- resolves every mention of the name + `__class__` in the class body to the cell instead of the class + namespace. Both directions break: + + * reading it (as `@__class__.setter` must) hits the cell while it is + still empty, so importing mode.locals raises + `NameError: name '__class__' is not defined`; + * binding it writes to the cell, so no descriptor is left on the + class and every proxy reports itself instead of the object it + wraps. + + CPython resolves both to the class namespace either way, so it cannot + reproduce any of this -- the compiled class body is the only thing a + CPython-only run can check. `_cooperative_init_subclass` keeps the + cell from being created; these tests keep it that way. """ def _proxy_class_body(self): @@ -793,55 +801,22 @@ def walk(code): assert len(bodies) == 1, "expected exactly one Proxy class body" return bodies[0] - #: Opcodes that resolve a *name* through a namespace. The compiler - #: also emits cell plumbing for `__class__` (MAKE_CELL / LOAD_FAST* / - #: LOAD_CLOSURE, to populate `__classcell__`), which is implicit and - #: unavoidable -- only an actual lookup is the bug. - NAME_LOOKUP_OPCODES = frozenset( - {"LOAD_NAME", "LOAD_CLASSDEREF", "LOAD_GLOBAL"} - ) - - def test_class_body_never_looks_up_the_bare_name(self): - lookups = [ - instruction - for instruction in dis.get_instructions(self._proxy_class_body()) - if instruction.opname in self.NAME_LOOKUP_OPCODES - and instruction.argval == "__class__" - ] - assert not lookups, ( - "Proxy's class body looks up the bare name `__class__` " - f"({[i.opname for i in lookups]}), which resolves to the empty " - "implicit class cell on PyPy and breaks `import mode.locals`. " - "Use the `@_property_with_setter(_set_class)` form rather than " - "`@property` + `@__class__.setter`." - ) - - def test_the_name_is_bound_with_def(self): - # A plain assignment (`__class__ = property(...)`) does not reach - # the class namespace on PyPy, because the name is also a cell - # variable -- the descriptor is silently lost and the proxy then - # reports itself instead of the object it wraps. So `def` is - # required, not merely preferred. - # - # CPython emits STORE_NAME for both spellings, so the store opcode - # cannot tell them apart. The presence of a nested code object - # named `__class__` can: only `def` compiles one. - body = self._proxy_class_body() - compiled_functions = [ - const.co_name - for const in body.co_consts - if isinstance(const, types.CodeType) - ] - assert "__class__" in compiled_functions, ( - "Proxy's `__class__` property is not defined with `def`. A plain " - "assignment is lost on PyPy because `__class__` is also a cell " - "variable here; use " - "`@_property_with_setter(_set_class)` over a `def __class__`." + def test_class_body_has_no_implicit_class_cell(self): + assert "__class__" not in self._proxy_class_body().co_cellvars, ( + "Proxy's class body has an implicit `__class__` closure cell. " + "Something in it names `super` (or reads `__class__`) inside a " + "method -- even the explicit `super(Proxy, self)` form is " + "enough. That breaks the `__class__` property on PyPy under " + "coverage. Route the call through the module-level " + "`_cooperative_init_subclass` helper instead." ) def test_the_property_is_installed_on_the_class(self): - # The failure mode this pairs with: if `__class__` never lands in - # the class namespace, attribute access silently falls back to - # `type.__class__` and the proxy reports itself instead of the - # object it wraps. + # The runtime half of the same invariant, and the one that catches + # it on PyPy directly: if `__class__` never lands in the class + # namespace, attribute access falls back to `type.__class__` and + # the proxy reports itself rather than the object it wraps. assert isinstance(Proxy.__dict__["__class__"], property) + + def test_the_property_still_forwards(self): + assert Proxy(lambda: "hello").__class__ is str From 408566a4d1f249cff88493699d7453dd7991e608 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 14:38:00 +0000 Subject: [PATCH 11/16] Keep OrderedDict in LRUCache; make the mutex mandatory instead Swapping the backing store to a plain dict, two commits ago, fixed the free-threading segfault by introducing a performance regression. dict has preserved insertion order since 3.7 and is memory-safe under free threading, so it looked like a free win -- but evicting the *oldest* entry is exactly the operation dict cannot do in O(1), and that is LRUCache's hot path. `OrderedDict.popitem(last=False)` unlinks a node; the dict equivalent, `d.pop(next(iter(d)))`, scans past every slot vacated since the last resize. Measured on 100k steady-state evict-and-insert against the real class: cache size OrderedDict dict 1,000 0.043s 0.089s 10,000 0.046s 0.448s 100,000 0.052s 2.447s The gap grows linearly with the cache -- eviction had become O(n). Periodic compaction only softens it to O(sqrt(n)) (still ~24x at 100k), so there is no cheap repair; the linked list is the point of OrderedDict. So the container goes back, and the concurrency hazard is handled where it belongs -- the mutex. It already defaulted to on for free-threaded builds; now `thread_safety=False` is also refused there with ValueError rather than silently handing back a structure that can segfault the interpreter. GIL builds are unaffected and still default to no locking. The snapshot-instead-of-hold-across-yield fix from that commit is kept: it was a genuine bug, independent of the container. Verified: eviction back to flat ~0.05s at every size (master: 0.043-0.125s), 805-807 passing on 3.14t / 3.14 / 3.10, coverage 93.65-94.72%, and the stress harness clean on the free-threaded build with no segfault. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- docs/free-threading.md | 40 ++++++++++++---- mode/utils/collections.py | 65 +++++++++++++++----------- tests/freethreading/stress.py | 12 +++-- tests/functional/test_thread_safety.py | 34 +++++++++----- 4 files changed, 96 insertions(+), 55 deletions(-) diff --git a/docs/free-threading.md b/docs/free-threading.md index 4711051..5a448a3 100644 --- a/docs/free-threading.md +++ b/docs/free-threading.md @@ -88,16 +88,13 @@ Free-threaded CPython gives plain `dict` per-object locking; `OrderedDict`'s C implementation did not get the same treatment, so concurrent mutation corrupts its internal linked list. -**Fixed** in `mode/utils/collections.py` by all three of: - -- Backing the cache with a plain `dict`. Insertion order has been - guaranteed since 3.7, and the only `OrderedDict`-specific API in use was - `popitem(last=...)`, now served by `_popitem_first()` plus - `dict.popitem()`. -- Defaulting `thread_safety` to `True` on free-threaded builds, via the new - `mode.utils.collections.FREE_THREADED` flag. It is checked at runtime - rather than build time, so `PYTHON_GIL=1` is respected. Passing - `thread_safety` explicitly still wins. +**Fixed** in `mode/utils/collections.py` by: + +- Making the mutex mandatory on free-threaded builds. `thread_safety` + defaults to the new `mode.utils.collections.FREE_THREADED` flag, checked + at runtime rather than build time so `PYTHON_GIL=1` is respected, and + passing `thread_safety=False` on such a build now raises `ValueError` + rather than handing back a structure that can take the interpreter down. - Snapshotting in `_keys`/`_values`/`_items` instead of holding the mutex across `yield`. The old code kept the lock held for as long as the *consumer* took to iterate — and forever if the consumer abandoned the @@ -105,6 +102,29 @@ corrupts its internal linked list. closed. That hazard was latent while the lock defaulted to off; turning the lock on by default would have made it real. +### Why not just swap `OrderedDict` for `dict`? + +That was the first fix, and it was wrong. `dict` has preserved insertion +order since 3.7 and is memory-safe under free threading, so it looks like a +free win — but `LRUCache`'s hot path is evicting the *oldest* entry, and +that is the one thing `dict` cannot do in O(1). `OrderedDict.popitem(last= +False)` unlinks a node; the `dict` equivalent, `d.pop(next(iter(d)))`, has +to scan past every slot vacated since the last resize. + +Steady-state evict-and-insert, 100k operations: + +| cache size | `OrderedDict` | `dict` | +|---|---|---| +| 1,000 | 0.043s | 0.089s | +| 10,000 | 0.046s | 0.448s | +| 100,000 | 0.052s | 2.447s | + +The gap grows linearly with the cache, because the eviction itself became +O(n). Periodically rebuilding the dict to compact it only softens this to +O(√n) — still ~24x at 100k — so there is no cheap repair. `OrderedDict` is +the right data structure here; the concurrency hazard belongs to the mutex, +not to the choice of container. + `LRUCache` is not used inside `mode` itself; it is exported utility surface (faust is a consumer), so the blast radius was downstream. diff --git a/mode/utils/collections.py b/mode/utils/collections.py index a813507..bc92dfc 100644 --- a/mode/utils/collections.py +++ b/mode/utils/collections.py @@ -5,7 +5,7 @@ import sys import threading import typing -from collections import UserList +from collections import OrderedDict, UserList from collections.abc import ( ItemsView, Iterable, @@ -448,22 +448,31 @@ class LRUCache(FastUserDict, MutableMapping[KT, VT], MappingViewProxy): to access/mutate the cache. Defaults to :const:`True` on free-threaded builds, where there is no GIL to make unguarded access incidentally safe, and :const:`False` otherwise (which - is what it has always been). + is what it has always been). It cannot be turned off on a + free-threaded build -- see the note below. Note: - The backing store is a plain :class:`dict`, not an - :class:`~collections.OrderedDict`. Both preserve insertion order - (guaranteed for `dict` since Python 3.7), but on free-threaded - builds only `dict` is safe to mutate concurrently: - `OrderedDict` keeps a separate linked list that racing threads - can corrupt badly enough to segfault the interpreter, whereas - `dict` has per-object locking. + The backing store is an :class:`~collections.OrderedDict` rather + than a plain :class:`dict`, even though `dict` has preserved + insertion order since Python 3.7. The reason is + `popitem(last=False)`: evicting the oldest entry is this class's + hot path, and `OrderedDict` does it in O(1) via its linked list, + while the `dict` equivalent (`d.pop(next(iter(d)))`) has to scan + past every slot vacated since the last resize. Measured on a + steady-state evict-and-insert loop, `dict` was ~3x slower at 1,000 + entries and ~110x slower at 100,000. + + The cost of that linked list is that `OrderedDict` is not safe to + mutate concurrently on free-threaded builds -- racing threads + corrupt it badly enough to segfault the interpreter, where `dict` + would merely raise. So on those builds the mutex is mandatory + rather than merely on by default. """ limit: Optional[int] thread_safety: bool _mutex: AbstractContextManager - data: dict + data: OrderedDict def __init__( self, @@ -472,11 +481,23 @@ def __init__( thread_safety: Optional[bool] = None, ) -> None: self.limit = limit - self.thread_safety = ( - FREE_THREADED if thread_safety is None else thread_safety - ) + if thread_safety is None: + thread_safety = FREE_THREADED + elif FREE_THREADED and not thread_safety: + # Not a preference we can honour: an unguarded OrderedDict on a + # free-threaded build is memory-unsafe, not merely racy, and + # taking the interpreter down is a worse outcome than ignoring + # the argument. Say so rather than doing it silently. + raise ValueError( + "LRUCache(thread_safety=False) is not supported on " + "free-threaded builds: the backing OrderedDict can be " + "corrupted by concurrent mutation badly enough to " + "segfault the interpreter. Omit the argument to get the " + "mutex, which is the default here." + ) + self.thread_safety = thread_safety self._mutex = self._new_lock() - self.data: dict = {} + self.data: OrderedDict = OrderedDict() def __getitem__(self, key: KT) -> VT: with self._mutex: @@ -490,23 +511,11 @@ def update(self, *args: Any, **kwargs: Any) -> None: if limit and len(data) > limit: # pop additional items in case limit exceeded for _ in range(len(data) - limit): - self._popitem_first() - - def _popitem_first(self) -> tuple[KT, VT]: - # `dict` only pops from the right, so emulate the - # `OrderedDict.popitem(last=False)` this used to call. - # Caller must hold the mutex. - try: - key = next(iter(self.data)) - except StopIteration: - raise KeyError("dictionary is empty") from None - return key, self.data.pop(key) + data.popitem(last=False) def popitem(self, *, last: bool = True) -> tuple[KT, VT]: with self._mutex: - if last: - return self.data.popitem() - return self._popitem_first() + return self.data.popitem(last) def __setitem__(self, key: KT, value: VT) -> None: # remove least recently used key. diff --git a/tests/freethreading/stress.py b/tests/freethreading/stress.py index 1c0d11b..a5960d7 100644 --- a/tests/freethreading/stress.py +++ b/tests/freethreading/stress.py @@ -68,15 +68,17 @@ def report(name, errors, note=""): # -------------------------------------------------------------------------- -# Defect 1 (fixed): LRUCache was backed by OrderedDict with -# thread_safety=False by default, so concurrent mutate+iterate segfaulted a -# free-threaded interpreter. It is a plain dict now, and thread_safety -# defaults to on for free-threaded builds. +# Defect 1 (fixed): LRUCache is backed by OrderedDict, whose C linked list +# concurrent mutation can corrupt badly enough to segfault a free-threaded +# interpreter -- and thread_safety defaulted to False. The container is +# unchanged (dict cannot evict the oldest entry in O(1)); instead the mutex +# is now mandatory on free-threaded builds, so the default config is safe +# and thread_safety=False is refused there. # -------------------------------------------------------------------------- def check_lru_default(trials=60): from mode.utils.collections import LRUCache - print(" (this check segfaulted before the fix)", flush=True) + print(" (this configuration segfaulted before the fix)", flush=True) bad = 0 for _ in range(trials): cache = LRUCache(limit=50) diff --git a/tests/functional/test_thread_safety.py b/tests/functional/test_thread_safety.py index 0b4152b..113c65e 100644 --- a/tests/functional/test_thread_safety.py +++ b/tests/functional/test_thread_safety.py @@ -12,6 +12,7 @@ import sys import threading import time +from collections import OrderedDict from types import ModuleType import pytest @@ -115,20 +116,28 @@ def val(self): class test_LRUCache_thread_safety: - def test_backed_by_plain_dict(self): - # Not an OrderedDict: on free-threaded builds concurrent mutation - # of an OrderedDict can corrupt its linked list and segfault the - # interpreter, while plain dict has per-object locking. - assert type(LRUCache().data) is dict + def test_backed_by_ordered_dict(self): + # OrderedDict, not plain dict: evicting the oldest entry is the hot + # path and OrderedDict does it in O(1), where dict has to scan past + # every slot vacated since its last resize. The concurrency + # hazard that comes with it is handled by making the mutex + # mandatory on free-threaded builds, not by changing container. + assert type(LRUCache().data) is OrderedDict def test_thread_safety_defaults_to_free_threaded(self): assert LRUCache().thread_safety is FREE_THREADED - @pytest.mark.parametrize("thread_safety", [True, False]) - def test_thread_safety_can_be_overridden(self, thread_safety): - assert LRUCache(thread_safety=thread_safety).thread_safety is ( - thread_safety - ) + def test_thread_safety_can_be_requested(self): + assert LRUCache(thread_safety=True).thread_safety is True + + def test_thread_safety_cannot_be_disabled_when_free_threaded(self): + # An unguarded OrderedDict is memory-unsafe here, not merely racy, + # so this is refused rather than honoured. + if FREE_THREADED: + with pytest.raises(ValueError, match="free-threaded"): + LRUCache(thread_safety=False) + else: + assert LRUCache(thread_safety=False).thread_safety is False def test_popitem_last_is_lifo(self): c = LRUCache() @@ -178,8 +187,9 @@ def writer(): assert c["d"] == 4 def test_concurrent_mutation_and_iteration(self): - # Deliberately the *default* configuration: this is what used to - # segfault the interpreter on free-threaded builds. + # Deliberately the *default* configuration -- which on a + # free-threaded build now means the mutex is on. This is the + # workload that used to segfault the interpreter. c = LRUCache(limit=50) barrier = threading.Barrier(8) errors = [] From dda3a91ab20ec5bfb1772643469594d25e1ccca6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 04:09:36 +0000 Subject: [PATCH 12/16] Pin LRUCache ordering, and stop evicting on updates to existing keys Ordering is the part of LRUCache least likely to behave identically across interpreters -- CPython, PyPy and free-threaded builds each implement ordered mappings differently, and none of the properties the class relies on were asserted anywhere. The existing coverage was a single `list(iter(d)) == [...]` after `update()`; the LRU touch, the eviction order and the pickle round-trip had none at all. Adds test_LRUCache_ordering covering insertion order across iter/keys/values/items, the read-moves-to-end touch, position stability when updating an existing key, eviction order with and without a touch, bulk eviction via update(), popitem from both ends, and order surviving a pickle round trip. These run on every leg of the matrix, so a divergence surfaces as a named failure on the interpreter that has it rather than as odd cache behaviour downstream. Writing them turned up a bug that predates this branch: `__setitem__` evicted before checking whether the key was already present, so updating a key in a full cache discarded an unrelated entry and left the cache below its own limit. With limit=3 holding a/b/c, `cache["c"] = ...` returned a two-entry cache that had silently dropped "a". Fixed by skipping the eviction when the key is already there -- an update does not grow the cache, so it needs no room made for it. Confirmed the new test fails against the old condition and passes with it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GjoV6PsbmL1fGyDyB9FwfD --- mode/utils/collections.py | 12 +++- tests/functional/utils/test_collections.py | 81 ++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) diff --git a/mode/utils/collections.py b/mode/utils/collections.py index bc92dfc..a1f2125 100644 --- a/mode/utils/collections.py +++ b/mode/utils/collections.py @@ -520,7 +520,17 @@ def popitem(self, *, last: bool = True) -> tuple[KT, VT]: def __setitem__(self, key: KT, value: VT) -> None: # remove least recently used key. with self._mutex: - if self.limit and len(self.data) >= self.limit: + # NOTE: `key not in self.data` matters. Updating a key that is + # already present does not grow the cache, so evicting to make + # room for it discards an unrelated entry for nothing -- a full + # cache would shrink below its own limit on every such update + # (limit=3 holding a/b/c, then `cache["c"] = ...`, used to leave + # two entries and drop "a"). + if ( + key not in self.data + and self.limit + and len(self.data) >= self.limit + ): self.data.pop(next(iter(self.data))) self.data[key] = value diff --git a/tests/functional/utils/test_collections.py b/tests/functional/utils/test_collections.py index 9073bf1..7b66413 100644 --- a/tests/functional/utils/test_collections.py +++ b/tests/functional/utils/test_collections.py @@ -570,6 +570,87 @@ def test_pickle(self, d): assert e == d +class test_LRUCache_ordering: + """Pin the ordering semantics LRUCache depends on. + + Every one of these is a property of the backing mapping rather than of + code in this repo, so they are the assertions most likely to diverge + between interpreters -- CPython, PyPy and free-threaded builds each + implement ordered mappings differently. Keeping them explicit means a + divergence shows up as a named test failure on the relevant leg of the + matrix instead of as mysterious cache behaviour downstream. + """ + + def test_iteration_follows_insertion_order(self): + c = LRUCache() + for key in "abc": + c[key] = key.upper() + assert list(c) == ["a", "b", "c"] + assert list(c.keys()) == ["a", "b", "c"] + assert list(c.values()) == ["A", "B", "C"] + assert list(c.items()) == [("a", "A"), ("b", "B"), ("c", "C")] + + def test_reading_a_key_moves_it_to_the_end(self): + # The LRU touch: this is what makes eviction least-recently-*used* + # rather than merely oldest-inserted. + c = LRUCache() + for key in "abc": + c[key] = key + c["a"] + assert list(c) == ["b", "c", "a"] + + def test_updating_an_existing_key_keeps_its_position(self): + c = LRUCache() + for key in "abc": + c[key] = key + c["a"] = "changed" + assert list(c) == ["a", "b", "c"] + assert c.data["a"] == "changed" + + def test_updating_an_existing_key_does_not_evict(self): + # Regression: __setitem__ used to evict before checking whether the + # key was already present, so updating a key in a full cache + # discarded an unrelated entry and left the cache under its limit. + c = LRUCache(limit=3) + for key in "abc": + c[key] = key + c["c"] = "changed" + assert len(c) == 3 + assert list(c) == ["a", "b", "c"] + + def test_eviction_discards_the_oldest(self): + c = LRUCache(limit=3) + for key in "abcd": + c[key] = key + assert list(c) == ["b", "c", "d"] + + def test_eviction_respects_a_touch(self): + c = LRUCache(limit=3) + for key in "abc": + c[key] = key + c["a"] + c["d"] = "d" + assert list(c) == ["c", "a", "d"] + + def test_update_evicts_the_oldest_first(self): + c = LRUCache(limit=3) + c.update({key: key for key in "abcde"}) + assert list(c) == ["c", "d", "e"] + + def test_popitem_pops_from_either_end(self): + c = LRUCache() + for key in "abc": + c[key] = key + assert c.popitem() == ("c", "c") + assert c.popitem(last=False) == ("a", "a") + + def test_order_survives_a_pickle_round_trip(self): + c = LRUCache() + for key in "abc": + c[key] = key + assert list(pickle.loads(pickle.dumps(c))) == ["a", "b", "c"] + + class test_AttributeDictMixin: @pytest.fixture def d(self): From 3b458befa0168dd6bc11c4bdfcf4fb5bcc7aef69 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 20:45:42 +0000 Subject: [PATCH 13/16] Lock the whole LRUCache mapping surface, and make disconnect() work Three gaps in the free-threading work, all found by re-reviewing it: LRUCache guarded the methods it defines itself, but inherits the rest of its mapping surface from FastUserDict, whose implementations reach self.data directly. So `del cache[k]`, `clear()`, `copy()`, `len()`, `in` and `repr()` still touched the backing OrderedDict with the mutex released -- the same unguarded access the segfault came from, reachable through the ordinary mapping API. A read is no safer than a write here: a len() racing an eviction reads a linked list mid-relink. Override all of them, plus pop/setdefault/get, which were inherited as combinations of locked primitives with the lock dropped in between. __setstate__ restored pickled state verbatim, so a cache pickled on a GIL build (where thread_safety=False is legal and was the default) came back on a free-threaded interpreter with a nullcontext for a mutex -- the configuration __init__ refuses. Unpickling is a construction path too; it upgrades the flag now, which keeps old pickles loadable where raising would not. Signal.disconnect() never removed a receiver connected with the default weak=False: connect stored `lambda: fun` and disconnect built a second lambda to look up, and two lambdas never compare equal. The receiver set only grew, so the concurrency test added for the iteration fix was racing churn in one direction only. Strong receivers are a _StrongRef now -- zero-argument callable like weakref.ref, but equal and hashing by the wrapped handler, so bound methods work by equality rather than identity. The sender-specific path used set.remove under `except ValueError`, which cannot catch the KeyError it raises; it discards instead. Tests assert lock entry directly with a counting context manager, since a missing override only narrows the race window rather than failing outright, and the concurrency tests now assert the end state (empty receiver set) rather than only that nothing raised. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012N1dUvnii3PHJ1fJtxpSmB --- docs/free-threading.md | 38 +++++ mode/signals.py | 57 ++++++- mode/utils/collections.py | 94 ++++++++++ tests/freethreading/stress.py | 53 +++++- tests/functional/test_signals.py | 153 ++++++++++++++++- tests/functional/test_thread_safety.py | 226 +++++++++++++++++++++++++ 6 files changed, 613 insertions(+), 8 deletions(-) diff --git a/docs/free-threading.md b/docs/free-threading.md index 5a448a3..00d166a 100644 --- a/docs/free-threading.md +++ b/docs/free-threading.md @@ -101,6 +101,25 @@ corrupts its internal linked list. generator, since the lock was only released when the generator was closed. That hazard was latent while the lock defaulted to off; turning the lock on by default would have made it real. +- Taking the mutex in *every* operation that reaches `data`, not just the + ones `LRUCache` already defined. `LRUCache` inherits from `FastUserDict`, + whose methods use `self.data` directly, so `del cache[k]`, `clear()`, + `copy()`, `len(cache)`, `k in cache` and `repr(cache)` all reached the + `OrderedDict` with the mutex released — the same unguarded access the + segfault came from, through the ordinary mapping API. Read-only + operations are no safer here than writes: a `len()` racing an eviction + reads a linked list mid-relink. `pop`, `setdefault` and `get` are + overridden as well; the primitives they are inherited as were each + locked, but the lock was dropped between the lookup and the store, which + is a surprising place for a class advertising thread safety to lose an + invariant. +- Enforcing the same invariant when unpickling. `__setstate__` is a second + construction path, and it restored the pickled state verbatim — so a + cache pickled on a GIL build (where `thread_safety=False` is both legal + and the historical default) came back on a free-threaded interpreter + with a `nullcontext` for a mutex, exactly the configuration `__init__` + refuses. It now upgrades `thread_safety` to `True` instead, which keeps + old pickles loadable where raising would not. ### Why not just swap `OrderedDict` for `dict`? @@ -228,6 +247,25 @@ protocol and does not, so `tuple(r)` raises the very error the snapshot exists to prevent. The first attempt at this fix used `tuple(r)` and the stress harness caught it. +Half of that race turned out to be unreachable, which made the fix look +better tested than it was. `disconnect(fun)` never removed a receiver +connected with the default `weak=False`: `connect` stored `lambda: fun` and +`disconnect` built a *second* lambda to look it up, and two lambdas never +compare equal, so the `discard` matched nothing. The receiver set only ever +grew, and the "connect/disconnect churn" being raced was churn in one +direction. Sender-specific disconnects were worse than a no-op — they used +`set.remove`, which raises `KeyError` for a receiver that is not there, +under an `except ValueError` that could not catch it. + +**Also fixed** in `mode/signals.py` by storing strong receivers as a +`_StrongRef` — a zero-argument callable, like `weakref.ref`, but one whose +`__eq__`/`__hash__` are those of the wrapped handler, so a reference built +during `disconnect` matches the one stored by `connect`. Bound methods work +because equality decides rather than identity: `owner.handler` is a fresh +object on every attribute access. The sender-specific path uses `discard` +now, and the concurrency test asserts the receiver set is *empty* at the +end rather than only that nothing raised. + ### Not fixable here: the `gevent` extra re-enables the GIL | extra | result on `3.14t` | diff --git a/mode/signals.py b/mode/signals.py index 63c3120..fb045e1 100644 --- a/mode/signals.py +++ b/mode/signals.py @@ -22,6 +22,46 @@ __all__ = ["BaseSignal", "Signal", "SyncSignal"] +class _StrongRef: + """Reference to a receiver connected with ``weak=False``. + + Mirrors the `weakref.ref` interface used for weak receivers -- calling + it returns the handler -- but keeps the handler alive and, crucially, + compares equal to any other reference wrapping the same handler. + + That equality is what makes `disconnect` work. Strong receivers used + to be stored as ``lambda: fun``, and `disconnect` built a *second* + lambda to look up; two lambdas are never equal, so the `discard` never + matched and the receiver stayed connected forever. + """ + + __slots__ = ("fun",) + + def __init__(self, fun: SignalHandlerT) -> None: + self.fun = fun + + def __call__(self) -> SignalHandlerT: + return self.fun + + def __hash__(self) -> int: + try: + return hash(self.fun) + except TypeError: + # A handler that defines __eq__ without __hash__ cannot be + # looked up by equality anyway. Falling back to identity + # keeps connect() working for it, exactly as the old lambda + # did. + return id(self.fun) + + def __eq__(self, other: object) -> bool: + if isinstance(other, _StrongRef): + return bool(self.fun == other.fun) + return NotImplemented + + def __repr__(self) -> str: + return f"<{type(self).__name__}: {self.fun!r}>" + + class BaseSignal(BaseSignalT[T]): """Base class for signal/observer pattern.""" @@ -113,7 +153,7 @@ def _connect( self, fun: SignalHandlerT, *, weak: bool = False, sender: Any = None ) -> SignalHandlerT: ref: SignalHandlerRefT - ref = self._create_ref(fun) if weak else lambda: fun + ref = self._create_ref(fun) if weak else _StrongRef(fun) if self.default_sender is not None: sender = self.default_sender if sender is None: @@ -125,14 +165,21 @@ def _connect( def disconnect( self, fun: SignalHandlerT, *, weak: bool = False, sender: Any = None ) -> None: - ref: SignalHandlerRefT = self._create_ref(fun) if weak else lambda: fun + ref: SignalHandlerRefT + ref = self._create_ref(fun) if weak else _StrongRef(fun) if self.default_sender is not None: sender = self.default_sender if sender is None: self._receivers.discard(ref) else: try: - self._filter_receivers[self._create_id(sender)].remove(ref) + # `discard`, not `remove`: disconnecting a receiver that + # was never connected for this sender is not an error, and + # `set.remove` signals it with KeyError -- which the + # `except ValueError` below never caught. That clause is + # for `_create_id`, whose hash() of the sender is what can + # raise here. + self._filter_receivers[self._create_id(sender)].discard(ref) except ValueError: pass @@ -186,8 +233,8 @@ def _is_alive( value = ref() return value is not None, value # Receivers connected with ``weak=False`` are stored as a - # zero-argument callable returning the handler (see ``_connect``), - # and are always alive. + # ``_StrongRef``: a zero-argument callable returning the handler + # (see ``_connect``), which keeps it alive by construction. deref = cast(Callable[[], SignalHandlerT], ref) return True, deref() diff --git a/mode/utils/collections.py b/mode/utils/collections.py index a1f2125..716d42f 100644 --- a/mode/utils/collections.py +++ b/mode/utils/collections.py @@ -88,6 +88,9 @@ class LazySettings: ... _Setlike = Union[Set[T], Iterable[T]] +#: Sentinel for "no default given", so that `None` stays a usable default. +_MISSING: Any = object() + class Heap(MutableSequence[_ComparableT]): """Generic interface to `heapq`. @@ -579,6 +582,87 @@ def incr(self, key: KT, delta: int = 1) -> int: self[key] = cast(VT, str(newval)) return newval + # NOTE: Everything below re-implements an inherited method that would + # otherwise reach `self.data` with the mutex released. On a + # free-threaded build that is not merely a stale answer: touching the + # backing OrderedDict while another thread mutates it can corrupt its + # linked list badly enough to take the interpreter down, and a + # read-only operation such as `len` or `in` is just as capable of + # observing the half-updated state as a write is. Anything added to + # `FastUserDict` that uses `self.data` directly needs an override here + # too. + + def __delitem__(self, key: KT) -> None: + with self._mutex: + del self.data[key] + + def __len__(self) -> int: + with self._mutex: + return len(self.data) + + def __contains__(self, key: object) -> bool: + with self._mutex: + return key in self.data + + def __repr__(self) -> str: + with self._mutex: + return repr(self.data) + + def copy(self) -> dict: + with self._mutex: + return dict(self.data) + + def clear(self) -> None: + with self._mutex: + self.data.clear() + + # The compound `MutableMapping` helpers below are inherited as + # combinations of the primitives above. Each primitive is locked, so + # inheriting them would already be memory-safe, but the lock is + # dropped between the lookup and the store -- which for a class that + # advertises thread safety is a surprising place to lose an + # invariant. They are made atomic instead. + + @overload + def pop(self, key: KT) -> VT: ... + + @overload + def pop(self, key: KT, default: Union[VT, T]) -> Union[VT, T]: ... + + def pop(self, key: KT, default: Any = _MISSING) -> Any: + with self._mutex: + try: + return cast(VT, self.data.pop(key)) + except KeyError: + if default is _MISSING: + raise + return default + + # NOTE: Not overloaded like `pop` and `get` above. `LRUCache` lists + # `FastUserDict` unparameterized among its bases, so the inherited + # `setdefault` erases to `(Any, None = ...) -> Any | None`, and a + # narrower `(KT, VT) -> VT` pair here is an incompatible override. + def setdefault(self, key: KT, default: Any = None) -> Any: + with self._mutex: + try: + return self[key] + except KeyError: + self[key] = cast(VT, default) + return default + + @overload + def get(self, key: KT) -> Optional[VT]: ... + + @overload + def get(self, key: KT, default: Union[VT, T]) -> Union[VT, T]: ... + + def get(self, key: KT, default: Any = None) -> Any: + with self._mutex: + try: + return self[key] + except KeyError: + return default + def _new_lock(self) -> AbstractContextManager: if self.thread_safety: return cast(AbstractContextManager, threading.RLock()) @@ -590,6 +674,16 @@ def __getstate__(self) -> Mapping[str, Any]: return d def __setstate__(self, state: dict[str, Any]) -> None: + # Unpickling is another way to construct the object, so it has to + # honour the same invariant `__init__` does: no unguarded + # OrderedDict on a free-threaded build. Pickles written by an + # older version -- or on a GIL build, where thread_safety=False is + # both the default and legal -- would otherwise come back here + # with `nullcontext` for a mutex. Upgrading the flag keeps those + # pickles loadable, which raising would not. + state = dict(state) + if FREE_THREADED and not state.get("thread_safety", False): + state["thread_safety"] = True self.__dict__ = state self._mutex = self._new_lock() diff --git a/tests/freethreading/stress.py b/tests/freethreading/stress.py index a5960d7..09cc351 100644 --- a/tests/freethreading/stress.py +++ b/tests/freethreading/stress.py @@ -117,6 +117,44 @@ def work(i, cache=cache): ) +def check_lru_mapping_surface(trials=40): + # The checks above only drive the methods LRUCache defines itself. + # Every other mapping operation used to be inherited from FastUserDict, + # which reaches self.data with the mutex released -- so `del`, `clear`, + # `copy`, `len`, `in` and `repr` had the same unguarded OrderedDict + # access that the segfault came from. Race them against writers. + from mode.utils.collections import LRUCache + + bad = 0 + for _ in range(trials): + cache = LRUCache(limit=50) + + def work(i, cache=cache): + for n in range(100): + key = f"{i}-{n}" + cache[key] = n + len(cache) + key in cache # noqa: B015 + repr(cache) + cache.copy() + cache.get(key) + cache.setdefault(f"sd-{i}", n) + cache.pop(key, None) + try: + del cache[f"{i}-{n - 1}"] + except KeyError: + pass + if not n % 25: + cache.clear() + + if race(work): + bad += 1 + print( + f"[{'FAIL' if bad else 'ok '}] LRUCache(mapping surface): " + f"{bad}/{trials} trials raised" + ) + + # -------------------------------------------------------------------------- # Defect 2 (fixed): cached_property.__get__ was a non-atomic check-then-act # on obj.__dict__, so racing threads each computed and handed out a distinct @@ -196,11 +234,18 @@ def work(i, proxy=proxy, seen=seen, seen_lock=seen_lock): # Defect 4 (fixed): Signal iterated its receiver set while connect/disconnect # mutated it. Pre-existing -- this failed on GIL builds too. It snapshots # with list() now (NOT tuple(), which does not lock the source set). +# +# The disconnect half of that race only became real once strong receivers +# were stored as _StrongRef: they used to be `lambda: fun`, and disconnect +# built a second lambda that could never compare equal, so the receiver set +# grew monotonically and was never actually mutated by disconnect(). The +# leftover count below is asserted, not just the absence of exceptions. # -------------------------------------------------------------------------- def check_signal(trials=30): from mode.signals import Signal bad = 0 + leaked = 0 for _ in range(trials): class Owner: @@ -223,9 +268,12 @@ async def handler(*args, **kwargs): if race(work): bad += 1 + if sig._receivers: + leaked += 1 print( - f"[{'FAIL' if bad else 'ok '}] Signal iter_receivers: " - f"{bad}/{trials} trials raised" + f"[{'FAIL' if bad or leaked else 'ok '}] Signal iter_receivers: " + f"{bad}/{trials} trials raised, " + f"{leaked}/{trials} left receivers connected" ) @@ -422,6 +470,7 @@ def main(): check_cached_property() check_service_proxy() check_lru_default() + check_lru_mapping_surface() if __name__ == "__main__": diff --git a/tests/functional/test_signals.py b/tests/functional/test_signals.py index f14c994..de4cbd3 100644 --- a/tests/functional/test_signals.py +++ b/tests/functional/test_signals.py @@ -5,7 +5,7 @@ import pytest from mode import label -from mode.signals import Signal, SignalT, SyncSignal, SyncSignalT +from mode.signals import Signal, SignalT, SyncSignal, SyncSignalT, _StrongRef class X: @@ -239,3 +239,154 @@ def foo(self, **kwargs): assert sig._create_ref(X.foo) assert sig._create_ref(X().foo) + + +class test_disconnect_removes_the_receiver: + """`disconnect` has to undo `connect`, strong references included. + + Strong receivers used to be stored as ``lambda: fun``, and + `disconnect` built a *second* lambda to look up. Two lambdas never + compare equal, so the `discard` matched nothing and the handler stayed + connected -- and stayed subscribed to every subsequent send. + """ + + @pytest.fixture + def handler(self): + async def handler(*args: Any, **kwargs: Any) -> None: ... + + return handler + + def test_strong_receiver(self, handler): + sig = Signal() + sig.connect(handler) + assert len(sig._receivers) == 1 + + sig.disconnect(handler) + assert not sig._receivers + + def test_weak_receiver(self, handler): + sig = Signal() + sig.connect(handler, weak=True) + assert len(sig._receivers) == 1 + + sig.disconnect(handler, weak=True) + assert not sig._receivers + + def test_strong_bound_method(self): + class Owner: + async def handler(self, *args: Any, **kwargs: Any) -> None: ... + + owner = Owner() + sig = Signal() + # `owner.handler` is a fresh bound method object on every attribute + # access, so this only works if equality is what decides, not + # identity. + sig.connect(owner.handler) + sig.disconnect(owner.handler) + assert not sig._receivers + + def test_connect_is_still_idempotent(self, handler): + sig = Signal() + sig.connect(handler) + sig.connect(handler) + assert len(sig._receivers) == 1 + + def test_only_the_named_receiver_is_removed(self, handler): + async def other(*args: Any, **kwargs: Any) -> None: ... + + sig = Signal() + sig.connect(handler) + sig.connect(other) + + sig.disconnect(handler) + assert {r() for r in sig._receivers} == {other} + + def test_disconnected_receiver_stops_being_iterated(self, handler): + sender = object() + sig = Signal() + sig.connect(handler) + assert list(sig.iter_receivers(sender)) == [handler] + + sig.disconnect(handler) + assert list(sig.iter_receivers(sender)) == [] + + def test_sender_specific_receiver(self, handler): + sender = object() + sig = Signal() + sig.connect(handler, sender=sender) + assert sig._filter_receivers[sig._create_id(sender)] + + sig.disconnect(handler, sender=sender) + assert not sig._filter_receivers[sig._create_id(sender)] + + def test_sender_specific_disconnect_of_unknown_receiver(self, handler): + # `set.remove` raised KeyError here, which the `except ValueError` + # around it never caught. + sig = Signal() + sig.connect(handler, sender=object()) + sig.disconnect(handler, sender=object()) + + def test_disconnect_of_never_connected_receiver(self, handler): + sig = Signal() + sig.disconnect(handler) + sig.disconnect(handler, sender=object()) + + def test_default_sender_disconnect(self, handler): + x = X() + x.on_started.connect(handler) + assert x.on_started._filter_receivers[x.on_started._create_id(x)] + + x.on_started.disconnect(handler) + assert not x.on_started._filter_receivers[x.on_started._create_id(x)] + + +class test_StrongRef: + def test_calling_it_returns_the_handler(self): + def fun(): ... + + assert _StrongRef(fun)() is fun + + def test_equal_and_hashes_alike_for_the_same_handler(self): + def fun(): ... + + assert _StrongRef(fun) == _StrongRef(fun) + assert hash(_StrongRef(fun)) == hash(_StrongRef(fun)) + assert len({_StrongRef(fun), _StrongRef(fun)}) == 1 + + def test_differs_from_a_ref_to_another_handler(self): + def fun(): ... + + def other(): ... + + assert _StrongRef(fun) != _StrongRef(other) + + def test_never_equal_to_a_plain_callable(self): + # Weak receivers live in the same set, so comparisons against + # something that is not a _StrongRef have to defer rather than + # claim equality. + def fun(): ... + + assert _StrongRef(fun).__eq__(fun) is NotImplemented + assert _StrongRef(fun) != fun + + def test_unhashable_handler_falls_back_to_identity(self): + class Unhashable: + __hash__ = None # type: ignore[assignment] + + def __call__(self): ... + + fun = Unhashable() + with pytest.raises(TypeError): + hash(fun) + # The old `lambda: fun` hashed by identity, so connecting one of + # these has to keep working. + assert hash(_StrongRef(fun)) == id(fun) + + sig = Signal() + sig.connect(fun) + assert len(sig._receivers) == 1 + + def test_repr_names_the_handler(self): + def fun(): ... + + assert "fun" in repr(_StrongRef(fun)) diff --git a/tests/functional/test_thread_safety.py b/tests/functional/test_thread_safety.py index 113c65e..a8694a2 100644 --- a/tests/functional/test_thread_safety.py +++ b/tests/functional/test_thread_safety.py @@ -9,10 +9,12 @@ `tests/freethreading/stress.py` for the heavier probabilistic reproducers. """ +import pickle import sys import threading import time from collections import OrderedDict +from contextlib import nullcontext from types import ModuleType import pytest @@ -213,6 +215,225 @@ def work(i): assert not errors + def test_concurrent_mapping_surface(self): + # The test above only drives the methods LRUCache defines itself. + # Every other mapping operation used to be inherited straight from + # FastUserDict, reaching self.data with the mutex released. + c = LRUCache(limit=50) + barrier = threading.Barrier(8) + errors = [] + + def work(i): + barrier.wait() + try: + for n in range(200): + key = f"{i}-{n}" + c[key] = n + len(c) + key in c # noqa: B015 + repr(c) + c.copy() + c.get(key) + c.setdefault(f"sd-{i}", n) + c.pop(key, None) + if not n % 50: + c.clear() + except BaseException as exc: # pragma: no cover + errors.append(exc) + + threads = [threading.Thread(target=work, args=(i,)) for i in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert not errors + + +class test_LRUCache_takes_the_mutex: + """Every operation reaching ``data`` must go through ``_mutex``. + + `LRUCache` inherits most of its mapping surface from `FastUserDict`, + whose implementations use ``self.data`` directly. An override that + goes missing is invisible to a stress test -- it just makes the race + window smaller -- so assert lock entry directly instead. + """ + + class TrackingMutex: + def __init__(self) -> None: + self.enters = 0 + + def __enter__(self) -> None: + self.enters += 1 + + def __exit__(self, *exc_info: object) -> None: + pass + + def assert_takes_mutex(self, operation): + cache = LRUCache(limit=10, thread_safety=True) + # Populate without going through the (locked) __setitem__, so the + # count below only reflects the operation under test. + cache.data["a"] = 1 + cache.data["b"] = 2 + mutex = self.TrackingMutex() + cache._mutex = mutex + + operation(cache) + + assert mutex.enters, "operation reached .data without the mutex" + + @pytest.mark.parametrize( + "name,operation", + [ + ("__setitem__", lambda c: c.__setitem__("c", 3)), + ("__getitem__", lambda c: c["a"]), + ("__delitem__", lambda c: c.__delitem__("a")), + ("__len__", len), + ("__contains__", lambda c: "a" in c), + ("__repr__", repr), + ("__iter__", lambda c: list(iter(c))), + ("keys", lambda c: list(c.keys())), + ("values", lambda c: list(c.values())), + ("items", lambda c: list(c.items())), + ("copy", lambda c: c.copy()), + ("clear", lambda c: c.clear()), + ("update", lambda c: c.update({"c": 3})), + ("popitem", lambda c: c.popitem()), + ("pop", lambda c: c.pop("a")), + ("pop-default", lambda c: c.pop("missing", None)), + ("setdefault-hit", lambda c: c.setdefault("a", 0)), + ("setdefault-miss", lambda c: c.setdefault("z", 0)), + ("get-hit", lambda c: c.get("a")), + ("get-miss", lambda c: c.get("missing")), + ("incr", lambda c: c.incr("a")), + ], + ) + def test_operation_takes_mutex(self, name, operation): + self.assert_takes_mutex(operation) + + +class test_LRUCache_mapping_semantics: + """The mutex overrides must not change what the methods do.""" + + def test_pop_returns_and_removes(self): + c = LRUCache() + c.update({"a": 1, "b": 2}) + assert c.pop("a") == 1 + assert "a" not in c + assert len(c) == 1 + + def test_pop_missing_raises_KeyError(self): + with pytest.raises(KeyError): + LRUCache().pop("a") + + def test_pop_missing_returns_default(self): + assert LRUCache().pop("a", "default") == "default" + # None has to stay usable as a default, so the "no default given" + # sentinel cannot be None. + assert LRUCache().pop("a", None) is None + + def test_setdefault_stores_and_returns(self): + c = LRUCache() + assert c.setdefault("a", 1) == 1 + assert c.setdefault("a", 2) == 1 + assert c["a"] == 1 + + def test_get(self): + c = LRUCache() + c["a"] = 1 + assert c.get("a") == 1 + assert c.get("b") is None + assert c.get("b", "default") == "default" + + def test_len_contains_and_repr(self): + c = LRUCache() + c.update({"a": 1}) + assert len(c) == 1 + assert "a" in c + assert "b" not in c + assert repr(c) == repr(c.data) + + def test_copy_is_a_plain_dict_snapshot(self): + c = LRUCache() + c.update({"a": 1}) + copy = c.copy() + assert copy == {"a": 1} + assert type(copy) is dict + c["b"] = 2 + assert copy == {"a": 1} + + def test_del_and_clear(self): + c = LRUCache() + c.update({"a": 1, "b": 2}) + del c["a"] + assert list(c.keys()) == ["b"] + c.clear() + assert not len(c) + with pytest.raises(KeyError): + del c["a"] + + def test_pop_does_not_reinsert_the_key(self): + # __getitem__ pops and reinserts to mark the key most recently + # used; pop() must not leave it behind while doing that. + c = LRUCache(limit=3) + c.update({"a": 1, "b": 2, "c": 3}) + assert c.pop("a") == 1 + assert list(c.keys()) == ["b", "c"] + + +class test_LRUCache_pickle: + def test_roundtrip_keeps_data_and_limit(self): + c = LRUCache(limit=3) + c.update({"a": 1, "b": 2}) + restored = pickle.loads(pickle.dumps(c)) + assert restored.limit == 3 + assert list(restored.items()) == [("a", 1), ("b", 2)] + assert restored.thread_safety is c.thread_safety + + def test_restored_cache_is_usable(self): + restored = pickle.loads(pickle.dumps(LRUCache(limit=2))) + restored["a"] = 1 + restored["b"] = 2 + restored["c"] = 3 + assert list(restored.keys()) == ["b", "c"] + + def test_unsafe_pickle_is_upgraded_when_free_threaded(self, monkeypatch): + # Unpickling is another construction path, so it has to honour the + # invariant __init__ enforces. A pickle written on a GIL build -- + # where thread_safety=False is both legal and the default -- used + # to restore a free-threaded cache with a nullcontext for a mutex. + monkeypatch.setattr("mode.utils.collections.FREE_THREADED", False) + payload = pickle.dumps(LRUCache(thread_safety=False)) + + monkeypatch.setattr("mode.utils.collections.FREE_THREADED", True) + restored = pickle.loads(payload) + + assert restored.thread_safety is True + assert not isinstance(restored._mutex, nullcontext) + + def test_unsafe_pickle_is_left_alone_with_the_gil(self, monkeypatch): + monkeypatch.setattr("mode.utils.collections.FREE_THREADED", False) + restored = pickle.loads(pickle.dumps(LRUCache(thread_safety=False))) + + assert restored.thread_safety is False + assert isinstance(restored._mutex, nullcontext) + + def test_setstate_preserves_true_thread_safety(self, monkeypatch): + monkeypatch.setattr("mode.utils.collections.FREE_THREADED", True) + restored = pickle.loads(pickle.dumps(LRUCache(thread_safety=True))) + + assert restored.thread_safety is True + assert not isinstance(restored._mutex, nullcontext) + + def test_setstate_does_not_mutate_the_state_it_is_given(self, monkeypatch): + monkeypatch.setattr("mode.utils.collections.FREE_THREADED", True) + state = {"limit": None, "thread_safety": False, "data": OrderedDict()} + cache = LRUCache.__new__(LRUCache) + cache.__setstate__(state) + + assert cache.thread_safety is True + assert state["thread_safety"] is False + class test_Signal_receiver_iteration: def test_get_live_receivers_tolerates_mutation(self): @@ -268,6 +489,11 @@ async def handler(*args, **kwargs): ... t.join() assert not errors + # Every connect above was paired with a disconnect, so the set has + # to be empty. Without this the test proved much less than it + # looked like it did: disconnect() was a no-op for strong + # receivers, so the "mutation" being raced was only ever growth. + assert not signal._receivers class test_mode_lazy_imports: From 1c7074130fb2946efa0a3ad7d7f80dc1fb5f52b0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 04:26:40 +0000 Subject: [PATCH 14/16] Store strong signal receivers unwrapped, and bound the concurrency tests The `_StrongRef` wrapper added in the previous commit fixed `Signal.disconnect` and passed on every CPython build, but it wedged PyPy: `test_iter_receivers_while_connecting` either finished in a second or never finished at all, and in CI it burned the job's six-hour limit without reporting anything. The wrapper defines `__eq__`/`__hash__` in Python. That makes every `set.add` and `set.discard` on the receiver set re-enter the interpreter partway through, which releases the GIL and lets another thread mutate the same set while the operation that called out is still walking it. That set is mutated from several threads by design, which is the whole point of the test. Storing `lambda: fun` had never had this problem -- a function hashes and compares in the interpreter -- so the wrapper introduced it. Store the handler itself instead, wrapped in nothing. It already hashes and compares the way `disconnect` needs: functions by identity, bound methods by `(__func__, __self__)`, so `owner.handler` matches the entry `connect` stored even though attribute access builds a fresh object every time. `SignalHandlerRefT` already admitted a bare handler, and `_is_alive` returns it as-is rather than calling it. This deletes the wrapper rather than repairing it, and drops a `typing` subscription that `_is_alive` was rebuilding on every receiver of every send. One behaviour change: a handler that cannot be hashed is now rejected by `connect` instead of by the first `send`. It was never usable either way -- `_get_live_receivers` collects dereferenced handlers into a set, so an unhashable one raised `TypeError` there -- and failing at registration points at the handler. The symptom here was silence rather than a failing test, so two things bound it now. Every wait in the concurrency tests goes through a `race` helper with a timeout, so a thread that stops making progress fails the test that owns it in a minute instead of hanging the run; and the test jobs carry a `timeout-minutes` well under the six-hour default. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CjHx3ivL7WAoHZTBMWMQPQ --- .github/workflows/tests.yml | 7 ++ docs/free-threading.md | 33 +++-- mode/signals.py | 73 ++++------- tests/freethreading/stress.py | 14 ++- tests/functional/test_signals.py | 109 +++++++++------- tests/functional/test_thread_safety.py | 164 ++++++++++++------------- 6 files changed, 214 insertions(+), 186 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 4209a72..a98e61c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -14,6 +14,13 @@ jobs: name: "Run tests with Python ${{ matrix.python-version }}" runs-on: "ubuntu-latest" + # The suite takes about a minute; the slowest leg (PyPy) about three. + # Without a limit here a job that stops making progress -- a wedged + # thread in one of the concurrency tests, say -- is only stopped by + # the six-hour default, holding up the whole run and reporting + # nothing useful when it finally dies. + timeout-minutes: 20 + continue-on-error: ${{ matrix.experimental }} strategy: fail-fast: false diff --git a/docs/free-threading.md b/docs/free-threading.md index 00d166a..84bfad0 100644 --- a/docs/free-threading.md +++ b/docs/free-threading.md @@ -257,14 +257,31 @@ direction. Sender-specific disconnects were worse than a no-op — they used `set.remove`, which raises `KeyError` for a receiver that is not there, under an `except ValueError` that could not catch it. -**Also fixed** in `mode/signals.py` by storing strong receivers as a -`_StrongRef` — a zero-argument callable, like `weakref.ref`, but one whose -`__eq__`/`__hash__` are those of the wrapped handler, so a reference built -during `disconnect` matches the one stored by `connect`. Bound methods work -because equality decides rather than identity: `owner.handler` is a fresh -object on every attribute access. The sender-specific path uses `discard` -now, and the concurrency test asserts the receiver set is *empty* at the -end rather than only that nothing raised. +**Also fixed** in `mode/signals.py` by storing a strong receiver as the +handler itself, with nothing wrapped around it. A handler already hashes +and compares the way `disconnect` needs — functions by identity, bound +methods by `(__func__, __self__)`, so `owner.handler` matches even though +attribute access builds a fresh object every time. The sender-specific +path uses `discard` now, and the concurrency test asserts the receiver set +is *empty* at the end rather than only that nothing raised. + +The first attempt at this stored a `_StrongRef` wrapper instead, holding +the handler and defining `__eq__`/`__hash__` in terms of it. It made +`disconnect` work and it passed on every CPython build — and it wedged +PyPy. Defining those two methods in Python means `set.add` and +`set.discard` re-enter the interpreter partway through, which releases the +GIL and lets another thread mutate the same set while the operation that +called out is still walking it. The receiver set is mutated from several +threads by design, so `test_iter_receivers_while_connecting` would either +finish in a second or never finish at all; in CI it burned the job's +six-hour limit. Entries in that set have to hash and compare in the +interpreter, which is a constraint on any future change to how receivers +are represented, not just on the wrapper that ran into it. + +Two things bound the damage from that class of mistake now, since the +symptom is silence rather than a failure: every wait in the concurrency +tests is bounded (see `race` in `tests/functional/test_thread_safety.py`), +and the test jobs carry a `timeout-minutes`. ### Not fixable here: the `gevent` extra re-enables the GIL diff --git a/mode/signals.py b/mode/signals.py index fb045e1..bf90a0c 100644 --- a/mode/signals.py +++ b/mode/signals.py @@ -22,46 +22,6 @@ __all__ = ["BaseSignal", "Signal", "SyncSignal"] -class _StrongRef: - """Reference to a receiver connected with ``weak=False``. - - Mirrors the `weakref.ref` interface used for weak receivers -- calling - it returns the handler -- but keeps the handler alive and, crucially, - compares equal to any other reference wrapping the same handler. - - That equality is what makes `disconnect` work. Strong receivers used - to be stored as ``lambda: fun``, and `disconnect` built a *second* - lambda to look up; two lambdas are never equal, so the `discard` never - matched and the receiver stayed connected forever. - """ - - __slots__ = ("fun",) - - def __init__(self, fun: SignalHandlerT) -> None: - self.fun = fun - - def __call__(self) -> SignalHandlerT: - return self.fun - - def __hash__(self) -> int: - try: - return hash(self.fun) - except TypeError: - # A handler that defines __eq__ without __hash__ cannot be - # looked up by equality anyway. Falling back to identity - # keeps connect() working for it, exactly as the old lambda - # did. - return id(self.fun) - - def __eq__(self, other: object) -> bool: - if isinstance(other, _StrongRef): - return bool(self.fun == other.fun) - return NotImplemented - - def __repr__(self) -> str: - return f"<{type(self).__name__}: {self.fun!r}>" - - class BaseSignal(BaseSignalT[T]): """Base class for signal/observer pattern.""" @@ -153,7 +113,21 @@ def _connect( self, fun: SignalHandlerT, *, weak: bool = False, sender: Any = None ) -> SignalHandlerT: ref: SignalHandlerRefT - ref = self._create_ref(fun) if weak else _StrongRef(fun) + # NOTE: A strong receiver is stored as the handler itself, not + # wrapped in anything. `disconnect` needs to find the stored + # entry by value, and a handler already hashes and compares the + # way that requires: functions by identity, and bound methods by + # ``(__func__, __self__)`` -- so ``owner.handler``, a fresh object + # on every attribute access, still matches the one connect stored. + # + # Just as important, both of those comparisons are implemented in + # the interpreter rather than in Python. A wrapper defining + # ``__eq__``/``__hash__`` in Python would make every ``add`` and + # ``discard`` below re-enter the interpreter mid-operation, which + # releases the GIL and lets another thread mutate this set while + # the lookup that called us is walking it. On PyPy that + # reliably wedges a concurrent connect/disconnect loop. + ref = self._create_ref(fun) if weak else fun if self.default_sender is not None: sender = self.default_sender if sender is None: @@ -166,7 +140,13 @@ def disconnect( self, fun: SignalHandlerT, *, weak: bool = False, sender: Any = None ) -> None: ref: SignalHandlerRefT - ref = self._create_ref(fun) if weak else _StrongRef(fun) + # Mirrors `_connect`: a strong receiver is the handler itself, so + # the value built here compares equal to the one stored there. + # This used to be ``lambda: fun``, and `_connect` stored a + # *different* lambda -- two lambdas are never equal, so the + # `discard` below matched nothing and the receiver stayed + # connected forever. + ref = self._create_ref(fun) if weak else fun if self.default_sender is not None: sender = self.default_sender if sender is None: @@ -232,11 +212,10 @@ def _is_alive( if isinstance(ref, ReferenceType): value = ref() return value is not None, value - # Receivers connected with ``weak=False`` are stored as a - # ``_StrongRef``: a zero-argument callable returning the handler - # (see ``_connect``), which keeps it alive by construction. - deref = cast(Callable[[], SignalHandlerT], ref) - return True, deref() + # Anything that is not a weak reference was connected with + # ``weak=False``, which `_connect` stores as the handler itself: + # alive by construction, and already the value to return. + return True, cast(SignalHandlerT, ref) def _create_ref(self, fun: SignalHandlerT) -> SignalHandlerRefT: if hasattr(fun, "__func__") and hasattr(fun, "__self__"): diff --git a/tests/freethreading/stress.py b/tests/freethreading/stress.py index 09cc351..aa3ac6a 100644 --- a/tests/freethreading/stress.py +++ b/tests/freethreading/stress.py @@ -236,10 +236,16 @@ def work(i, proxy=proxy, seen=seen, seen_lock=seen_lock): # with list() now (NOT tuple(), which does not lock the source set). # # The disconnect half of that race only became real once strong receivers -# were stored as _StrongRef: they used to be `lambda: fun`, and disconnect -# built a second lambda that could never compare equal, so the receiver set -# grew monotonically and was never actually mutated by disconnect(). The -# leftover count below is asserted, not just the absence of exceptions. +# were stored as the handler itself: they used to be `lambda: fun`, and +# disconnect built a second lambda that could never compare equal, so the +# receiver set grew monotonically and was never actually mutated by +# disconnect(). The leftover count below is asserted, not just the +# absence of exceptions. +# +# Storing them bare also keeps hashing and comparison in the interpreter. +# A wrapper defining __eq__/__hash__ in Python makes set.add/set.discard +# re-enter the interpreter mid-operation, releasing the GIL while the set +# is being walked; that wedges this loop outright on PyPy. # -------------------------------------------------------------------------- def check_signal(trials=30): from mode.signals import Signal diff --git a/tests/functional/test_signals.py b/tests/functional/test_signals.py index de4cbd3..64ea635 100644 --- a/tests/functional/test_signals.py +++ b/tests/functional/test_signals.py @@ -5,7 +5,7 @@ import pytest from mode import label -from mode.signals import Signal, SignalT, SyncSignal, SyncSignalT, _StrongRef +from mode.signals import Signal, SignalT, SyncSignal, SyncSignalT class X: @@ -178,13 +178,15 @@ def test_with_default_sender(self, sig): sig3 = super(type(sig2), sig2).clone() assert sig3.asdict() == sig2.asdict() - def test_disconnect_lambda(self, sig): + def test_disconnect_discards_the_handler_itself(self, sig): + # A strong receiver is stored unwrapped, so the value handed to + # `discard` is the handler. It used to be a freshly built + # ``lambda: fun``, which could never equal the one `connect` + # stored, so the discard matched nothing. sig._receivers = Mock() r = Mock() sig.disconnect(r, sender=None) - sig._receivers.discard.assert_called_once() - lmbda = sig._receivers.discard.call_args[0][0] - assert lmbda() == r + sig._receivers.discard.assert_called_once_with(r) def test_disconnect_raises(self, sig): sig._create_id = Mock(side_effect=ValueError()) @@ -229,7 +231,12 @@ class Object: x = Object() x.value = 10 - assert sig._is_alive(lambda: 42) == (True, 42) + + async def handler(*args, **kwargs): ... + + # Not a weakref -- a strong receiver, stored as the handler + # itself, so it is returned as-is rather than called. + assert sig._is_alive(handler) == (True, handler) assert sig._is_alive(ref(x)) == (True, x) def test_create_ref_methods(self, sig): @@ -299,7 +306,7 @@ async def other(*args: Any, **kwargs: Any) -> None: ... sig.connect(other) sig.disconnect(handler) - assert {r() for r in sig._receivers} == {other} + assert set(sig._receivers) == {other} def test_disconnected_receiver_stops_being_iterated(self, handler): sender = object() @@ -340,53 +347,69 @@ def test_default_sender_disconnect(self, handler): assert not x.on_started._filter_receivers[x.on_started._create_id(x)] -class test_StrongRef: - def test_calling_it_returns_the_handler(self): - def fun(): ... - - assert _StrongRef(fun)() is fun +class test_strong_receivers_are_stored_unwrapped: + """A ``weak=False`` receiver is kept in the set as the handler itself. - def test_equal_and_hashes_alike_for_the_same_handler(self): - def fun(): ... + Nothing wraps it. That is what lets `disconnect` find it -- functions + hash and compare by identity, bound methods by + ``(__func__, __self__)`` -- and it keeps every `set` operation on the + receiver set free of Python-level ``__hash__``/``__eq__``, which would + otherwise re-enter the interpreter mid-operation and let another + thread mutate the set underneath it. + """ - assert _StrongRef(fun) == _StrongRef(fun) - assert hash(_StrongRef(fun)) == hash(_StrongRef(fun)) - assert len({_StrongRef(fun), _StrongRef(fun)}) == 1 + def test_the_set_holds_the_handler(self): + async def fun(*args: Any, **kwargs: Any) -> None: ... - def test_differs_from_a_ref_to_another_handler(self): - def fun(): ... + sig = Signal() + sig.connect(fun) + assert set(sig._receivers) == {fun} - def other(): ... + def test_the_stored_receiver_is_not_a_callable_wrapper(self): + # `_is_alive` distinguishes weak from strong by asking whether the + # entry is a `weakref`, so a strong entry must be the handler and + # not something that returns it when called. + async def fun(*args: Any, **kwargs: Any) -> None: ... - assert _StrongRef(fun) != _StrongRef(other) + sig = Signal() + sig.connect(fun) + (stored,) = sig._receivers + assert stored is fun + assert sig._is_alive(stored) == (True, fun) - def test_never_equal_to_a_plain_callable(self): - # Weak receivers live in the same set, so comparisons against - # something that is not a _StrongRef have to defer rather than - # claim equality. - def fun(): ... + def test_weak_and_strong_receivers_coexist(self): + async def strong(*args: Any, **kwargs: Any) -> None: ... - assert _StrongRef(fun).__eq__(fun) is NotImplemented - assert _StrongRef(fun) != fun + async def weak(*args: Any, **kwargs: Any) -> None: ... - def test_unhashable_handler_falls_back_to_identity(self): - class Unhashable: - __hash__ = None # type: ignore[assignment] - - def __call__(self): ... + sig = Signal() + sig.connect(strong) + sig.connect(weak, weak=True) + assert set(sig.iter_receivers(object())) == {strong, weak} - fun = Unhashable() - with pytest.raises(TypeError): - hash(fun) - # The old `lambda: fun` hashed by identity, so connecting one of - # these has to keep working. - assert hash(_StrongRef(fun)) == id(fun) + def test_hashing_is_not_implemented_in_python(self): + # The point of storing the handler bare: `set.add`/`set.discard` + # must not call back into Python to hash or compare an entry. + async def fun(*args: Any, **kwargs: Any) -> None: ... sig = Signal() sig.connect(fun) - assert len(sig._receivers) == 1 + (stored,) = sig._receivers + assert type(stored).__hash__ is object.__hash__ + assert type(stored).__eq__ is object.__eq__ + + def test_unhashable_handler_is_rejected_at_connect(self): + # A handler that cannot be hashed cannot go in the receiver set. + # It never worked: `lambda: fun` let `connect` succeed, and then + # the first `send` blew up in `_get_live_receivers`, which + # collects the dereferenced handlers into a set of their own. + # Failing at registration points at the handler instead. + class Unhashable: + __hash__ = None # type: ignore[assignment] - def test_repr_names_the_handler(self): - def fun(): ... + async def __call__(self, *args: Any, **kwargs: Any) -> None: ... - assert "fun" in repr(_StrongRef(fun)) + sig = Signal() + with pytest.raises(TypeError): + sig.connect(Unhashable()) + assert not sig._receivers diff --git a/tests/functional/test_thread_safety.py b/tests/functional/test_thread_safety.py index a8694a2..0bf952b 100644 --- a/tests/functional/test_thread_safety.py +++ b/tests/functional/test_thread_safety.py @@ -25,24 +25,62 @@ from mode.utils.collections import FREE_THREADED, LRUCache from mode.utils.objects import cached_property +#: Upper bound for any one concurrency test below. Generous: these +#: finish in well under a second when they are healthy, and the point of +#: the bound is only to keep a wedged thread from waiting forever. +RACE_TIMEOUT = 60.0 + + +def race(work, nthreads=8, timeout=RACE_TIMEOUT): + """Run ``work(i)`` in `nthreads` threads released together. + + Returns the exceptions the workers raised, for the caller to assert + on. A worker still running after `timeout` fails the test here. + + Every wait is bounded on purpose. The defects these tests cover + show up as a thread that stops making progress, and an unbounded + `threading.Barrier.wait` or `threading.Thread.join` turns that into + a CI job that reports nothing until it hits its own time limit -- + six hours, in the case that prompted this helper. Bounded, the same + defect fails in a minute and names the test it happened in. + """ + barrier = threading.Barrier(nthreads) + errors = [] + + def target(i): + try: + barrier.wait(timeout=timeout) + work(i) + except BaseException as exc: # pragma: no cover + errors.append(exc) + + threads = [ + threading.Thread(target=target, args=(i,), daemon=True) + for i in range(nthreads) + ] + for t in threads: + t.start() + deadline = time.monotonic() + timeout + for t in threads: + t.join(timeout=max(0.0, deadline - time.monotonic())) + still_running = sum(1 for t in threads if t.is_alive()) + assert not still_running, ( + f"{still_running}/{nthreads} threads still running after {timeout}s" + ) + return errors + class test_cached_property_is_computed_once: def _race_on(self, obj, nthreads=8): - barrier = threading.Barrier(nthreads) seen = [] lock = threading.Lock() - def work(): - barrier.wait() + def work(i): value = obj.val with lock: seen.append(value) - threads = [threading.Thread(target=work) for _ in range(nthreads)] - for t in threads: - t.start() - for t in threads: - t.join() + assert not race(work, nthreads) return seen def test_concurrent_miss_computes_once(self): @@ -82,23 +120,17 @@ def _service(self): return service proxy = MyProxy() - barrier = threading.Barrier(8) seen = [] seen_lock = threading.Lock() - def work(): - barrier.wait() + def work(i): # Resolve outside the lock -- holding it here would serialise # the very access this test is trying to race. service = proxy._service with seen_lock: seen.append(service) - threads = [threading.Thread(target=work) for _ in range(8)] - for t in threads: - t.start() - for t in threads: - t.join() + assert not race(work) assert len(built) == 1 assert len({id(s) for s in seen}) == 1 @@ -193,61 +225,37 @@ def test_concurrent_mutation_and_iteration(self): # free-threaded build now means the mutex is on. This is the # workload that used to segfault the interpreter. c = LRUCache(limit=50) - barrier = threading.Barrier(8) - errors = [] def work(i): - barrier.wait() - try: - for n in range(200): - c[f"{i}-{n}"] = n - list(c.keys()) - list(c.items()) - list(c.values()) - except BaseException as exc: # pragma: no cover - errors.append(exc) - - threads = [threading.Thread(target=work, args=(i,)) for i in range(8)] - for t in threads: - t.start() - for t in threads: - t.join() - - assert not errors + for n in range(200): + c[f"{i}-{n}"] = n + list(c.keys()) + list(c.items()) + list(c.values()) + + assert not race(work) def test_concurrent_mapping_surface(self): # The test above only drives the methods LRUCache defines itself. # Every other mapping operation used to be inherited straight from # FastUserDict, reaching self.data with the mutex released. c = LRUCache(limit=50) - barrier = threading.Barrier(8) - errors = [] def work(i): - barrier.wait() - try: - for n in range(200): - key = f"{i}-{n}" - c[key] = n - len(c) - key in c # noqa: B015 - repr(c) - c.copy() - c.get(key) - c.setdefault(f"sd-{i}", n) - c.pop(key, None) - if not n % 50: - c.clear() - except BaseException as exc: # pragma: no cover - errors.append(exc) - - threads = [threading.Thread(target=work, args=(i,)) for i in range(8)] - for t in threads: - t.start() - for t in threads: - t.join() - - assert not errors + for n in range(200): + key = f"{i}-{n}" + c[key] = n + len(c) + key in c # noqa: B015 + repr(c) + c.copy() + c.get(key) + c.setdefault(f"sd-{i}", n) + c.pop(key, None) + if not n % 50: + c.clear() + + assert not race(work) class test_LRUCache_takes_the_mutex: @@ -464,31 +472,19 @@ class Owner: owner = Owner() signal = Owner.sig - barrier = threading.Barrier(8) - errors = [] def work(i): - barrier.wait() - try: - for _n in range(200): - - async def handler(*args, **kwargs): ... - - if i % 2: - signal.connect(handler) - signal.disconnect(handler) - else: - list(signal.iter_receivers(owner)) - except BaseException as exc: # pragma: no cover - errors.append(exc) - - threads = [threading.Thread(target=work, args=(i,)) for i in range(8)] - for t in threads: - t.start() - for t in threads: - t.join() - - assert not errors + for _n in range(200): + + async def handler(*args, **kwargs): ... + + if i % 2: + signal.connect(handler) + signal.disconnect(handler) + else: + list(signal.iter_receivers(owner)) + + assert not race(work) # Every connect above was paired with a disconnect, so the set has # to be empty. Without this the test proved much less than it # looked like it did: disconnect() was a no-op for strong From 018f4d37a7e0ff4fb2c8f76cb3253e4aeda7098a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 07:15:25 +0000 Subject: [PATCH 15/16] Evict atomically, and stop racing the unlocked default in tests The Python 3.13 leg failed test_concurrent_mutation_and_iteration with "OrderedDict mutated during iteration" -- a flake, not a regression, and one that predates this branch: the test hammers the *default* LRUCache configuration, which on a GIL build is still thread_safety=False, i.e. no mutex at all. The eviction in __setitem__ was self.data.pop(next(iter(self.data))) and unlocked, a GIL switch between `iter` and `next` while another thread inserts raises exactly that RuntimeError; two threads resolving the same oldest key make the loser's `pop` raise KeyError instead. At a 1 microsecond switch interval the pair reproduces in nearly every trial on stock CPython; at the default interval it is a once-in-many-runs CI failure, which is what happened. Two changes: Evict with popitem(last=False). Identical under the mutex -- it is the very operation the free-threading doc's benchmark keeps OrderedDict for, and update() already evicted this way -- but it is one C call on CPython, so a legacy unlocked cache no longer has the iter/next/pop windows. Narrower, not safe: the check-then-act around it still races unlocked (over-eviction, or KeyError when a concurrent clear empties the cache between check and call), and PyPy's popitem is Python-level, so the mutex remains the only real answer there. Because no stress test can tell the two eviction forms apart on a locked cache -- and a revert would pass the entire suite -- the mechanism gets a deterministic guard: the backing dict records __iter__ calls, and an eviction that iterates fails the test on every OrderedDict implementation. Hammer thread_safety=True, not the default. On a free-threaded build they are the same configuration -- the default resolves to True, which has its own test -- so the workload that used to segfault is exercised unchanged there. On a GIL build the default is deliberately unlocked and promises nothing under concurrent mutation; a test racing it can only pass by luck, which it now stops relying on. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CjHx3ivL7WAoHZTBMWMQPQ --- docs/free-threading.md | 24 +++++++++++++ mode/utils/collections.py | 17 +++++++++- tests/functional/test_thread_safety.py | 47 +++++++++++++++++++++++--- 3 files changed, 82 insertions(+), 6 deletions(-) diff --git a/docs/free-threading.md b/docs/free-threading.md index 84bfad0..4f82e37 100644 --- a/docs/free-threading.md +++ b/docs/free-threading.md @@ -120,6 +120,30 @@ corrupts its internal linked list. with a `nullcontext` for a mutex, exactly the configuration `__init__` refuses. It now upgrades `thread_safety` to `True` instead, which keeps old pickles loadable where raising would not. +- Evicting with `popitem(last=False)` instead of the historical + `pop(next(iter(data)))`. Under the mutex they are equivalent (and it is + the very operation the table below keeps `OrderedDict` for), but GIL + builds still permit — and default to — sharing an *unlocked* cache, and + there the three-call form races: a switch between `iter` and `next` + while another thread inserts raises "OrderedDict mutated during + iteration", and two threads resolving the same oldest key make the + loser's `pop` raise `KeyError`. CI caught the first flavor on a stock + 3.13 run; at a 1µs switch interval it reproduces in almost every trial, + and the single-call form takes both windows away on CPython, where the + C `popitem` is atomic under the GIL (PyPy's is Python-level and can + itself raise mid-iteration unlocked — the mutex is the only fix + there). This narrows the + unlocked race, it does not close it: the check-then-act around the call + can still over-evict, and `popitem` still raises `KeyError` if another + thread empties the cache between the check and the call. Thread safety + remains the mutex's job — which is why the concurrency tests hammer + `thread_safety=True` explicitly rather than the default: on + free-threaded builds that is the same configuration the default + resolves to, and on GIL builds the unlocked default makes no promise + under concurrent mutation for a test to assert. The eviction mechanism + itself has a deterministic guard + (`test_eviction_does_not_iterate_the_data`), since on a locked cache no + stress test can tell the two forms apart. ### Why not just swap `OrderedDict` for `dict`? diff --git a/mode/utils/collections.py b/mode/utils/collections.py index 716d42f..655d162 100644 --- a/mode/utils/collections.py +++ b/mode/utils/collections.py @@ -534,7 +534,22 @@ def __setitem__(self, key: KT, value: VT) -> None: and self.limit and len(self.data) >= self.limit ): - self.data.pop(next(iter(self.data))) + # popitem(last=False) drops the oldest entry, same as the + # historical `pop(next(iter(data)))` -- but in one call + # instead of three. Under the mutex they are equivalent; + # this matters for a cache shared between threads *without* + # the mutex, which GIL builds still permit (and default + # to). There a switch between `iter` and `next` while + # another thread inserts raises "OrderedDict mutated + # during iteration", and two threads resolving the same + # oldest key makes the loser's `pop` raise KeyError. On + # CPython the single call is atomic (PyPy's popitem is + # Python-level, so it is not); and the check-then-act + # around it still races unlocked -- over-eviction, or a + # KeyError from `popitem` when another thread empties the + # cache first. A narrower window, not thread safety: + # that remains the mutex's job. + self.data.popitem(last=False) self.data[key] = value # NOTE: Iteration takes a snapshot under the mutex and yields from that diff --git a/tests/functional/test_thread_safety.py b/tests/functional/test_thread_safety.py index 0bf952b..9a3cdd4 100644 --- a/tests/functional/test_thread_safety.py +++ b/tests/functional/test_thread_safety.py @@ -197,6 +197,35 @@ def test_limit_still_evicts_oldest(self): c[i] = i assert list(c.keys()) == [7, 8, 9] + def test_eviction_does_not_iterate_the_data(self): + # Eviction must be `popitem(last=False)` -- one call -- and not + # the historical `pop(next(iter(data)))`. An *unlocked* cache on + # a GIL build (the historical default there) races the latter's + # iter/next/pop gaps: a switch between `iter` and `next` while + # another thread inserts raises "OrderedDict mutated during + # iteration", and two threads resolving the same oldest key make + # the loser's `pop` raise KeyError. A stress test cannot tell + # the two forms apart on a locked cache, so assert the mechanism + # itself, by recording `__iter__` calls on the backing dict. + # Recording, not raising: whether *other* operations -- the + # views, `dict()` -- route through `__iter__` varies between + # OrderedDict implementations (C, pure-Python, PyPy), and only + # iteration *during the fill* is the defect. `popitem` itself + # iterates on none of them. + iterations = [] + + class RecordingData(OrderedDict): + def __iter__(self): + iterations.append(True) + return super().__iter__() + + c = LRUCache(limit=3) + c.data = RecordingData() + for i in range(10): + c[i] = i + assert not iterations, "eviction iterated the backing dict" + assert list(c.data.keys()) == [7, 8, 9] + def test_iteration_does_not_hold_the_lock_across_yields(self): # A half-consumed iterator must not keep the mutex held: the lock # is reentrant, so only a *different* thread shows the problem. @@ -221,10 +250,16 @@ def writer(): assert c["d"] == 4 def test_concurrent_mutation_and_iteration(self): - # Deliberately the *default* configuration -- which on a - # free-threaded build now means the mutex is on. This is the - # workload that used to segfault the interpreter. - c = LRUCache(limit=50) + # thread_safety=True explicitly, NOT the default. On a + # free-threaded build they are the same configuration -- the + # default resolves to True there, which has its own test above -- + # so this still hammers the exact setup that used to segfault the + # interpreter. On a GIL build the default is *deliberately* + # unlocked, and racing that asserts nothing the class promises: + # the eviction in `__setitem__` is a check-then-act that a GIL + # switch can split, which surfaced in CI as a one-in-many-runs + # "OrderedDict mutated during iteration". + c = LRUCache(limit=50, thread_safety=True) def work(i): for n in range(200): @@ -239,7 +274,9 @@ def test_concurrent_mapping_surface(self): # The test above only drives the methods LRUCache defines itself. # Every other mapping operation used to be inherited straight from # FastUserDict, reaching self.data with the mutex released. - c = LRUCache(limit=50) + # thread_safety=True for the same reason as above: the locked + # configuration is the one that promises this workload is safe. + c = LRUCache(limit=50, thread_safety=True) def work(i): for n in range(200): From 6388a96d34dd062795b2e20ebad87351d3a1edab Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 07:43:08 +0000 Subject: [PATCH 16/16] Simplify the free-threading changes A four-angle cleanup pass (reuse, simplification, efficiency, altitude) over the PR's diff. No behavioural changes intended beyond the ones called out below. LRUCache hot paths take the mutex once instead of three times. `get` and `setdefault` wrapped `self[key]`, whose `__getitem__` re-entered `__setitem__` for the LRU touch -- three RLock acquisitions per cache hit. The unlocked bodies now live in `_touch` (pop/re-insert) and `_store` (evict/insert), each public method locks once, and the `get` override is deleted outright: its only data access was the single, already-locked `self[key]`, so it duplicated `Mapping.get` for nothing. Parameterizing the base as `FastUserDict[KT, VT]` keeps the inherited `get` fully typed and dissolves the NOTE apologising for `setdefault`'s erased signature. The eviction guard also runs its cheap size checks before the containment probe, and `_keys`/`_values`/`_items` collapse into `iter(self.copy()...)` -- one home for the snapshot idiom and no generator frame per element. The hand-maintained invariants are now enforced, not described. A method added to `FastUserDict` without a matching LRUCache override used to be a silent segfault-on-3.14t waiting to happen (the NOTE admitted it); a reflective test now fails on it. `mode/__init__.py.__all__` matching `all_by_module` is asserted the same way, and `__dir__` is derived from the module namespace instead of a template-era list that advertised VERSION/version_info -- names no version of mode ever defined. The stress harness follows the bounded-wait policy it was built to police. Its `race()` mirrored the CI helper's shape but kept unbounded `barrier.wait`/`join`, so the next wedge-class regression would have hung a manual run instead of naming the failing check; waits are bounded now, the cold-import subprocess gets a timeout, the three LRU checks share one trial loop, and the seven hand-rolled `[FAIL]/[ok ]` lines go through a helper. The banner works on interpreters without `sys._is_gil_enabled` via the same fallback `FREE_THREADED` uses. Test cleanups: pure eviction/popitem/ordering semantics live only in `test_LRUCache_ordering` (thread-safety keeps what involves the mutex or threads); the shared `handler` fixture replaces per-test inline copies; `recwarn` replaces a hand-rolled catch_warnings context manager; the signal-mutation test drops connect-loop scaffolding that only made sense when strong receivers were wrapped; `__setstate__` no longer defensively copies its state dict (no caller can observe the difference), and the test pinning that courtesy went with it. Comment trims where the same story was told three or four times: the popitem rationale, the strong-receiver storage rationale and the class docstring benchmark now say it once and point at docs/free-threading.md. Reviewed but deliberately left alone: cached_property's descriptor-level lock (per-instance schemes leak state into instance dicts or need id-keyed tables; the trade-off is documented where it diverges from stdlib), the DEPRECATED_LOOPS mapping, the tri-state thread_safety default (a def-time-bound `= FREE_THREADED` default would go stale under monkeypatching), and the deliberate duplication between the CI race helper and the standalone stress script, which cannot import test packages. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CjHx3ivL7WAoHZTBMWMQPQ --- mode/__init__.py | 23 ++- mode/signals.py | 29 ++-- mode/utils/collections.py | 148 ++++++---------- tests/freethreading/stress.py | 190 +++++++++++---------- tests/functional/test_signals.py | 36 ++-- tests/functional/test_thread_safety.py | 82 ++++----- tests/functional/utils/test_collections.py | 14 +- tests/unit/test_loop.py | 30 ++-- 8 files changed, 253 insertions(+), 299 deletions(-) diff --git a/mode/__init__.py b/mode/__init__.py index 2f377c2..b6c80c9 100644 --- a/mode/__init__.py +++ b/mode/__init__.py @@ -31,6 +31,10 @@ from .utils.times import Seconds, want_seconds from .worker import Worker +# NOTE: Must list exactly the names in `all_by_module` below. It is kept +# as a literal (rather than derived) because ruff and mypy only recognise +# the TYPE_CHECKING re-exports through a literal __all__; the sync is +# enforced by tests/functional/test_thread_safety.py's lazy-import tests. __all__ = [ "BaseSignal", "BaseSignalT", @@ -117,16 +121,9 @@ def __getattr__(name: str) -> Any: def __dir__() -> Sequence[str]: - return [ - *__all__, - "__file__", - "__path__", - "__doc__", - "__all__", - "__docformat__", - "__name__", - "VERSION", - "version_info", - "__package__", - "__version__", - ] + # Everything actually in the module namespace, plus the lazy exports + # __getattr__ can still resolve. Derived so it cannot drift into + # advertising names that do not exist (the old hand-written list was + # carried over from a template and promised VERSION/version_info, + # which no version of this module ever defined). + return sorted(set(globals()) | set(object_origins)) diff --git a/mode/signals.py b/mode/signals.py index bf90a0c..b814062 100644 --- a/mode/signals.py +++ b/mode/signals.py @@ -113,20 +113,13 @@ def _connect( self, fun: SignalHandlerT, *, weak: bool = False, sender: Any = None ) -> SignalHandlerT: ref: SignalHandlerRefT - # NOTE: A strong receiver is stored as the handler itself, not - # wrapped in anything. `disconnect` needs to find the stored - # entry by value, and a handler already hashes and compares the - # way that requires: functions by identity, and bound methods by - # ``(__func__, __self__)`` -- so ``owner.handler``, a fresh object - # on every attribute access, still matches the one connect stored. - # - # Just as important, both of those comparisons are implemented in - # the interpreter rather than in Python. A wrapper defining - # ``__eq__``/``__hash__`` in Python would make every ``add`` and - # ``discard`` below re-enter the interpreter mid-operation, which - # releases the GIL and lets another thread mutate this set while - # the lookup that called us is walking it. On PyPy that - # reliably wedges a concurrent connect/disconnect loop. + # NOTE: A strong receiver is stored as the handler itself, + # unwrapped. Handlers already hash and compare the way + # `disconnect` needs (functions by identity, bound methods by + # ``(__func__, __self__)``), and keeping Python-level + # __hash__/__eq__ out of the receiver set keeps `set.add` and + # `set.discard` atomic -- a wrapper re-entering the interpreter + # mid-operation reliably wedged PyPy; see docs/free-threading.md. ref = self._create_ref(fun) if weak else fun if self.default_sender is not None: sender = self.default_sender @@ -141,11 +134,9 @@ def disconnect( ) -> None: ref: SignalHandlerRefT # Mirrors `_connect`: a strong receiver is the handler itself, so - # the value built here compares equal to the one stored there. - # This used to be ``lambda: fun``, and `_connect` stored a - # *different* lambda -- two lambdas are never equal, so the - # `discard` below matched nothing and the receiver stayed - # connected forever. + # the value built here compares equal to the stored entry. (It + # was once a fresh ``lambda: fun``, which never matched -- making + # disconnect a silent no-op for strong receivers.) ref = self._create_ref(fun) if weak else fun if self.default_sender is not None: sender = self.default_sender diff --git a/mode/utils/collections.py b/mode/utils/collections.py index 655d162..e1d46a7 100644 --- a/mode/utils/collections.py +++ b/mode/utils/collections.py @@ -439,7 +439,7 @@ def __iter__(self) -> Iterator[tuple[KT, VT]]: yield from self._mapping._items() -class LRUCache(FastUserDict, MutableMapping[KT, VT], MappingViewProxy): +class LRUCache(FastUserDict[KT, VT], MutableMapping[KT, VT], MappingViewProxy): """LRU Cache implementation using a doubly linked list to track access. Arguments: @@ -456,20 +456,15 @@ class LRUCache(FastUserDict, MutableMapping[KT, VT], MappingViewProxy): Note: The backing store is an :class:`~collections.OrderedDict` rather - than a plain :class:`dict`, even though `dict` has preserved - insertion order since Python 3.7. The reason is - `popitem(last=False)`: evicting the oldest entry is this class's - hot path, and `OrderedDict` does it in O(1) via its linked list, - while the `dict` equivalent (`d.pop(next(iter(d)))`) has to scan - past every slot vacated since the last resize. Measured on a - steady-state evict-and-insert loop, `dict` was ~3x slower at 1,000 - entries and ~110x slower at 100,000. - - The cost of that linked list is that `OrderedDict` is not safe to - mutate concurrently on free-threaded builds -- racing threads - corrupt it badly enough to segfault the interpreter, where `dict` - would merely raise. So on those builds the mutex is mandatory - rather than merely on by default. + than a plain :class:`dict`: evicting the oldest entry is this + class's hot path, and ``popitem(last=False)`` does it in O(1) via + the linked list, where the ``dict`` equivalent degrades badly as + the cache grows (measured in ``docs/free-threading.md``). The + cost of that linked list is that `OrderedDict` is not safe to + mutate concurrently on free-threaded builds -- racing threads can + corrupt it badly enough to segfault the interpreter -- so on + those builds the mutex is mandatory rather than merely on by + default. """ limit: Optional[int] @@ -504,8 +499,14 @@ def __init__( def __getitem__(self, key: KT) -> VT: with self._mutex: - value = self[key] = self.data.pop(key) - return cast(VT, value) + return self._touch(key) + + def _touch(self, key: KT) -> VT: + # Caller must hold the mutex. Pop and re-insert to mark the key + # most recently used. + value = self.data.pop(key) + self.data[key] = value + return cast(VT, value) def update(self, *args: Any, **kwargs: Any) -> None: with self._mutex: @@ -521,45 +522,32 @@ def popitem(self, *, last: bool = True) -> tuple[KT, VT]: return self.data.popitem(last) def __setitem__(self, key: KT, value: VT) -> None: - # remove least recently used key. with self._mutex: - # NOTE: `key not in self.data` matters. Updating a key that is - # already present does not grow the cache, so evicting to make - # room for it discards an unrelated entry for nothing -- a full - # cache would shrink below its own limit on every such update - # (limit=3 holding a/b/c, then `cache["c"] = ...`, used to leave - # two entries and drop "a"). - if ( - key not in self.data - and self.limit - and len(self.data) >= self.limit - ): - # popitem(last=False) drops the oldest entry, same as the - # historical `pop(next(iter(data)))` -- but in one call - # instead of three. Under the mutex they are equivalent; - # this matters for a cache shared between threads *without* - # the mutex, which GIL builds still permit (and default - # to). There a switch between `iter` and `next` while - # another thread inserts raises "OrderedDict mutated - # during iteration", and two threads resolving the same - # oldest key makes the loser's `pop` raise KeyError. On - # CPython the single call is atomic (PyPy's popitem is - # Python-level, so it is not); and the check-then-act - # around it still races unlocked -- over-eviction, or a - # KeyError from `popitem` when another thread empties the - # cache first. A narrower window, not thread safety: - # that remains the mutex's job. - self.data.popitem(last=False) - self.data[key] = value - - # NOTE: Iteration takes a snapshot under the mutex and yields from that - # snapshot with the mutex released, rather than holding it across the - # yields. Holding a lock across a yield keeps it held for as long as - # the *consumer* takes to iterate -- and forever if the consumer - # abandons the generator half way, since the mutex is only released - # when the generator is closed. Snapshotting also means a concurrent - # writer cannot invalidate an iteration already in progress, which is - # what "dictionary changed size during iteration" used to be. + self._store(key, value) + + def _store(self, key: KT, value: VT) -> None: + # Caller must hold the mutex. Evict the least recently used + # entry when inserting a *new* key into a full cache -- updating + # an existing key does not grow the cache, so evicting for it + # would shrink a full cache on every such update. The cheap size + # checks run first so unlimited and under-limit caches skip the + # containment probe. Eviction is a single `popitem(last=False)` + # call, NOT `pop(next(iter(...)))`, so that even an unlocked + # cache on a GIL build (the historical default there) cannot race + # the iter/next/pop gaps; docs/free-threading.md has the story. + if ( + self.limit + and len(self.data) >= self.limit + and key not in self.data + ): + self.data.popitem(last=False) + self.data[key] = value + + # NOTE: Iteration operates on a `copy()` snapshot taken under the + # mutex, iterated with the mutex released. Holding the lock across + # yields would keep it held for as long as the *consumer* takes -- + # forever, if a generator is abandoned half-consumed -- and iterating + # the live dict is what "changed size during iteration" used to be. def __iter__(self) -> Iterator: return self._keys() @@ -568,33 +556,26 @@ def keys(self) -> KeysView[KT]: return ProxyKeysView(self) def _keys(self) -> Iterator[KT]: - # userdict.keys in py3k calls __getitem__ - with self._mutex: - keys = list(self.data) - yield from keys + return iter(self.copy()) def values(self) -> ValuesView[VT]: return ProxyValuesView(self) def _values(self) -> Iterator[VT]: - with self._mutex: - values = list(self.data.values()) - yield from values + return iter(self.copy().values()) def items(self) -> ItemsView[KT, VT]: return ProxyItemsView(self) def _items(self) -> Iterator[tuple[KT, VT]]: - with self._mutex: - items = list(self.data.items()) - yield from items + return iter(self.copy().items()) def incr(self, key: KT, delta: int = 1) -> int: with self._mutex: # this acts as memcached does- store as a string, but return a # integer as long as it exists and we can cast it newval = int(self.data.pop(key)) + delta - self[key] = cast(VT, str(newval)) + self._store(key, cast(VT, str(newval))) return newval # NOTE: Everything below re-implements an inherited method that would @@ -631,12 +612,13 @@ def clear(self) -> None: with self._mutex: self.data.clear() - # The compound `MutableMapping` helpers below are inherited as - # combinations of the primitives above. Each primitive is locked, so - # inheriting them would already be memory-safe, but the lock is - # dropped between the lookup and the store -- which for a class that - # advertises thread safety is a surprising place to lose an - # invariant. They are made atomic instead. + # `pop` and `setdefault` below are inherited as combinations of the + # locked primitives above, which is already memory-safe -- but the + # lock is dropped between the lookup and the store, a surprising + # place for a class that advertises thread safety to lose an + # invariant. They are made atomic instead. (`get` needs no + # override: its only data access is the single, already-locked + # `self[key]`.) @overload def pop(self, key: KT) -> VT: ... @@ -653,29 +635,12 @@ def pop(self, key: KT, default: Any = _MISSING) -> Any: raise return default - # NOTE: Not overloaded like `pop` and `get` above. `LRUCache` lists - # `FastUserDict` unparameterized among its bases, so the inherited - # `setdefault` erases to `(Any, None = ...) -> Any | None`, and a - # narrower `(KT, VT) -> VT` pair here is an incompatible override. def setdefault(self, key: KT, default: Any = None) -> Any: with self._mutex: try: - return self[key] - except KeyError: - self[key] = cast(VT, default) - return default - - @overload - def get(self, key: KT) -> Optional[VT]: ... - - @overload - def get(self, key: KT, default: Union[VT, T]) -> Union[VT, T]: ... - - def get(self, key: KT, default: Any = None) -> Any: - with self._mutex: - try: - return self[key] + return self._touch(key) except KeyError: + self._store(key, cast(VT, default)) return default def _new_lock(self) -> AbstractContextManager: @@ -696,7 +661,6 @@ def __setstate__(self, state: dict[str, Any]) -> None: # both the default and legal -- would otherwise come back here # with `nullcontext` for a mutex. Upgrading the flag keeps those # pickles loadable, which raising would not. - state = dict(state) if FREE_THREADED and not state.get("thread_safety", False): state["thread_safety"] = True self.__dict__ = state diff --git a/tests/freethreading/stress.py b/tests/freethreading/stress.py index aa3ac6a..ffabe2c 100644 --- a/tests/freethreading/stress.py +++ b/tests/freethreading/stress.py @@ -28,33 +28,49 @@ import sys import threading +import time import traceback NTHREADS = 16 +#: Upper bound on any single race() call. A wedge-class regression -- the +#: kind this harness exists to catch -- must fail the check that hit it, +#: not hang a manual run: the same bounded-wait policy as the CI-facing +#: `race` helper in tests/functional/test_thread_safety.py. +RACE_TIMEOUT = 60.0 -def race(target, nthreads=NTHREADS): + +def race(target, nthreads=NTHREADS, timeout=RACE_TIMEOUT): """Run ``target(i)`` in ``nthreads`` threads released by a barrier. Returns the list of tracebacks raised by the threads (empty if none). + A thread still running after `timeout` is reported as an error. """ barrier = threading.Barrier(nthreads) errors = [] def wrapper(i): - barrier.wait() try: + barrier.wait(timeout=timeout) target(i) except BaseException: errors.append(traceback.format_exc()) threads = [ - threading.Thread(target=wrapper, args=(i,)) for i in range(nthreads) + threading.Thread(target=wrapper, args=(i,), daemon=True) + for i in range(nthreads) ] for t in threads: t.start() + deadline = time.monotonic() + timeout for t in threads: - t.join() + t.join(timeout=max(0.0, deadline - time.monotonic())) + wedged = sum(1 for t in threads if t.is_alive()) + if wedged: + errors.append( + f"WEDGED: {wedged}/{nthreads} threads still running " + f"after {timeout}s" + ) return errors @@ -64,7 +80,11 @@ def report(name, errors, note=""): print(f"[FAIL] {name}: {len(errors)} threads -> {last_line}") else: print(f"[ok ] {name} {note}".rstrip()) - return bool(errors) + + +def report_trials(name, failed, summary): + """The `[FAIL]/[ok ]` line for checks that count failing trials.""" + print(f"[{'FAIL' if failed else 'ok '}] {name}: {summary}") # -------------------------------------------------------------------------- @@ -75,84 +95,65 @@ def report(name, errors, note=""): # is now mandatory on free-threaded builds, so the default config is safe # and thread_safety=False is refused there. # -------------------------------------------------------------------------- -def check_lru_default(trials=60): +def _lru_iteration_workload(i, cache): + for n in range(100): + cache[f"{i}-{n}"] = n + list(cache.keys()) + list(cache.items()) + + +def _lru_mapping_workload(i, cache): + for n in range(100): + key = f"{i}-{n}" + cache[key] = n + len(cache) + key in cache # noqa: B015 + repr(cache) + cache.copy() + cache.get(key) + cache.setdefault(f"sd-{i}", n) + cache.pop(key, None) + try: + del cache[f"{i}-{n - 1}"] + except KeyError: + pass + if not n % 25: + cache.clear() + + +def _lru_trials(name, workload, trials, **kwargs): from mode.utils.collections import LRUCache - print(" (this configuration segfaulted before the fix)", flush=True) bad = 0 for _ in range(trials): - cache = LRUCache(limit=50) - - def work(i, cache=cache): - for n in range(100): - cache[f"{i}-{n}"] = n - list(cache.keys()) - - if race(work): + cache = LRUCache(limit=50, **kwargs) + if race(lambda i, cache=cache: workload(i, cache)): bad += 1 - print( - f"[{'FAIL' if bad else 'ok '}] LRUCache(default): " - f"{bad}/{trials} trials raised" - ) + report_trials(name, bad, f"{bad}/{trials} trials raised") -def check_lru_thread_safe(trials=20): - from mode.utils.collections import LRUCache - - bad = 0 - for _ in range(trials): - cache = LRUCache(limit=50, thread_safety=True) +def check_lru_default(trials=60): + print(" (this configuration segfaulted before the fix)", flush=True) + _lru_trials("LRUCache(default)", _lru_iteration_workload, trials) - def work(i, cache=cache): - for n in range(100): - cache[f"{i}-{n}"] = n - list(cache.keys()) - list(cache.items()) - if race(work): - bad += 1 - print( - f"[{'FAIL' if bad else 'ok '}] LRUCache(thread_safety=True): " - f"{bad}/{trials} trials raised" +def check_lru_thread_safe(trials=20): + _lru_trials( + "LRUCache(thread_safety=True)", + _lru_iteration_workload, + trials, + thread_safety=True, ) def check_lru_mapping_surface(trials=40): - # The checks above only drive the methods LRUCache defines itself. - # Every other mapping operation used to be inherited from FastUserDict, - # which reaches self.data with the mutex released -- so `del`, `clear`, - # `copy`, `len`, `in` and `repr` had the same unguarded OrderedDict - # access that the segfault came from. Race them against writers. - from mode.utils.collections import LRUCache - - bad = 0 - for _ in range(trials): - cache = LRUCache(limit=50) - - def work(i, cache=cache): - for n in range(100): - key = f"{i}-{n}" - cache[key] = n - len(cache) - key in cache # noqa: B015 - repr(cache) - cache.copy() - cache.get(key) - cache.setdefault(f"sd-{i}", n) - cache.pop(key, None) - try: - del cache[f"{i}-{n - 1}"] - except KeyError: - pass - if not n % 25: - cache.clear() - - if race(work): - bad += 1 - print( - f"[{'FAIL' if bad else 'ok '}] LRUCache(mapping surface): " - f"{bad}/{trials} trials raised" - ) + # The iteration workload only drives the methods LRUCache defines + # itself. Every other mapping operation used to be inherited from + # FastUserDict, which reaches self.data with the mutex released -- so + # `del`, `clear`, `copy`, `len`, `in` and `repr` had the same + # unguarded OrderedDict access that the segfault came from. Race + # them against writers. + _lru_trials("LRUCache(mapping surface)", _lru_mapping_workload, trials) # -------------------------------------------------------------------------- @@ -187,10 +188,11 @@ def work(i, x=x, seen=seen, lock=lock): race(work) if len({id(v) for v in seen}) != 1: bad += 1 - print( - f"[{'FAIL' if bad else 'ok '}] cached_property: {bad}/{trials} " - f"trials returned >1 distinct object " - f"({computes[0]} computes for {trials} properties)" + report_trials( + "cached_property", + bad, + f"{bad}/{trials} trials returned >1 distinct object " + f"({computes[0]} computes for {trials} properties)", ) @@ -224,9 +226,10 @@ def work(i, proxy=proxy, seen=seen, seen_lock=seen_lock): race(work) if len({id(s) for s in seen}) != 1 or len(built) != 1: bad += 1 - print( - f"[{'FAIL' if bad else 'ok '}] ServiceProxy._service: " - f"{bad}/{trials} trials built/returned >1 Service instance" + report_trials( + "ServiceProxy._service", + bad, + f"{bad}/{trials} trials built/returned >1 Service instance", ) @@ -276,10 +279,11 @@ async def handler(*args, **kwargs): bad += 1 if sig._receivers: leaked += 1 - print( - f"[{'FAIL' if bad or leaked else 'ok '}] Signal iter_receivers: " + report_trials( + "Signal iter_receivers", + bad or leaked, f"{bad}/{trials} trials raised, " - f"{leaked}/{trials} left receivers connected" + f"{leaked}/{trials} left receivers connected", ) @@ -445,22 +449,34 @@ def work(): bad = 0 first = "" for _ in range(trials): - proc = subprocess.run( - [sys.executable, "-c", code], capture_output=True, text=True - ) + try: + proc = subprocess.run( + [sys.executable, "-c", code], + capture_output=True, + text=True, + timeout=RACE_TIMEOUT, + ) + except subprocess.TimeoutExpired: + bad += 1 + first = first or f"child wedged for {RACE_TIMEOUT}s" + continue if proc.returncode: bad += 1 first = first or proc.stdout.strip().splitlines()[-1] - print( - f"[{'FAIL' if bad else 'ok '}] concurrent cold `import mode`: " + report_trials( + "concurrent cold `import mode`", + bad, f"{bad}/{trials} runs had a failing thread" - + (f" -> {first}" if first else "") + + (f" -> {first}" if first else ""), ) def main(): print(f"python: {sys.version.splitlines()[0]}") - print(f"GIL enabled: {sys._is_gil_enabled()}\n") + # Same fallback as mode.utils.collections.FREE_THREADED: builds + # without the attribute (< 3.13) always have the GIL. + gil = getattr(sys, "_is_gil_enabled", lambda: True)() + print(f"GIL enabled: {gil}\n") print("-- surfaces verified safe --") check_service_subclass_creation() diff --git a/tests/functional/test_signals.py b/tests/functional/test_signals.py index 64ea635..2f07bc5 100644 --- a/tests/functional/test_signals.py +++ b/tests/functional/test_signals.py @@ -248,6 +248,13 @@ def foo(self, **kwargs): assert sig._create_ref(X().foo) +@pytest.fixture +def handler(): + async def handler(*args: Any, **kwargs: Any) -> None: ... + + return handler + + class test_disconnect_removes_the_receiver: """`disconnect` has to undo `connect`, strong references included. @@ -257,12 +264,6 @@ class test_disconnect_removes_the_receiver: connected -- and stayed subscribed to every subsequent send. """ - @pytest.fixture - def handler(self): - async def handler(*args: Any, **kwargs: Any) -> None: ... - - return handler - def test_strong_receiver(self, handler): sig = Signal() sig.connect(handler) @@ -358,24 +359,15 @@ class test_strong_receivers_are_stored_unwrapped: thread mutate the set underneath it. """ - def test_the_set_holds_the_handler(self): - async def fun(*args: Any, **kwargs: Any) -> None: ... - - sig = Signal() - sig.connect(fun) - assert set(sig._receivers) == {fun} - - def test_the_stored_receiver_is_not_a_callable_wrapper(self): + def test_the_stored_receiver_is_the_handler_itself(self, handler): # `_is_alive` distinguishes weak from strong by asking whether the # entry is a `weakref`, so a strong entry must be the handler and # not something that returns it when called. - async def fun(*args: Any, **kwargs: Any) -> None: ... - sig = Signal() - sig.connect(fun) + sig.connect(handler) (stored,) = sig._receivers - assert stored is fun - assert sig._is_alive(stored) == (True, fun) + assert stored is handler + assert sig._is_alive(stored) == (True, handler) def test_weak_and_strong_receivers_coexist(self): async def strong(*args: Any, **kwargs: Any) -> None: ... @@ -387,13 +379,11 @@ async def weak(*args: Any, **kwargs: Any) -> None: ... sig.connect(weak, weak=True) assert set(sig.iter_receivers(object())) == {strong, weak} - def test_hashing_is_not_implemented_in_python(self): + def test_hashing_is_not_implemented_in_python(self, handler): # The point of storing the handler bare: `set.add`/`set.discard` # must not call back into Python to hash or compare an entry. - async def fun(*args: Any, **kwargs: Any) -> None: ... - sig = Signal() - sig.connect(fun) + sig.connect(handler) (stored,) = sig._receivers assert type(stored).__hash__ is object.__hash__ assert type(stored).__eq__ is object.__eq__ diff --git a/tests/functional/test_thread_safety.py b/tests/functional/test_thread_safety.py index 9a3cdd4..76f5d94 100644 --- a/tests/functional/test_thread_safety.py +++ b/tests/functional/test_thread_safety.py @@ -9,6 +9,7 @@ `tests/freethreading/stress.py` for the heavier probabilistic reproducers. """ +import inspect import pickle import sys import threading @@ -22,7 +23,7 @@ import mode from mode.proxy import ServiceProxy from mode.signals import Signal -from mode.utils.collections import FREE_THREADED, LRUCache +from mode.utils.collections import FREE_THREADED, FastUserDict, LRUCache from mode.utils.objects import cached_property #: Upper bound for any one concurrency test below. Generous: these @@ -173,29 +174,9 @@ def test_thread_safety_cannot_be_disabled_when_free_threaded(self): else: assert LRUCache(thread_safety=False).thread_safety is False - def test_popitem_last_is_lifo(self): - c = LRUCache() - c.update({"a": 1, "b": 2, "c": 3}) - assert c.popitem() == ("c", 3) - assert c.popitem(last=True) == ("b", 2) - - def test_popitem_first_is_fifo(self): - c = LRUCache() - c.update({"a": 1, "b": 2, "c": 3}) - assert c.popitem(last=False) == ("a", 1) - assert c.popitem(last=False) == ("b", 2) - - def test_popitem_empty_raises_KeyError(self): - with pytest.raises(KeyError): - LRUCache().popitem() - with pytest.raises(KeyError): - LRUCache().popitem(last=False) - - def test_limit_still_evicts_oldest(self): - c = LRUCache(limit=3) - for i in range(10): - c[i] = i - assert list(c.keys()) == [7, 8, 9] + # (Pure eviction/popitem/ordering semantics live in + # tests/functional/utils/test_collections.py::test_LRUCache_ordering; + # this class only covers what involves the mutex or threads.) def test_eviction_does_not_iterate_the_data(self): # Eviction must be `popitem(last=False)` -- one call -- and not @@ -316,10 +297,9 @@ def __exit__(self, *exc_info: object) -> None: def assert_takes_mutex(self, operation): cache = LRUCache(limit=10, thread_safety=True) - # Populate without going through the (locked) __setitem__, so the + # Populate first: the tracking mutex is installed after, so the # count below only reflects the operation under test. - cache.data["a"] = 1 - cache.data["b"] = 2 + cache.update({"a": 1, "b": 2}) mutex = self.TrackingMutex() cache._mutex = mutex @@ -356,6 +336,25 @@ def assert_takes_mutex(self, operation): def test_operation_takes_mutex(self, name, operation): self.assert_takes_mutex(operation) + def test_every_FastUserDict_method_is_overridden(self): + # The override list above is hand-maintained, and so is this + # test's parametrization -- neither notices a method *added* to + # `FastUserDict` later, which would reach `self.data` with the + # mutex released (the NOTE in LRUCache admits as much). Enforce + # the completeness invariant reflectively: every function defined + # on `FastUserDict` must be shadowed by `LRUCache` itself. + # (`fromkeys` is exempt: a classmethod that only touches data + # through the locked `update`.) + missing = [ + name + for name, member in vars(FastUserDict).items() + if inspect.isfunction(member) and name not in vars(LRUCache) + ] + assert not missing, ( + f"FastUserDict methods that LRUCache does not override " + f"(they would touch self.data without the mutex): {missing}" + ) + class test_LRUCache_mapping_semantics: """The mutex overrides must not change what the methods do.""" @@ -470,15 +469,6 @@ def test_setstate_preserves_true_thread_safety(self, monkeypatch): assert restored.thread_safety is True assert not isinstance(restored._mutex, nullcontext) - def test_setstate_does_not_mutate_the_state_it_is_given(self, monkeypatch): - monkeypatch.setattr("mode.utils.collections.FREE_THREADED", True) - state = {"limit": None, "thread_safety": False, "data": OrderedDict()} - cache = LRUCache.__new__(LRUCache) - cache.__setstate__(state) - - assert cache.thread_safety is True - assert state["thread_safety"] is False - class test_Signal_receiver_iteration: def test_get_live_receivers_tolerates_mutation(self): @@ -489,13 +479,14 @@ def test_get_live_receivers_tolerates_mutation(self): async def handler(*args, **kwargs): ... - for _ in range(4): - signal.connect(handler) + async def late_handler(*args, **kwargs): ... + + signal.connect(handler) receivers = signal._receivers original_is_alive = signal._is_alive def mutating_is_alive(ref): - receivers.add(lambda: handler) + receivers.add(late_handler) return original_is_alive(ref) signal._is_alive = mutating_is_alive @@ -563,3 +554,16 @@ def test_dir_lists_the_lazy_names(self): listed = dir(mode) for name in mode.__all__: assert name in listed + + def test_all_matches_the_lazy_export_table(self): + # `__all__` is the literal copy ruff and mypy read; `all_by_module` + # is what `__getattr__` actually resolves. A name added to one + # and not the other would silently vanish from `import *` or + # raise AttributeError -- so the two may not drift. + assert set(mode.__all__) == set(mode.object_origins) + + def test_dir_advertises_only_real_names(self): + # The old hand-written __dir__ promised VERSION/version_info, + # which no version of this module ever defined. + for name in dir(mode): + assert hasattr(mode, name), name diff --git a/tests/functional/utils/test_collections.py b/tests/functional/utils/test_collections.py index 7b66413..5d4cd5f 100644 --- a/tests/functional/utils/test_collections.py +++ b/tests/functional/utils/test_collections.py @@ -546,12 +546,8 @@ def test_get_set_update_pop(self, d): assert d.popitem() == (199, 199) - def test_iter_keys_items_values(self, d): - d.update({"a": 1, "b": 2, "c": 3}) - assert list(iter(d)) == ["a", "b", "c"] - assert list(iter(d)) == list(d.keys()) - assert list(d.values()) == [1, 2, 3] - assert list(d.items()) == [("a", 1), ("b", 2), ("c", 3)] + # (Iteration order is pinned by + # test_LRUCache_ordering.test_iteration_follows_insertion_order.) def test_incr(self, d): d["a"] = "0" @@ -644,6 +640,12 @@ def test_popitem_pops_from_either_end(self): assert c.popitem() == ("c", "c") assert c.popitem(last=False) == ("a", "a") + def test_popitem_empty_raises_KeyError(self): + with pytest.raises(KeyError): + LRUCache().popitem() + with pytest.raises(KeyError): + LRUCache().popitem(last=False) + def test_order_survives_a_pickle_round_trip(self): c = LRUCache() for key in "abc": diff --git a/tests/unit/test_loop.py b/tests/unit/test_loop.py index 180265f..a7ca551 100644 --- a/tests/unit/test_loop.py +++ b/tests/unit/test_loop.py @@ -1,5 +1,3 @@ -import warnings -from contextlib import contextmanager from unittest.mock import patch import pytest @@ -8,13 +6,6 @@ from mode.loop import DEPRECATED_LOOPS, LOOPS -@contextmanager -def recorded_warnings(): - with warnings.catch_warnings(record=True) as recorded: - warnings.simplefilter("always") - yield recorded - - class test_use: # NOTE: `importlib.import_module` is patched out throughout. Actually # selecting a backend applies process-wide monkey-patches (gevent and @@ -22,10 +13,10 @@ class test_use: # runs afterwards. @pytest.mark.parametrize("loop", ["eventlet", "gevent", "uvloop"]) - def test_imports_the_backend_module(self, loop): + def test_imports_the_backend_module(self, loop, recwarn): + # `recwarn` absorbs the gevent deprecation warning quietly. with patch("importlib.import_module") as import_module: - with recorded_warnings(): - mode.loop.use(loop) + mode.loop.use(loop) import_module.assert_called_once_with(LOOPS[loop]) def test_aio_imports_nothing(self): @@ -61,12 +52,11 @@ def test_warning_precedes_the_import(self): mode.loop.use("gevent") @pytest.mark.parametrize("loop", ["aio", "eventlet", "uvloop"]) - def test_other_backends_do_not_warn(self, loop): + def test_other_backends_do_not_warn(self, loop, recwarn): with patch("importlib.import_module"): - with recorded_warnings() as recorded: - mode.loop.use(loop) - assert not [ - w - for w in recorded - if issubclass(w.category, DeprecationWarning) - ] + mode.loop.use(loop) + assert not [ + w + for w in recwarn.list + if issubclass(w.category, DeprecationWarning) + ]