From e1f76858bd1e47ca346bb0699ff2b8668f722ab0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 19:42:38 +0000 Subject: [PATCH 01/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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):