From 1f0ec9424fd116a1f6896bf648d16a309f1e6847 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 2 Mar 2019 11:45:07 +0100 Subject: [PATCH 1/7] Use a lock on cleanup, expire_tool and cache_tool This should fix https://github.com/galaxyproject/galaxy/issues/7444. --- lib/galaxy/tools/cache.py | 79 ++++++++++++++++++--------------- test/unit/tools/test_toolbox.py | 2 +- 2 files changed, 44 insertions(+), 37 deletions(-) diff --git a/lib/galaxy/tools/cache.py b/lib/galaxy/tools/cache.py index 053aee8e862..a2d35036ae5 100644 --- a/lib/galaxy/tools/cache.py +++ b/lib/galaxy/tools/cache.py @@ -1,5 +1,8 @@ import os -from threading import local +from threading import ( + local, + Lock, +) from sqlalchemy.orm.exc import DetachedInstanceError @@ -13,6 +16,7 @@ class ToolCache(object): """ def __init__(self): + self._lock = Lock() self._hash_by_tool_paths = {} self._tools_by_path = {} self._tool_paths_by_id = {} @@ -30,22 +34,23 @@ class ToolCache(object): """ removed_tool_ids = [] try: - paths_to_cleanup = {path: tool.all_ids for path, tool in self._tools_by_path.items() if self._should_cleanup(path)} - for config_filename, tool_ids in paths_to_cleanup.items(): - del self._hash_by_tool_paths[config_filename] - if os.path.exists(config_filename): - # This tool has probably been broken while editing on disk - # We record it here, so that we can recover it - self._removed_tools_by_path[config_filename] = self._tools_by_path[config_filename] - del self._tools_by_path[config_filename] - for tool_id in tool_ids: - if tool_id in self._tool_paths_by_id: - del self._tool_paths_by_id[tool_id] - removed_tool_ids.extend(tool_ids) - for tool_id in removed_tool_ids: - self._removed_tool_ids.add(tool_id) - if tool_id in self._new_tool_ids: - self._new_tool_ids.remove(tool_id) + with self._lock: + paths_to_cleanup = {path: tool.all_ids for path, tool in self._tools_by_path.items() if self._should_cleanup(path)} + for config_filename, tool_ids in paths_to_cleanup.items(): + del self._hash_by_tool_paths[config_filename] + if os.path.exists(config_filename): + # This tool has probably been broken while editing on disk + # We record it here, so that we can recover it + self._removed_tools_by_path[config_filename] = self._tools_by_path[config_filename] + del self._tools_by_path[config_filename] + for tool_id in tool_ids: + if tool_id in self._tool_paths_by_id: + del self._tool_paths_by_id[tool_id] + removed_tool_ids.extend(tool_ids) + for tool_id in removed_tool_ids: + self._removed_tool_ids.add(tool_id) + if tool_id in self._new_tool_ids: + self._new_tool_ids.remove(tool_id) except Exception: # If by chance the file is being removed while calculating the hash or modtime # we don't want the thread to die. @@ -79,31 +84,33 @@ class ToolCache(object): return self.get_tool(self._tool_paths_by_id.get(tool_id)) def expire_tool(self, tool_id): - if tool_id in self._tool_paths_by_id: - config_filename = self._tool_paths_by_id[tool_id] - del self._hash_by_tool_paths[config_filename] - del self._tool_paths_by_id[tool_id] - del self._tools_by_path[config_filename] - del self._mod_time_by_path[config_filename] - if tool_id in self._new_tool_ids: - self._new_tool_ids.remove(tool_id) + with self._lock: + if tool_id in self._tool_paths_by_id: + config_filename = self._tool_paths_by_id[tool_id] + del self._hash_by_tool_paths[config_filename] + del self._tool_paths_by_id[tool_id] + del self._tools_by_path[config_filename] + del self._mod_time_by_path[config_filename] + if tool_id in self._new_tool_ids: + self._new_tool_ids.remove(tool_id) def cache_tool(self, config_filename, tool): tool_hash = md5_hash_file(config_filename) if tool_hash is None: return tool_id = str(tool.id) - self._hash_by_tool_paths[config_filename] = tool_hash - self._mod_time_by_path[config_filename] = os.path.getmtime(config_filename) - self._tool_paths_by_id[tool_id] = config_filename - self._tools_by_path[config_filename] = tool - self._new_tool_ids.add(tool_id) - for macro_path in tool._macro_paths: - self._mod_time_by_path[macro_path] = os.path.getmtime(macro_path) - if tool_id not in self._macro_paths_by_id: - self._macro_paths_by_id[tool_id] = {macro_path} - else: - self._macro_paths_by_id[tool_id].add(macro_path) + with self._lock: + self._hash_by_tool_paths[config_filename] = tool_hash + self._mod_time_by_path[config_filename] = os.path.getmtime(config_filename) + self._tool_paths_by_id[tool_id] = config_filename + self._tools_by_path[config_filename] = tool + self._new_tool_ids.add(tool_id) + for macro_path in tool._macro_paths: + self._mod_time_by_path[macro_path] = os.path.getmtime(macro_path) + if tool_id not in self._macro_paths_by_id: + self._macro_paths_by_id[tool_id] = {macro_path} + else: + self._macro_paths_by_id[tool_id].add(macro_path) def reset_status(self): """ diff --git a/test/unit/tools/test_toolbox.py b/test/unit/tools/test_toolbox.py index 5e10976ceab..dfdb0204c12 100644 --- a/test/unit/tools/test_toolbox.py +++ b/test/unit/tools/test_toolbox.py @@ -236,7 +236,7 @@ class ToolBoxTestCase(BaseToolBoxTestCase): def _try_until_no_errors(self, f): e = None - for i in range(300): + for i in range(10): try: f() return From 2d7b3e32d300ba33b0dadd3b8ff1cbec6b98a9bb Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Tue, 8 Jan 2019 17:28:03 +0000 Subject: [PATCH 2/7] Add debugging statements to ``ToolCache.cleanup()`` --- lib/galaxy/tools/cache.py | 8 +++++++- test/unit/tools/test_toolbox.py | 1 - 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tools/cache.py b/lib/galaxy/tools/cache.py index a2d35036ae5..dd662f528dc 100644 --- a/lib/galaxy/tools/cache.py +++ b/lib/galaxy/tools/cache.py @@ -1,3 +1,4 @@ +import logging import os from threading import ( local, @@ -8,6 +9,8 @@ from sqlalchemy.orm.exc import DetachedInstanceError from galaxy.util.hash_util import md5_hash_file +log = logging.getLogger(__name__) + class ToolCache(object): """ @@ -51,10 +54,13 @@ class ToolCache(object): self._removed_tool_ids.add(tool_id) if tool_id in self._new_tool_ids: self._new_tool_ids.remove(tool_id) - except Exception: + except Exception as e: + log.debug("Exception while checking tools to remove from cache: %s" % e) # If by chance the file is being removed while calculating the hash or modtime # we don't want the thread to die. pass + if removed_tool_ids: + log.debug("Removed the following tools from cache: %s" % removed_tool_ids) return removed_tool_ids def _should_cleanup(self, config_filename): diff --git a/test/unit/tools/test_toolbox.py b/test/unit/tools/test_toolbox.py index dfdb0204c12..6acd49783cc 100644 --- a/test/unit/tools/test_toolbox.py +++ b/test/unit/tools/test_toolbox.py @@ -193,7 +193,6 @@ class ToolBoxTestCase(BaseToolBoxTestCase): assert tool is not None assert len(tool._macro_paths) == 1 macro_path = tool._macro_paths[0] - time.sleep(1.5) with open(macro_path, 'w') as macro_out: macro_out.write(SIMPLE_MACRO.substitute(tool_version="3.0")) time.sleep(1.5) From d3652f871a59e1dc9be40aaea693fb4219dbb48c Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Sat, 2 Mar 2019 23:38:28 +0000 Subject: [PATCH 3/7] Use lock also for ``ToolCache.reset_status()`` method --- lib/galaxy/tools/cache.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/tools/cache.py b/lib/galaxy/tools/cache.py index dd662f528dc..d03445c43da 100644 --- a/lib/galaxy/tools/cache.py +++ b/lib/galaxy/tools/cache.py @@ -122,9 +122,10 @@ class ToolCache(object): """ Reset tracking of new and newly disabled tools. """ - self._new_tool_ids = set() - self._removed_tool_ids = set() - self._removed_tools_by_path = {} + with self._lock: + self._new_tool_ids = set() + self._removed_tool_ids = set() + self._removed_tools_by_path = {} class ToolShedRepositoryCache(object): From 294c7b6eebfb02f947697111c82033b3ba68be04 Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Sun, 3 Mar 2019 13:01:56 +0000 Subject: [PATCH 4/7] Use ``_try_until_no_errors`` also for ``test_tool_reload_when_macro_is_altered`` unit test --- test/unit/tools/test_toolbox.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/unit/tools/test_toolbox.py b/test/unit/tools/test_toolbox.py index 6acd49783cc..89f690a49eb 100644 --- a/test/unit/tools/test_toolbox.py +++ b/test/unit/tools/test_toolbox.py @@ -195,10 +195,12 @@ class ToolBoxTestCase(BaseToolBoxTestCase): macro_path = tool._macro_paths[0] with open(macro_path, 'w') as macro_out: macro_out.write(SIMPLE_MACRO.substitute(tool_version="3.0")) - time.sleep(1.5) - tool = self.app.toolbox.get_tool("tool_with_macro") - assert tool.version == "3.0" + def check_tool_macro(): + tool = self.app.toolbox.get_tool("tool_with_macro") + assert tool.version == "3.0" + + self._try_until_no_errors(check_tool_macro) def test_tool_reload_for_broken_tool(self): self._init_tool(filename="simple_tool.xml", version="1.0") From 999c2c038e850a75d5fce0030bdcdbcf3bf0b13d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 4 Mar 2019 14:30:28 +0100 Subject: [PATCH 5/7] Store toolbox in test_case._toolbox Apparently accessing double-underscore variables in threads leads to name-mangling issues, so effectively updating test_case.__toolbox wouldn't be reflected outside of the watcher thread. We previously used self.app.toolbox which circumenvented this issue. It is possible that `check_tool_errors` / `check_no_tool_errors` were accessing the old toolbox, unless the watching thread happened to fire between function definition and assert, and that's probably why increasing the number of trials had no effect. --- test/unit/tools/test_toolbox.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/test/unit/tools/test_toolbox.py b/test/unit/tools/test_toolbox.py index 89f690a49eb..cf4c826b6cc 100644 --- a/test/unit/tools/test_toolbox.py +++ b/test/unit/tools/test_toolbox.py @@ -51,11 +51,9 @@ class BaseToolBoxTestCase(unittest.TestCase, UsesApp, UsesTools): @property def toolbox(self): - if self.__toolbox is None: - self.__toolbox = SimplifiedToolBox(self) - # wire app with this new toolbox - self.app.toolbox = self.__toolbox - return self.__toolbox + if self._toolbox is None: + self.app.toolbox = self._toolbox = SimplifiedToolBox(self) + return self._toolbox def setUp(self): self.reindexed = False @@ -67,7 +65,7 @@ class BaseToolBoxTestCase(unittest.TestCase, UsesApp, UsesTools): itp_config = os.path.join(self.test_directory, "integrated_tool_panel.xml") self.app.config.integrated_tool_panel_config = itp_config self.app.watchers = ConfigWatchers(self.app) - self.__toolbox = None + self._toolbox = None self.config_files = [] def _repo_install(self, changeset, config_filename=None): @@ -197,7 +195,7 @@ class ToolBoxTestCase(BaseToolBoxTestCase): macro_out.write(SIMPLE_MACRO.substitute(tool_version="3.0")) def check_tool_macro(): - tool = self.app.toolbox.get_tool("tool_with_macro") + tool = self.toolbox.get_tool("tool_with_macro") assert tool.version == "3.0" self._try_until_no_errors(check_tool_macro) @@ -216,7 +214,7 @@ class ToolBoxTestCase(BaseToolBoxTestCase): out.write('certainly not a valid tool') def check_tool_errors(): - tool = self.app.toolbox.get_tool("test_tool") + tool = self.toolbox.get_tool("test_tool") assert tool is not None assert tool.version == "1.0" assert tool.tool_errors == 'Current on-disk tool is not valid' @@ -227,7 +225,7 @@ class ToolBoxTestCase(BaseToolBoxTestCase): self._init_tool(filename="simple_tool.xml", version="2.0") def check_no_tool_errors(): - tool = self.app.toolbox.get_tool("test_tool") + tool = self.toolbox.get_tool("test_tool") assert tool is not None assert tool.version == "2.0" assert tool.tool_errors is None @@ -562,4 +560,4 @@ class SimplifiedToolBox(ToolBox): def reload_callback(test_case): test_case.app.tool_cache.cleanup() - test_case.__toolbox = test_case.app.toolbox = SimplifiedToolBox(test_case) + test_case._toolbox = test_case.app.toolbox = SimplifiedToolBox(test_case) From b84607728d7754347de94a1787f00e5a96cebde1 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 4 Mar 2019 14:40:46 +0100 Subject: [PATCH 6/7] Tear down started threads --- test/unit/tools/test_toolbox.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/unit/tools/test_toolbox.py b/test/unit/tools/test_toolbox.py index cf4c826b6cc..7d1eda35d17 100644 --- a/test/unit/tools/test_toolbox.py +++ b/test/unit/tools/test_toolbox.py @@ -68,6 +68,9 @@ class BaseToolBoxTestCase(unittest.TestCase, UsesApp, UsesTools): self._toolbox = None self.config_files = [] + def tearDown(self): + self.app.watchers.shutdown() + def _repo_install(self, changeset, config_filename=None): metadata = { 'tools': [{ From c1f8221b0bd9e8235a24d387990586b9dbbcb21c Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 4 Mar 2019 12:00:25 +0100 Subject: [PATCH 7/7] Use ``with`` statement --- test/unit/tools_support.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit/tools_support.py b/test/unit/tools_support.py index 0f9e9d617fa..a5a9243f1b2 100644 --- a/test/unit/tools_support.py +++ b/test/unit/tools_support.py @@ -102,7 +102,8 @@ class UsesTools(object): def __write_tool(self, contents, path=None): path = path or self.tool_file - open(path, "w").write(contents) + with open(path, "w") as out: + out.write(contents) class MockContext(object):