fix: finalize a cached instance built during a close at the next close (#726)

Author: lesnik512

modern_di/cache.py

@@ -47,9 +47,10 @@ def cached_count(items: dict[int, CacheItem]) -> int:
 
 
 async def close_async(creation_order: list[CacheItem]) -> None:
-    """Close every item newest first and empty ``creation_order``; failures raise together at the end."""
+    """Close the items queued when this starts, newest first, and dequeue them; failures raise together."""
     finalizer_errors: list[Exception] = []
-    for cache_item in reversed(creation_order):
+    closing = creation_order[:]
+    for cache_item in reversed(closing):
         finalizer = cache_item.settings.finalizer
         if finalizer is not None and cache_item.cache is not types.UNSET and not cache_item.finalized:
             try:
@@ -62,16 +63,17 @@ async def close_async(creation_order: list[CacheItem]) -> None:
             else:
                 cache_item.finalized = True
         cache_item.clear()
-    creation_order.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 every item newest first; an async finalizer stays in ``creation_order`` for a later async close."""
+    """Close the items queued when this starts, newest first; an async finalizer stays queued for an async close."""
     finalizer_errors: list[Exception] = []
     remaining: list[CacheItem] = []
-    for cache_item in reversed(creation_order):
+    closing = creation_order[:]
+    for cache_item in reversed(closing):
         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:
@@ -94,6 +96,6 @@ def close_sync(creation_order: list[CacheItem]) -> None:
                 cache_item.finalized = True
         cache_item.clear()
     remaining.reverse()
-    creation_order[:] = remaining
+    creation_order[: len(closing)] = remaining
     if finalizer_errors:
         raise exceptions.FinalizerError(exceptions=finalizer_errors, is_async=False)

tests/test_cache.py

@@ -1,3 +1,4 @@
+import asyncio
 import threading
 import typing
 from concurrent.futures import ThreadPoolExecutor
@@ -6,6 +7,7 @@
 
 from modern_di import Container, Group, Scope
 from modern_di.cache import CacheItem, close_async
+from modern_di.exceptions import FinalizerError
 from modern_di.providers import CacheSettings, Factory
 from modern_di.types import UNSET
 from tests.helpers import cache_item
@@ -160,3 +162,84 @@ async def test_close_async_runs_only_owed_finalizers_and_empties_the_order() ->
     assert persistent.cache == "persistent"
     assert with_finalizer.cache is UNSET
     assert creation_order == []
+
+
+def _close_with_a_build_finishing_mid_close(
+    close: typing.Callable[[Container], typing.Any], *, x_finalizer: typing.Callable[[object], typing.Any]
+) -> tuple[Container, list[str]]:
+    """Close a container while another thread's cached build finishes during that close.
+
+    ``Y`` is cached before the close starts. ``X``'s creator blocks until ``Y``'s finalizer
+    releases it, and that finalizer waits for the build to finish, so ``X`` is cached and queued
+    while the close is still running.
+    """
+    log: list[str] = []
+    started = threading.Event()
+    go = threading.Event()
+
+    class X:
+        def __init__(self) -> None:
+            started.set()
+            go.wait(timeout=5)
+
+    class Y: ...
+
+    def finalize_y(_: Y) -> None:
+        log.append("Y")
+        go.set()
+        builder.join(timeout=5)
+
+    class G(Group):
+        x = Factory(creator=X, cache=CacheSettings(finalizer=x_finalizer))
+        y = Factory(creator=Y, cache=CacheSettings(finalizer=finalize_y))
+
+    container = Container(groups=[G])
+    container.resolve(Y)
+    builder = threading.Thread(target=container.resolve, args=(X,))
+    builder.start()
+    assert started.wait(timeout=5)
+    close(container)
+    assert not builder.is_alive()
+    return container, log
+
+
+def test_a_build_finishing_during_close_sync_is_finalized_by_the_next_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"], [])
+
+    container.close_sync()
+
+    assert log == ["X"]
+
+
+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:
+    log: list[str] = []
+    container, closed = _close_with_a_build_finishing_mid_close(_close_async, x_finalizer=lambda _: log.append("X"))
+    assert (closed, log) == (["Y"], [])
+
+    _close_async(container)
+
+    assert log == ["X"]
+
+
+def test_a_build_finishing_during_close_sync_keeps_an_async_finalizer_pending() -> None:
+    log: list[str] = []
+
+    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()
+    assert log == []
+
+    _close_async(container)
+
+    assert log == ["X"]