From 5be12c7f522c750766decc9ccae2b35099081afe Mon Sep 17 00:00:00 2001 From: Federico Busetti <729029+febus982@users.noreply.github.com> Date: Sun, 18 Jan 2026 11:58:19 +0000 Subject: [PATCH 1/2] Remove obsolete async helpers and update engine/session cleanup logic --- sqlalchemy_bind_manager/_async_helpers.py | 42 ----------- sqlalchemy_bind_manager/_bind_manager.py | 32 +++++++- sqlalchemy_bind_manager/_session_handler.py | 18 +++-- .../session_handler/test_session_lifecycle.py | 57 ++------------ tests/test_sqlalchemy_bind_manager.py | 75 +++---------------- 5 files changed, 57 insertions(+), 167 deletions(-) delete mode 100644 sqlalchemy_bind_manager/_async_helpers.py diff --git a/sqlalchemy_bind_manager/_async_helpers.py b/sqlalchemy_bind_manager/_async_helpers.py deleted file mode 100644 index 98704c9..0000000 --- a/sqlalchemy_bind_manager/_async_helpers.py +++ /dev/null @@ -1,42 +0,0 @@ -# Copyright (c) 2025 Federico Busetti <729029+febus982@users.noreply.github.com> -# -# Permission is hereby granted, free of charge, to any person obtaining a -# copy of this software and associated documentation files (the "Software"), -# to deal in the Software without restriction, including without limitation -# the rights to use, copy, modify, merge, publish, distribute, sublicense, -# and/or sell copies of the Software, and to permit persons to whom the -# Software is furnished to do so, subject to the following conditions: -# -# The above copyright notice and this permission notice shall be included in -# all copies or substantial portions of the Software. -# -# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL -# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING -# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER -# DEALINGS IN THE SOFTWARE. -import asyncio -from typing import Coroutine - -# Reference: https://docs.astral.sh/ruff/rules/asyncio-dangling-task/ -_background_asyncio_tasks = set() - - -def run_async_from_sync(coro: Coroutine) -> None: - try: - loop = asyncio.get_event_loop() - if loop.is_running(): - task = loop.create_task(coro) - # Add task to the set. This creates a strong reference. - _background_asyncio_tasks.add(task) - - # To prevent keeping references to finished tasks forever, - # make each task remove its own reference from the set after - # completion: - task.add_done_callback(_background_asyncio_tasks.discard) - else: - loop.run_until_complete(coro) - except RuntimeError: - asyncio.run(coro) diff --git a/sqlalchemy_bind_manager/_bind_manager.py b/sqlalchemy_bind_manager/_bind_manager.py index 16113a0..a509750 100644 --- a/sqlalchemy_bind_manager/_bind_manager.py +++ b/sqlalchemy_bind_manager/_bind_manager.py @@ -18,7 +18,9 @@ # FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER # DEALINGS IN THE SOFTWARE. -from typing import Mapping, MutableMapping, Union +import atexit +import weakref +from typing import ClassVar, Mapping, MutableMapping, Union from pydantic import BaseModel, ConfigDict, StrictBool from sqlalchemy import MetaData, create_engine @@ -32,7 +34,6 @@ from sqlalchemy.orm import Session, sessionmaker from sqlalchemy.orm.decl_api import DeclarativeMeta, registry -from sqlalchemy_bind_manager._async_helpers import run_async_from_sync from sqlalchemy_bind_manager.exceptions import ( InvalidConfigError, NotInitializedBindError, @@ -73,6 +74,7 @@ class SQLAlchemyAsyncBind(BaseModel): class SQLAlchemyBindManager: __binds: MutableMapping[str, Union[SQLAlchemyBind, SQLAlchemyAsyncBind]] + _instances: ClassVar[weakref.WeakSet["SQLAlchemyBindManager"]] = weakref.WeakSet() def __init__( self, @@ -87,14 +89,24 @@ def __init__( self.__init_bind(name, conf) else: self.__init_bind(DEFAULT_BIND_NAME, config) + SQLAlchemyBindManager._instances.add(self) - def __del__(self): + def _dispose_sync(self) -> None: + """Dispose all engines synchronously. + + This method is safe to call from any context, including __del__ + and atexit handlers. For async engines, it uses the underlying + sync_engine to perform synchronous disposal. + """ for bind in self.__binds.values(): if isinstance(bind, SQLAlchemyAsyncBind): - run_async_from_sync(bind.engine.dispose()) + bind.engine.sync_engine.dispose() else: bind.engine.dispose() + def __del__(self) -> None: + self._dispose_sync() + def __init_bind(self, name: str, config: SQLAlchemyConfig): if not isinstance(config, SQLAlchemyConfig): raise InvalidConfigError( @@ -210,3 +222,15 @@ def get_session( :return: The SQLAlchemy Session object """ return self.get_bind(bind_name).session_class() + + +@atexit.register +def _cleanup_all_managers() -> None: + """Cleanup handler that runs during interpreter shutdown. + + This ensures all SQLAlchemyBindManager instances have their engines + disposed before the interpreter exits, even if __del__ hasn't been + called yet due to reference cycles or other GC timing issues. + """ + for manager in list(SQLAlchemyBindManager._instances): + manager._dispose_sync() diff --git a/sqlalchemy_bind_manager/_session_handler.py b/sqlalchemy_bind_manager/_session_handler.py index 59dc82d..19c1c53 100644 --- a/sqlalchemy_bind_manager/_session_handler.py +++ b/sqlalchemy_bind_manager/_session_handler.py @@ -28,7 +28,6 @@ ) from sqlalchemy.orm import Session, scoped_session -from sqlalchemy_bind_manager._async_helpers import run_async_from_sync from sqlalchemy_bind_manager._bind_manager import ( SQLAlchemyAsyncBind, SQLAlchemyBind, @@ -69,12 +68,20 @@ def commit(self, session: Session) -> None: """ try: session.commit() - except: + except Exception: session.rollback() raise class AsyncSessionHandler: + """Async session handler for managing async scoped sessions. + + Note: Unlike SessionHandler, this class does not implement __del__ cleanup + because async_scoped_session.remove() is an async operation that cannot be + safely executed during garbage collection. Sessions should be properly + closed via the get_session() context manager. + """ + scoped_session: async_scoped_session def __init__(self, bind: SQLAlchemyAsyncBind): @@ -85,11 +92,6 @@ def __init__(self, bind: SQLAlchemyAsyncBind): bind.session_class, asyncio.current_task ) - def __del__(self): - if not getattr(self, "scoped_session", None): - return - run_async_from_sync(self.scoped_session.remove()) - @asynccontextmanager async def get_session(self, read_only: bool = False) -> AsyncIterator[AsyncSession]: session = self.scoped_session() @@ -110,6 +112,6 @@ async def commit(self, session: AsyncSession) -> None: """ try: await session.commit() - except: + except Exception: await session.rollback() raise diff --git a/tests/session_handler/test_session_lifecycle.py b/tests/session_handler/test_session_lifecycle.py index 7c3a79d..7ed373c 100644 --- a/tests/session_handler/test_session_lifecycle.py +++ b/tests/session_handler/test_session_lifecycle.py @@ -11,8 +11,14 @@ from sqlalchemy_bind_manager._session_handler import AsyncSessionHandler, SessionHandler -async def test_session_is_removed_on_cleanup(session_handler_class, sa_bind): - sh = session_handler_class(sa_bind) +def test_sync_session_is_removed_on_cleanup(sa_manager): + """Test that sync SessionHandler removes session on garbage collection. + + Note: AsyncSessionHandler does not implement __del__ cleanup because + async_scoped_session.remove() is an async operation that cannot be + safely executed during garbage collection. + """ + sh = SessionHandler(sa_manager.get_bind("sync")) original_session_remove = sh.scoped_session.remove with patch.object( @@ -26,53 +32,6 @@ async def test_session_is_removed_on_cleanup(session_handler_class, sa_bind): mocked_remove.assert_called_once() -def test_session_is_removed_on_cleanup_even_if_loop_is_not_running(sa_manager): - # Running the test without a loop will trigger the loop creation - sh = AsyncSessionHandler(sa_manager.get_bind("async")) - original_session_remove = sh.scoped_session.remove - original_get_event_loop = asyncio.get_event_loop - - with ( - patch.object( - sh.scoped_session, - "remove", - wraps=original_session_remove, - ) as mocked_close, - patch( - "asyncio.get_event_loop", - wraps=original_get_event_loop, - ) as mocked_get_event_loop, - ): - # This should trigger the garbage collector and close the session - sh = None - - mocked_get_event_loop.assert_called_once() - mocked_close.assert_called_once() - - -def test_session_is_removed_on_cleanup_even_if_loop_search_errors_out(sa_manager): - # Running the test without a loop will trigger the loop creation - sh = AsyncSessionHandler(sa_manager.get_bind("async")) - original_session_remove = sh.scoped_session.remove - - with ( - patch.object( - sh.scoped_session, - "remove", - wraps=original_session_remove, - ) as mocked_close, - patch( - "asyncio.get_event_loop", - side_effect=RuntimeError(), - ) as mocked_get_event_loop, - ): - # This should trigger the garbage collector and close the session - sh = None - - mocked_get_event_loop.assert_called_once() - mocked_close.assert_called_once() - - @pytest.mark.parametrize("read_only_flag", [True, False]) async def test_commit_is_called_only_if_not_read_only( read_only_flag, diff --git a/tests/test_sqlalchemy_bind_manager.py b/tests/test_sqlalchemy_bind_manager.py index a53e014..1800545 100644 --- a/tests/test_sqlalchemy_bind_manager.py +++ b/tests/test_sqlalchemy_bind_manager.py @@ -75,67 +75,19 @@ def test_multiple_binds(multiple_config): assert isinstance(sa_manager.get_session("async"), AsyncSession) -async def test_engine_is_disposed_on_cleanup(multiple_config): - sa_manager = SQLAlchemyBindManager(multiple_config) - sync_engine = sa_manager.get_bind("default").engine - async_engine = sa_manager.get_bind("async").engine - - original_sync_dispose = sync_engine.dispose - original_async_dispose = async_engine.dispose - - with ( - patch.object( - sync_engine, - "dispose", - wraps=original_sync_dispose, - ) as mocked_dispose, - patch.object( - type(async_engine), - "dispose", - wraps=original_async_dispose, - ) as mocked_async_dispose, - ): - sa_manager = None - - mocked_dispose.assert_called_once() - mocked_async_dispose.assert_called() - - -def test_engine_is_disposed_on_cleanup_even_if_no_loop(multiple_config): - sa_manager = SQLAlchemyBindManager(multiple_config) - sync_engine = sa_manager.get_bind("default").engine - async_engine = sa_manager.get_bind("async").engine - - original_sync_dispose = sync_engine.dispose - original_async_dispose = async_engine.dispose - - with ( - patch.object( - sync_engine, - "dispose", - wraps=original_sync_dispose, - ) as mocked_dispose, - patch.object( - type(async_engine), - "dispose", - wraps=original_async_dispose, - ) as mocked_async_dispose, - ): - sa_manager = None - - mocked_dispose.assert_called_once() - mocked_async_dispose.assert_called() - +def test_engine_is_disposed_on_cleanup(multiple_config): + """Test that engines are disposed synchronously during garbage collection. -def test_engine_is_disposed_on_cleanup_even_if_loop_search_errors_out( - multiple_config, -): + This test verifies that both sync and async engines are properly disposed + using synchronous disposal (sync_engine.dispose() for async engines). + """ sa_manager = SQLAlchemyBindManager(multiple_config) sync_engine = sa_manager.get_bind("default").engine async_engine = sa_manager.get_bind("async").engine original_sync_dispose = sync_engine.dispose - original_async_dispose = async_engine.dispose + # For async engines, we now use sync_engine.dispose() for safe cleanup + original_async_sync_dispose = async_engine.sync_engine.dispose with ( patch.object( @@ -144,17 +96,12 @@ def test_engine_is_disposed_on_cleanup_even_if_loop_search_errors_out( wraps=original_sync_dispose, ) as mocked_dispose, patch.object( - type(async_engine), + async_engine.sync_engine, "dispose", - wraps=original_async_dispose, - ) as mocked_async_dispose, - patch( - "asyncio.get_event_loop", - side_effect=RuntimeError(), - ) as mocked_get_event_loop, + wraps=original_async_sync_dispose, + ) as mocked_async_sync_dispose, ): sa_manager = None - mocked_get_event_loop.assert_called_once() mocked_dispose.assert_called_once() - mocked_async_dispose.assert_called() + mocked_async_sync_dispose.assert_called_once() From 28cc3d83904bca00b012d0e86e1f133ee8b969e6 Mon Sep 17 00:00:00 2001 From: Federico Busetti <729029+febus982@users.noreply.github.com> Date: Sun, 18 Jan 2026 12:04:15 +0000 Subject: [PATCH 2/2] Add test for atexit cleanup handler Ensures _cleanup_all_managers() is covered by tests, restoring 100% test coverage. Co-Authored-By: Claude Opus 4.5 --- tests/test_sqlalchemy_bind_manager.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/test_sqlalchemy_bind_manager.py b/tests/test_sqlalchemy_bind_manager.py index 1800545..80eb64f 100644 --- a/tests/test_sqlalchemy_bind_manager.py +++ b/tests/test_sqlalchemy_bind_manager.py @@ -105,3 +105,18 @@ def test_engine_is_disposed_on_cleanup(multiple_config): mocked_dispose.assert_called_once() mocked_async_sync_dispose.assert_called_once() + + +def test_atexit_cleanup_disposes_all_managers(multiple_config): + """Test that the atexit handler disposes all tracked manager instances.""" + from sqlalchemy_bind_manager._bind_manager import _cleanup_all_managers + + sa_manager = SQLAlchemyBindManager(multiple_config) + + with patch.object( + sa_manager, + "_dispose_sync", + ) as mocked_dispose_sync: + _cleanup_all_managers() + + mocked_dispose_sync.assert_called_once()