Merge pull request #12390 from mvdbeek/fix_detached_instance_error

Keep separate session for ToolShedRepositoryCache

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()
 

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

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()

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

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