From c84441099068136da75cb598f3cf3a5918be8d8b Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 30 Aug 2021 17:05:08 +0200 Subject: [PATCH] Keep separate session for ToolShedRepositoryCache Should fix ``` Traceback (most recent call last): File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/jobs/runners/__init__.py", line 244, in prepare_job stderr_file=stderr_file, File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/jobs/runners/__init__.py", line 285, in build_command_line stderr_file=stderr_file, File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/jobs/command_factory.py", line 79, in build_command __handle_dependency_resolution(commands_builder, job_wrapper, remote_command_params) File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/jobs/command_factory.py", line 194, in __handle_dependency_resolution if local_dependency_resolution and job_wrapper.dependency_shell_commands: File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/jobs/__init__.py", line 1012, in dependency_shell_commands job_directory=self.working_directory File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/tools/__init__.py", line 1891, in build_dependency_shell_commands tool_instance=self File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/tool_util/deps/__init__.py", line 184, in dependency_shell_commands requirements_to_dependencies = self.requirements_to_dependencies(requirements, **kwds) File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/tool_util/deps/__init__.py", line 194, in requirements_to_dependencies requirement_to_dependency = self._requirements_to_dependencies_dict(requirements, **kwds) File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/tool_util/deps/__init__.py", line 283, in _requirements_to_dependencies_dict dependency = resolver.resolve(requirement, **kwds) File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/tool_util/deps/resolvers/galaxy_packages.py", line 79, in resolve return self._find_dep_versioned(name, version, type=type, **kwds) File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/tool_util/deps/resolvers/tool_shed_packages.py", line 25, in _find_dep_versioned path = self._get_package_installed_dependency_path(installed_tool_dependency, name, version) File "/cvmfs/test.galaxyproject.org/galaxy/lib/galaxy/tool_util/deps/resolvers/tool_shed_packages.py", line 43, in _get_package_installed_dependency_path tool_shed_repository = installed_tool_dependency.tool_shed_repository File "/cvmfs/test.galaxyproject.org/venv/lib/python3.6/site-packages/sqlalchemy/orm/attributes.py", line 480, in __get__ return self.impl.get(state, dict_) File "/cvmfs/test.galaxyproject.org/venv/lib/python3.6/site-packages/sqlalchemy/orm/attributes.py", line 931, in get value = self.callable_(state, passive) File "/cvmfs/test.galaxyproject.org/venv/lib/python3.6/site-packages/sqlalchemy/orm/strategies.py", line 836, in _load_for_state % (orm_util.state_str(state), self.key) sqlalchemy.orm.exc.DetachedInstanceError: Parent instance is not bound to a Session; lazy load operation of attribute 'tool_shed_repository' cannot proceed (Background on this error at: http://sqlalche.me/e/14/bhk3) ``` Which I think broke in https://github.com/galaxyproject/galaxy/pull/11737#discussion_r636354063. Instead of hoping that we preload all attributes it seems more robust to use a separate session for the cache that is not tied to a thread and that will not be closed by flushes in other parts of the application. --- lib/galaxy/app.py | 4 +++ lib/galaxy/model/orm/engine_factory.py | 7 ++++- lib/galaxy/tools/cache.py | 27 +++++++++---------- test/unit/tools/conftest.py | 12 ++++----- .../tools/test_tool_shed_repository_cache.py | 22 ++------------- 5 files changed, 31 insertions(+), 41 deletions(-) diff --git a/lib/galaxy/app.py b/lib/galaxy/app.py index bb6e982f65b..58b1eea05ea 100644 --- a/lib/galaxy/app.py +++ b/lib/galaxy/app.py @@ -216,6 +216,7 @@ class UniverseApplication(StructuredApp, GalaxyManagerApplication): ("job manager", self._shutdown_job_manager), ("application heartbeat", self._shutdown_heartbeat), ("repository manager", self._shutdown_repo_manager), + ("database connection repository cache", self._shutdown_repo_cache), ("database connection", self._shutdown_model), ("application stack", self._shutdown_application_stack), ] @@ -391,6 +392,9 @@ class UniverseApplication(StructuredApp, GalaxyManagerApplication): def _shutdown_repo_manager(self): self.update_repository_manager.shutdown() + def _shutdown_repo_cache(self): + self.tool_shed_repository_cache.shutdown() + def _shutdown_application_stack(self): self.application_stack.shutdown() diff --git a/lib/galaxy/model/orm/engine_factory.py b/lib/galaxy/model/orm/engine_factory.py index eb1f05d2630..e84c796ba9d 100644 --- a/lib/galaxy/model/orm/engine_factory.py +++ b/lib/galaxy/model/orm/engine_factory.py @@ -86,7 +86,12 @@ def build_engine(url, engine_options, database_query_profiling_proxy=False, trac except AttributeError: pass + # Set check_same_thread to False for sqlite, handled by request-specific session + # See https://fastapi.tiangolo.com/tutorial/sql-databases/#note + connect_args = {} + if 'sqlite://' in url: + connect_args['check_same_thread'] = False # Create the database engine - engine = create_engine(url, **engine_options) + engine = create_engine(url, connect_args=connect_args, **engine_options) register_after_fork(engine, lambda e: e.dispose()) return engine diff --git a/lib/galaxy/tools/cache.py b/lib/galaxy/tools/cache.py index 69617e5eda5..25a551d7f4b 100644 --- a/lib/galaxy/tools/cache.py +++ b/lib/galaxy/tools/cache.py @@ -13,10 +13,10 @@ from sqlalchemy.orm import ( defer, joinedload, ) +from sqlalchemy.orm.session import sessionmaker from sqlitedict import SqliteDict from galaxy.model.tool_shed_install import ToolShedRepository -from galaxy.structured_app import MinimalManagerApp from galaxy.tool_util.toolbox.base import ToolConfRepository from galaxy.util import unicodify from galaxy.util.hash_util import md5_hash_file @@ -286,8 +286,8 @@ class ToolShedRepositoryCache: repositories: List[ToolShedRepository] repos_by_tuple: Dict[Tuple[str, str, str], List[ToolConfRepository]] - def __init__(self, app: MinimalManagerApp): - self.app = app + def __init__(self, session: sessionmaker): + self.session = session() # Contains ToolConfRepository objects created from shed_tool_conf.xml entries self.local_repositories = [] # Repositories loaded from database @@ -300,17 +300,13 @@ class ToolShedRepositoryCache: self.repos_by_tuple[(repository.tool_shed, repository.owner, repository.name)].append(repository) def rebuild(self): - try: - session = self.app.install_model._SessionLocal() - self.repositories = session.query(ToolShedRepository).options( - defer(ToolShedRepository.metadata), joinedload('tool_dependencies') - ).all() - repos_by_tuple = defaultdict(list) - for repository in self.repositories + self.local_repositories: - repos_by_tuple[(repository.tool_shed, repository.owner, repository.name)].append(repository) - self.repos_by_tuple = repos_by_tuple - finally: - session.close() + self.repositories = self.session.query(ToolShedRepository).options( + defer(ToolShedRepository.metadata), joinedload('tool_dependencies') + ).all() + repos_by_tuple = defaultdict(list) + for repository in self.repositories + self.local_repositories: + repos_by_tuple[(repository.tool_shed, repository.owner, repository.name)].append(repository) + self.repos_by_tuple = repos_by_tuple def get_installed_repository(self, tool_shed=None, name=None, owner=None, installed_changeset_revision=None, changeset_revision=None, repository_id=None): if repository_id: @@ -327,3 +323,6 @@ class ToolShedRepositoryCache: continue return repo return None + + def shutdown(self) -> None: + self.session.close() diff --git a/test/unit/tools/conftest.py b/test/unit/tools/conftest.py index 4e6ff369dfb..d150a0ac9a1 100644 --- a/test/unit/tools/conftest.py +++ b/test/unit/tools/conftest.py @@ -18,13 +18,13 @@ def mock_app(): @pytest.fixture def tool_shed_repository_cache(mock_app): - tool_shed_repository_cache = ToolShedRepositoryCache(app=mock_app) + tool_shed_repository_cache = ToolShedRepositoryCache(session=mock_app.install_model.context) return tool_shed_repository_cache @pytest.fixture def repos(mock_app): - repositories = [create_repo(mock_app, changeset=i + 1, installed_changeset=i) for i in range(10)] + repositories = [create_repo(mock_app.install_model.context, changeset=i + 1, installed_changeset=i) for i in range(10)] mock_app.install_model.context.flush() return repositories @@ -46,7 +46,7 @@ def tool_conf_repos(tool_shed_repository_cache): return tool_shed_repository_cache.local_repositories -def create_repo(app, changeset, installed_changeset, config_filename=None): +def create_repo(session, changeset, installed_changeset, config_filename=None): metadata = { 'tools': [{ 'add_to_tool_panel': False, # to have repository.includes_tools_for_display_in_tool_panel=False in InstalledRepositoryManager.activate_repository() @@ -64,8 +64,8 @@ def create_repo(app, changeset, installed_changeset, config_filename=None): repository.installed_changeset_revision = str(installed_changeset) repository.deleted = False repository.uninstalled = False - app.install_model.context.add(repository) - app.install_model.context.flush() + session.add(repository) + session.flush() tool_dependency = tool_shed_install.ToolDependency( name='Name', version='100', @@ -73,5 +73,5 @@ def create_repo(app, changeset, installed_changeset, config_filename=None): status='ok', tool_shed_repository_id=repository.id, ) - app.install_model.context.add(tool_dependency) + session.add(tool_dependency) return repository diff --git a/test/unit/tools/test_tool_shed_repository_cache.py b/test/unit/tools/test_tool_shed_repository_cache.py index ae99298292e..26966703b07 100644 --- a/test/unit/tools/test_tool_shed_repository_cache.py +++ b/test/unit/tools/test_tool_shed_repository_cache.py @@ -1,5 +1,4 @@ import pytest -from sqlalchemy.orm.exc import DetachedInstanceError from .conftest import create_repo @@ -22,8 +21,8 @@ def test_add_repository_and_tool_conf_repository_to_repository_cache(tool_shed_r tool_shed_repository_cache.rebuild() assert len(tool_shed_repository_cache.repositories) == 10 assert len(tool_shed_repository_cache.local_repositories) == 10 - create_repo(tool_shed_repository_cache.app, '21', '20') - tool_shed_repository_cache.app.install_model.context.flush() + create_repo(tool_shed_repository_cache.session, '21', '20') + tool_shed_repository_cache.session.flush() tool_shed_repository_cache.rebuild() assert len(tool_shed_repository_cache.repositories) == 11 assert len(tool_shed_repository_cache.local_repositories) == 10 @@ -55,20 +54,3 @@ def test_get_installed_repository(tool_shed_repository_cache, repos, tool_conf_r assert repo else: assert repo is None - - -def test_repo_cache_expunge(tool_shed_repository_cache, repos): - tool_shed_repository_cache.rebuild() - assert len(tool_shed_repository_cache.repositories) == 10 - # Modify and commit a repo, will expire in memory attributes of orm objects (unless using a different session) - repo = tool_shed_repository_cache.repositories[0] - repo.name = 'new name' - tool_shed_repository_cache.app.install_model.session.flush() - # remove session, should demonstrate the separate session is in use - tool_shed_repository_cache.app.install_model.session.remove() - assert repo.changeset_revision == "1" - assert repo.tool_dependencies[0].name == 'Name' - with pytest.raises(DetachedInstanceError): - # Make sure this still raises DetachedInstanceError, - # keeping this in memory would be expensive - repo.metadata