diff --git a/modern_di/cache.py b/modern_di/cache.py index d7d7b222..301532de 100644 --- a/modern_di/cache.py +++ b/modern_di/cache.py @@ -4,11 +4,10 @@ import typing from modern_di import exceptions, types -from modern_di.providers import CacheSettings, Factory +from modern_di.providers import CacheSettings _T = typing.TypeVar("_T") -_R = typing.TypeVar("_R") _V = typing.TypeVar("_V") @@ -24,13 +23,8 @@ def clear(self) -> None: self.cache = types.UNSET self.finalized = False - def get_or_create( - self, - build: typing.Callable[[_T], _R], - target: _T, - create: typing.Callable[[_R], _V], - ) -> tuple[_V, bool]: - """Return the memoized singleton, or ``create(build(target))`` it once under this item's lock. + def get_or_create(self, make: typing.Callable[[_T], _V], target: _T) -> tuple[_V, bool]: + """Return the memoized singleton, or ``make(target)`` it once under this item's lock. A hit never takes the lock. A miss builds and creates under it, so concurrent misses build the value and its dependencies once. `created` is True only for the caller that built. @@ -40,42 +34,10 @@ def get_or_create( with self.lock: if self.cache is not types.UNSET: return self.cache, False - value = create(build(target)) + value = make(target) self.cache = value return value, True - def _pending_finalizer(self) -> typing.Callable[[typing.Any], object] | None: - """Return the finalizer still owed to the cached value, or None when nothing is owed.""" - return None if self.cache is types.UNSET or self.finalized else self.settings.finalizer - - def close_sync(self) -> None: - if (finalizer := self._pending_finalizer()) is not None: - if self.settings._is_async_finalizer: # noqa: SLF001 - raise exceptions.AsyncFinalizerInSyncCloseError(instance_type=type(self.cache)) - try: - result = finalizer(self.cache) - except Exception: - self.clear() - raise - if result is not None and inspect.isawaitable(result): - if inspect.iscoroutine(result): - result.close() # suppress "never awaited" warning - raise exceptions.AsyncFinalizerInSyncCloseError(instance_type=type(self.cache)) - self.finalized = True - - self.clear() - - -def fetch_cache_item(items: dict[int, CacheItem], provider: Factory[typing.Any]) -> CacheItem: - """Return the cache item in ``items`` for a cached ``provider``, creating it on first use.""" - # Get before setdefault: a bare setdefault builds a throwaway CacheItem on every hit. - provider_id = provider._provider_id # noqa: SLF001 - item = items.get(provider_id) - if item is not None: - return item - settings = typing.cast("CacheSettings[typing.Any]", provider._cache_settings) # noqa: SLF001 - return items.setdefault(provider_id, CacheItem(settings=settings)) - def cached_count(items: dict[int, CacheItem]) -> int: """Return how many of ``items`` hold a value.""" @@ -107,13 +69,26 @@ def close_sync(creation_order: list[CacheItem]) -> None: finalizer_errors: list[Exception] = [] remaining: list[CacheItem] = [] for cache_item in reversed(creation_order): - try: - cache_item.close_sync() - except exceptions.AsyncFinalizerInSyncCloseError as e: - finalizer_errors.append(e) - remaining.append(cache_item) - except Exception as e: # noqa: BLE001 - finalizer_errors.append(e) + finalizer = cache_item.settings.finalizer + value = cache_item.cache + if finalizer is not None and value is not types.UNSET and not cache_item.finalized: + if cache_item.settings._is_async_finalizer: # noqa: SLF001 + finalizer_errors.append(exceptions.AsyncFinalizerInSyncCloseError(instance_type=type(value))) + remaining.append(cache_item) + continue + try: + result = finalizer(value) + except Exception as e: # noqa: BLE001 + finalizer_errors.append(e) + else: + if result is not None and inspect.isawaitable(result): + if inspect.iscoroutine(result): + result.close() # suppress "never awaited" warning + finalizer_errors.append(exceptions.AsyncFinalizerInSyncCloseError(instance_type=type(value))) + remaining.append(cache_item) + continue + cache_item.finalized = True + cache_item.clear() remaining.reverse() creation_order[:] = remaining if finalizer_errors: diff --git a/modern_di/resolver_compiler.py b/modern_di/resolver_compiler.py index cf0f0d2e..59474435 100644 --- a/modern_di/resolver_compiler.py +++ b/modern_di/resolver_compiler.py @@ -21,7 +21,7 @@ import typing from modern_di import exceptions, types -from modern_di.cache import fetch_cache_item +from modern_di.cache import CacheItem from modern_di.dependency_graph import redirect_hops, terminal_chain from modern_di.providers.abstract import AbstractProvider from modern_di.providers.alias import Alias @@ -101,9 +101,8 @@ def compile_resolver(provider: "AbstractProvider[typing.Any]", registry: "Provid _TRANSIENT = "def resolve(container):\n" + _NAVIGATE + _BUILD_ARGUMENTS + _CALL_CREATOR _CACHED = ( - "def build(target):\n" + "def make(target):\n" + _BUILD_ARGUMENTS - + " return {built}\n\ndef create(built):\n" + _CALL_CREATOR + "\ndef resolve(container):\n" + _NAVIGATE @@ -111,11 +110,11 @@ def compile_resolver(provider: "AbstractProvider[typing.Any]", registry: "Provid cache_items = target._cache_items cache_item = cache_items.get(pid) if cache_item is None: - cache_item = fetch_cache_item(cache_items, provider) + cache_item = cache_items.setdefault(pid, CacheItem(settings=cache_settings)) cached = cache_item.cache if cached is not UNSET: return cached - value, created = cache_item.get_or_create(build, target, create) + value, created = cache_item.get_or_create(make, target) if created: target._creation_order.append(cache_item) return value @@ -129,23 +128,18 @@ def _source(arity: int, names: tuple[str, ...] | None, static: bool, cached: boo build = [f" a{i} = r{i}(target)" for i in range(arity)] or [" pass"] call_offset = 0 args = ", ".join(f"a{i}" for i in range(arity)) - built = "(" + "".join(f"a{i}, " for i in range(arity)) + ")" - star = "*" else: calls = [f" {name!r}: r{i}(target)," for i, name in enumerate(names)] build = [" kwargs = {", *calls, " }"] call_offset = 1 if static: build.append(" kwargs.update(static)") - args, built, star = "**kwargs", "kwargs", "**" - if cached: - template, args = _CACHED, f"{star}built" - else: - template = _TRANSIENT + args = "**kwargs" + template = _CACHED if cached else _TRANSIENT # `{build}` must be the first multi-line placeholder: the lines before it are counted as-is. build_line = template[: template.index("{build}")].count("\n") + 1 arg_lines = {build_line + call_offset + i: i for i in range(arity)} - return template.format(build="\n".join(build), built=built, args=args), arg_lines + return template.format(build="\n".join(build), args=args), arg_lines _shape_ids = itertools.count() @@ -178,7 +172,7 @@ def _navigate( _FACTORY_GLOBALS: dict[str, typing.Any] = { "UNSET": types.UNSET, - "fetch_cache_item": fetch_cache_item, + "CacheItem": CacheItem, "_navigate": _navigate, "ResolutionError": exceptions.ResolutionError, "CreatorCallError": exceptions.CreatorCallError, @@ -201,6 +195,7 @@ def _compile_factory(f: "Factory[typing.Any]", registry: "ProvidersRegistry") -> ) namespace = _FACTORY_GLOBALS.copy() namespace["provider"] = f + namespace["cache_settings"] = f.cache_settings namespace["pid"] = f.provider_id namespace["scope"] = f.scope namespace["creator"] = f._creator diff --git a/tests/helpers.py b/tests/helpers.py index 3de5a138..cbee8d31 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -1,10 +1,11 @@ import typing from modern_di import Container -from modern_di.cache import CacheItem, fetch_cache_item -from modern_di.providers import Factory +from modern_di.cache import CacheItem +from modern_di.providers import CacheSettings, Factory def cache_item(container: Container, provider: Factory[typing.Any]) -> CacheItem: """Return `container`'s cache item for the cached `provider`, creating it if needed.""" - return fetch_cache_item(container._cache_items, provider) + settings = typing.cast("CacheSettings[typing.Any]", provider.cache_settings) + return container._cache_items.setdefault(provider.provider_id, CacheItem(settings=settings)) diff --git a/tests/test_cache.py b/tests/test_cache.py index d7ede76c..28870bc7 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -4,9 +4,11 @@ import pytest -from modern_di.cache import CacheItem, close_async, fetch_cache_item +from modern_di import Container, Group, Scope +from modern_di.cache import CacheItem, close_async from modern_di.providers import CacheSettings, Factory from modern_di.types import UNSET +from tests.helpers import cache_item def _item() -> CacheItem: @@ -42,7 +44,7 @@ def create(kwargs: dict[str, typing.Any]) -> tuple[str, dict[str, typing.Any]]: calls["create"] += 1 return ("made", kwargs) - value, created = item.get_or_create(build=build, target=None, create=create) + value, created = item.get_or_create(lambda target: create(build(target)), None) assert created is True assert value == ("made", {"x": 1}) @@ -62,7 +64,7 @@ def create(_: object) -> str: msg = "create must not run on a cache hit" raise AssertionError(msg) - value, created = item.get_or_create(build=build, target=None, create=create) + value, created = item.get_or_create(lambda target: create(build(target)), None) assert created is False assert value == "cached" @@ -93,7 +95,7 @@ def create(_: object) -> str: msg = "create must not run when another thread already stored the value" raise AssertionError(msg) - value, created = item.get_or_create(build=build, target=None, create=create) + value, created = item.get_or_create(lambda target: create(build(target)), None) assert created is False assert value == "won-the-race" @@ -102,33 +104,42 @@ def create(_: object) -> str: def test_get_or_create_releases_the_item_lock() -> None: item = _item() - value, created = item.get_or_create(build=lambda _: 0, target=None, create=lambda _: "v") + value, created = item.get_or_create(lambda _: "v", None) assert (value, created) == ("v", True) assert _acquirable_from_another_thread(item) def test_each_cache_item_owns_its_lock() -> None: - cache_items: dict[int, CacheItem] = {} - first = fetch_cache_item(cache_items, Factory(creator=lambda: 1, bound_type=int, cache=True)) - second = fetch_cache_item(cache_items, Factory(creator=lambda: "", bound_type=str, cache=True)) + container = Container(scope=Scope.APP) + first = cache_item(container, Factory(creator=lambda: 1, bound_type=int, cache=True)) + second = cache_item(container, Factory(creator=lambda: "", bound_type=str, cache=True)) assert first.lock is not second.lock @pytest.mark.thread_race -def test_concurrent_fetches_of_one_provider_share_one_item() -> None: +def test_concurrent_first_resolves_of_one_provider_build_once() -> None: n = 8 - cache_items: dict[int, CacheItem] = {} - provider = Factory(creator=lambda: 1, bound_type=int, cache=True) + built: list[object] = [] + + def make() -> object: + built.append(value := object()) + return value + + class G(Group): + cached = Factory(creator=make, bound_type=object, cache=True) + + container = Container(scope=Scope.APP, groups=[G]) barrier = threading.Barrier(n, timeout=5) - def fetch() -> CacheItem: + def resolve() -> object: barrier.wait() - return fetch_cache_item(cache_items, provider) + return container.resolve(object) with ThreadPoolExecutor(max_workers=n) as pool: - items = [f.result(timeout=5) for f in [pool.submit(fetch) for _ in range(n)]] + values = [f.result(timeout=5) for f in [pool.submit(resolve) for _ in range(n)]] - assert all(item is items[0] for item in items) + assert len(built) == 1 + assert all(value is built[0] for value in values) async def test_close_async_runs_only_owed_finalizers_and_empties_the_order() -> None: