From d337565fee1434c3f2eb01917c0adcbac77c01d8 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sun, 18 Oct 2026 13:17:27 +0300 Subject: [PATCH] fix: finalize a build that finishes during a close in that close --- docs/providers/lifecycle.md | 8 +++--- modern_di/cache.py | 21 +++++++++------ modern_di/container.py | 7 ++--- tests/providers/test_cached_factory.py | 37 ++++++++++++++++++++++++++ tests/test_cache.py | 18 ++++++++----- 5 files changed, 69 insertions(+), 22 deletions(-) diff --git a/docs/providers/lifecycle.md b/docs/providers/lifecycle.md index 102e5538..c3cc37c6 100644 --- a/docs/providers/lifecycle.md +++ b/docs/providers/lifecycle.md @@ -246,10 +246,10 @@ The container counts as closed as soon as `close_sync()` or `close_async()` star that resolves from its own container gets `ContainerClosedError`. Give a finalizer what it needs through the cached instance. -Closing does not wait for resolves running on other threads. If another thread is still building a -cached instance when the close starts, that close does not finalize it: the instance stays cached in -the closed container, and its finalizer runs at the next close. Stop resolving from a container -before you close it. +Closing does not wait for resolves running on other threads. If another thread finishes building a +cached instance while a close runs, that close finalizes it. If it finishes after the close returns, +the instance stays cached in the closed container, and its finalizer runs at the next close. Stop +resolving from a container before you close it. After close, resolving raises `ContainerClosedError` and the creator does not run. That includes a resolve through a child that reaches the closed container. A child of a closed container can still diff --git a/modern_di/cache.py b/modern_di/cache.py index 5a3c5b31..f932a950 100644 --- a/modern_di/cache.py +++ b/modern_di/cache.py @@ -47,10 +47,10 @@ def cached_count(items: dict[int, CacheItem]) -> int: async def close_async(creation_order: list[CacheItem]) -> None: - """Close the items queued when this starts, newest first, and dequeue them; failures raise together.""" + """Pop and close items newest first until ``creation_order`` is empty; failures raise together at the end.""" finalizer_errors: list[Exception] = [] - closing = creation_order[:] - for cache_item in reversed(closing): + while creation_order: + cache_item = creation_order.pop() finalizer = cache_item.settings.finalizer if finalizer is not None and cache_item.cache is not types.UNSET and not cache_item.finalized: try: @@ -60,20 +60,22 @@ async def close_async(creation_order: list[CacheItem]) -> None: except Exception as e: # noqa: BLE001 e.add_note(finalizer_note(type(cache_item.cache))) finalizer_errors.append(e) + except BaseException: + creation_order.append(cache_item) + raise else: cache_item.finalized = True cache_item.clear() - del creation_order[: len(closing)] if finalizer_errors: raise exceptions.FinalizerError(exceptions=finalizer_errors, is_async=True) def close_sync(creation_order: list[CacheItem]) -> None: - """Close the items queued when this starts, newest first; an async finalizer stays queued for an async close.""" + """Pop and close items newest first until ``creation_order`` is empty; an async finalizer is queued again.""" finalizer_errors: list[Exception] = [] remaining: list[CacheItem] = [] - closing = creation_order[:] - for cache_item in reversed(closing): + while creation_order: + cache_item = creation_order.pop() 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: @@ -86,6 +88,9 @@ def close_sync(creation_order: list[CacheItem]) -> None: except Exception as e: # noqa: BLE001 e.add_note(finalizer_note(type(value))) finalizer_errors.append(e) + except BaseException: + creation_order.extend([cache_item, *reversed(remaining)]) + raise else: if result is not None and inspect.isawaitable(result): if inspect.iscoroutine(result): @@ -96,6 +101,6 @@ def close_sync(creation_order: list[CacheItem]) -> None: cache_item.finalized = True cache_item.clear() remaining.reverse() - creation_order[: len(closing)] = remaining + creation_order[:0] = remaining if finalizer_errors: raise exceptions.FinalizerError(exceptions=finalizer_errors, is_async=False) diff --git a/modern_di/container.py b/modern_di/container.py index dfcd26e5..0ad2cee4 100644 --- a/modern_di/container.py +++ b/modern_di/container.py @@ -262,8 +262,8 @@ async def close_async(self) -> None: Overrides are kept. A resolve from inside a finalizer raises :class:`~modern_di.exceptions.ContainerClosedError`. Every finalizer runs even when one raises; the failures come back together as one :class:`~modern_di.exceptions.FinalizerError`. - Not coordinated with resolves on other threads: an instance still being built is finalized by - the next close. + Not coordinated with resolves on other threads: an instance that finishes building while this + runs is finalized by it, and one that finishes later by the next close. """ self._closed = True if self._creation_order: @@ -276,7 +276,8 @@ def close_sync(self) -> None: :class:`~modern_di.exceptions.ContainerClosedError`. Every finalizer runs even when one raises; the failures come back together as one :class:`~modern_di.exceptions.FinalizerError`. An async finalizer fails here and stays pending for a later :meth:`close_async`. Not coordinated - with resolves on other threads: an instance still being built is finalized by the next close. + with resolves on other threads: an instance that finishes building while this runs is finalized + by it, and one that finishes later by the next close. """ self._closed = True if self._creation_order: diff --git a/tests/providers/test_cached_factory.py b/tests/providers/test_cached_factory.py index d99def6e..cccc54be 100644 --- a/tests/providers/test_cached_factory.py +++ b/tests/providers/test_cached_factory.py @@ -681,6 +681,9 @@ class _First: ... class _Second: ... +class _Third: ... + + class _Built: count = 0 @@ -823,6 +826,40 @@ class SlowGroup(Group): assert container.resolve(_Second) is not second +async def test_interrupted_close_sync_keeps_unfinalized_items_queued() -> None: + events: list[str] = [] + + def sync_finalizer(_: _First) -> None: + events.append("first") + + def interrupting_finalizer(_: _Second) -> None: + events.append("second") + if events.count("second") == 1: + raise KeyboardInterrupt + + async def async_finalizer(_: _Third) -> None: + events.append("third") + + class InterruptGroup(Group): + first = providers.Factory(creator=_First, cache=providers.CacheSettings(finalizer=sync_finalizer)) + second = providers.Factory(creator=_Second, cache=providers.CacheSettings(finalizer=interrupting_finalizer)) + third = providers.Factory(creator=_Third, cache=providers.CacheSettings(finalizer=async_finalizer)) + + container = Container(groups=[InterruptGroup]) + container.resolve(_First) + container.resolve(_Second) + container.resolve(_Third) + + with pytest.raises(KeyboardInterrupt): + container.close_sync() + + assert events == ["second"] + + await container.close_async() + + assert events == ["second", "third", "second", "first"] + + async def test_close_async_after_a_cancelled_close_does_not_refinalize_closed_items() -> None: events: list[str] = [] diff --git a/tests/test_cache.py b/tests/test_cache.py index 4255292a..21ba53e8 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -203,12 +203,12 @@ class G(Group): return container, log -def test_a_build_finishing_during_close_sync_is_finalized_by_the_next_close() -> None: +def test_a_build_finishing_during_close_sync_is_finalized_by_that_close() -> None: log: list[str] = [] container, closed = _close_with_a_build_finishing_mid_close( Container.close_sync, x_finalizer=lambda _: log.append("X") ) - assert (closed, log) == (["Y"], []) + assert (closed, log) == (["Y"], ["X"]) container.close_sync() @@ -219,10 +219,10 @@ def _close_async(container: Container) -> None: asyncio.run(container.close_async()) -def test_a_build_finishing_during_close_async_is_finalized_by_the_next_close() -> None: +def test_a_build_finishing_during_close_async_is_finalized_by_that_close() -> None: log: list[str] = [] container, closed = _close_with_a_build_finishing_mid_close(_close_async, x_finalizer=lambda _: log.append("X")) - assert (closed, log) == (["Y"], []) + assert (closed, log) == (["Y"], ["X"]) _close_async(container) @@ -235,9 +235,13 @@ def test_a_build_finishing_during_close_sync_keeps_an_async_finalizer_pending() async def finalize_x(_: object) -> None: log.append("X") - container, _ = _close_with_a_build_finishing_mid_close(Container.close_sync, x_finalizer=finalize_x) - with pytest.raises(FinalizerError): - container.close_sync() + def close_sync_reporting_the_async_finalizer(container: Container) -> None: + with pytest.raises(FinalizerError): + container.close_sync() + + container, _ = _close_with_a_build_finishing_mid_close( + close_sync_reporting_the_async_finalizer, x_finalizer=finalize_x + ) assert log == [] _close_async(container)