From 14f5294c825b4cca73be8924b44cfc5156d1a3fb Mon Sep 17 00:00:00 2001 From: Oliver Layer Date: Thu, 26 Oct 2023 16:38:00 +0200 Subject: [PATCH] PR feedback --- homcc/server/cache.py | 15 ++-- tests/server/cache_test.py | 163 ++++++++++++++++++------------------- 2 files changed, 86 insertions(+), 92 deletions(-) diff --git a/homcc/server/cache.py b/homcc/server/cache.py index f2de2cf..9cca4e7 100644 --- a/homcc/server/cache.py +++ b/homcc/server/cache.py @@ -22,7 +22,7 @@ class Cache: """Path to the cache on the file system.""" max_size_bytes: int """Maximum size of the cache in bytes.""" - current_size: int + current_size_bytes: int """Current size of the cache in bytes.""" def __init__(self, root_folder: Path, max_size_bytes: int): @@ -33,7 +33,7 @@ def __init__(self, root_folder: Path, max_size_bytes: int): self.cache: OrderedDict[str, str] = OrderedDict() self.cache_mutex: Lock = Lock() self.max_size_bytes = max_size_bytes - self.current_size = 0 + self.current_size_bytes = 0 def _get_cache_file_path(self, hash_value: str) -> Path: return self.cache_folder / hash_value @@ -55,7 +55,7 @@ def _evict_oldest(self): Evicts the oldest entry from the cache. Note: The caller of this method has to ensure that the cache is locked. """ - oldest_hash = next(iter(self.cache)) + oldest_hash, _ = self.cache.popitem(last=False) oldest_path = self._get_cache_file_path(oldest_hash) oldest_size = oldest_path.stat().st_size @@ -68,8 +68,7 @@ def _evict_oldest(self): oldest_path, ) - self.current_size -= oldest_size - del self.cache[oldest_hash] + self.current_size_bytes -= oldest_size @staticmethod def _create_cache_folder(root_temp_folder: Path) -> Path: @@ -92,7 +91,7 @@ def put(self, hash_value: str, content: bytearray): logger.error( """File with hash '%s' can not be added to cache as it is larger than the maximum cache size. (size in bytes: %i, max. cache size in bytes: %i)""", - hash, + hash_value, len(content), self.max_size_bytes, ) @@ -100,9 +99,9 @@ def put(self, hash_value: str, content: bytearray): cached_file_path = self._get_cache_file_path(hash_value) with self.cache_mutex: - while self.current_size + len(content) > self.max_size_bytes: + while self.current_size_bytes + len(content) > self.max_size_bytes: self._evict_oldest() Path.write_bytes(cached_file_path, content) - self.current_size += len(content) + self.current_size_bytes += len(content) self.cache[hash_value] = str(cached_file_path) diff --git a/tests/server/cache_test.py b/tests/server/cache_test.py index 348ac88..93c2059 100644 --- a/tests/server/cache_test.py +++ b/tests/server/cache_test.py @@ -5,7 +5,6 @@ """Test module for the server cache.""" from pathlib import Path -from tempfile import TemporaryDirectory from homcc.server.cache import Cache @@ -13,34 +12,32 @@ class TestCache: """Tests the server cache.""" - def test_simple(self): - with TemporaryDirectory() as tmp_dir: - root_dir = Path(tmp_dir) - cache = Cache(root_dir, 1000) - cache_dir = root_dir / "cache" + def test_simple(self, tmp_path: Path): + cache = Cache(tmp_path, 1000) + cache_dir = tmp_path / "cache" - file1 = bytearray([0x1, 0x2, 0x3, 0x9]) - cache.put("hash1", file1) + file1 = bytearray([0x1, 0x2, 0x3, 0x9]) + cache.put("hash1", file1) - assert cache.get("hash1") == str(cache_dir / "hash1") - assert "hash1" in cache - assert Path.read_bytes(Path(cache.get("hash1"))) == file1 + assert cache.get("hash1") == str(cache_dir / "hash1") + assert "hash1" in cache + assert Path.read_bytes(Path(cache.get("hash1"))) == file1 - file2 = bytearray([0x3, 0x6, 0x3, 0x9]) - cache.put("hash2", file2) + file2 = bytearray([0x3, 0x6, 0x3, 0x9]) + cache.put("hash2", file2) - assert cache.get("hash2") == str(cache_dir / "hash2") - assert "hash2" in cache - assert Path.read_bytes(Path(cache.get("hash2"))) == file2 + assert cache.get("hash2") == str(cache_dir / "hash2") + assert "hash2" in cache + assert Path.read_bytes(Path(cache.get("hash2"))) == file2 - file3 = bytearray([0x4, 0x2]) - cache.put("hash3", file3) + file3 = bytearray([0x4, 0x2]) + cache.put("hash3", file3) - assert cache.get("hash3") == str(cache_dir / "hash3") - assert "hash3" in cache - assert Path.read_bytes(Path(cache.get("hash3"))) == file3 + assert cache.get("hash3") == str(cache_dir / "hash3") + assert "hash3" in cache + assert Path.read_bytes(Path(cache.get("hash3"))) == file3 - assert "other_hash" not in cache + assert "other_hash" not in cache @staticmethod def assert_hash_in_cache(cache: Cache, hash_value: str): @@ -52,65 +49,63 @@ def assert_hash_not_in_cache(cache: Cache, hash_value: str): assert hash_value not in cache assert not (cache.cache_folder / hash_value).exists() - def test_eviction_size_limit(self): - with TemporaryDirectory() as tmp_dir: - cache = Cache(Path(tmp_dir), max_size_bytes=10) - - cache.put("hash1", bytearray([0x1, 0x2, 0x3, 0x9])) - cache.put("hash2", bytearray([0x1, 0x2, 0x3, 0xA])) - cache.put("hash3", bytearray([0xFF, 0xFF])) - assert len(cache) == 3 - self.assert_hash_in_cache(cache, "hash1") - self.assert_hash_in_cache(cache, "hash2") - self.assert_hash_in_cache(cache, "hash3") - - cache.put("hash4", bytearray([0x1])) - assert len(cache) == 3 - self.assert_hash_not_in_cache(cache, "hash1") - self.assert_hash_in_cache(cache, "hash2") - self.assert_hash_in_cache(cache, "hash3") - self.assert_hash_in_cache(cache, "hash4") - - cache.put("hash5", bytearray([0x1])) - assert len(cache) == 4 - self.assert_hash_in_cache(cache, "hash2") - self.assert_hash_in_cache(cache, "hash3") - self.assert_hash_in_cache(cache, "hash4") - self.assert_hash_in_cache(cache, "hash5") - - cache.put("hash6", bytearray([0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9])) - assert len(cache) == 2 - self.assert_hash_not_in_cache(cache, "hash2") - self.assert_hash_not_in_cache(cache, "hash3") - self.assert_hash_not_in_cache(cache, "hash4") - self.assert_hash_in_cache(cache, "hash5") - self.assert_hash_in_cache(cache, "hash6") - - def test_eviction_order_lru(self): - with TemporaryDirectory() as tmp_dir: - cache = Cache(Path(tmp_dir), max_size_bytes=10) - - cache.put("hash1", bytearray([0x1, 0x2, 0x3, 0x9])) - cache.put("hash2", bytearray([0x1, 0x2, 0x3, 0xA])) - cache.put("hash3", bytearray([0xFF, 0xFF])) - assert len(cache) == 3 - self.assert_hash_in_cache(cache, "hash1") - self.assert_hash_in_cache(cache, "hash2") - self.assert_hash_in_cache(cache, "hash3") - - cache.get("hash1") # make "hash1" the latest used element - cache.put("hash4", bytearray([0xFF, 0xFF, 0x0, 0x0])) - assert len(cache) == 3 - self.assert_hash_not_in_cache(cache, "hash2") - self.assert_hash_in_cache(cache, "hash1") - self.assert_hash_in_cache(cache, "hash3") - self.assert_hash_in_cache(cache, "hash4") - - assert "hash3" in cache # make "hash3" the latest used element - cache.put("hash5", bytearray([0xFF, 0xFF, 0x0, 0x0, 0xFF, 0xFF, 0x0, 0x0])) - assert len(cache) == 2 - self.assert_hash_in_cache(cache, "hash3") - self.assert_hash_in_cache(cache, "hash5") - self.assert_hash_not_in_cache(cache, "hash1") - self.assert_hash_not_in_cache(cache, "hash2") - self.assert_hash_not_in_cache(cache, "hash4") + def test_eviction_size_limit(self, tmp_path: Path): + cache = Cache(tmp_path, max_size_bytes=10) + + cache.put("hash1", bytearray([0x1, 0x2, 0x3, 0x9])) + cache.put("hash2", bytearray([0x1, 0x2, 0x3, 0xA])) + cache.put("hash3", bytearray([0xFF, 0xFF])) + assert len(cache) == 3 + self.assert_hash_in_cache(cache, "hash1") + self.assert_hash_in_cache(cache, "hash2") + self.assert_hash_in_cache(cache, "hash3") + + cache.put("hash4", bytearray([0x1])) + assert len(cache) == 3 + self.assert_hash_not_in_cache(cache, "hash1") + self.assert_hash_in_cache(cache, "hash2") + self.assert_hash_in_cache(cache, "hash3") + self.assert_hash_in_cache(cache, "hash4") + + cache.put("hash5", bytearray([0x1])) + assert len(cache) == 4 + self.assert_hash_in_cache(cache, "hash2") + self.assert_hash_in_cache(cache, "hash3") + self.assert_hash_in_cache(cache, "hash4") + self.assert_hash_in_cache(cache, "hash5") + + cache.put("hash6", bytearray([0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9])) + assert len(cache) == 2 + self.assert_hash_not_in_cache(cache, "hash2") + self.assert_hash_not_in_cache(cache, "hash3") + self.assert_hash_not_in_cache(cache, "hash4") + self.assert_hash_in_cache(cache, "hash5") + self.assert_hash_in_cache(cache, "hash6") + + def test_eviction_order_lru(self, tmp_path: Path): + cache = Cache(tmp_path, max_size_bytes=10) + + cache.put("hash1", bytearray([0x1, 0x2, 0x3, 0x9])) + cache.put("hash2", bytearray([0x1, 0x2, 0x3, 0xA])) + cache.put("hash3", bytearray([0xFF, 0xFF])) + assert len(cache) == 3 + self.assert_hash_in_cache(cache, "hash1") + self.assert_hash_in_cache(cache, "hash2") + self.assert_hash_in_cache(cache, "hash3") + + cache.get("hash1") # make "hash1" the latest used element + cache.put("hash4", bytearray([0xFF, 0xFF, 0x0, 0x0])) + assert len(cache) == 3 + self.assert_hash_not_in_cache(cache, "hash2") + self.assert_hash_in_cache(cache, "hash1") + self.assert_hash_in_cache(cache, "hash3") + self.assert_hash_in_cache(cache, "hash4") + + assert "hash3" in cache # make "hash3" the latest used element + cache.put("hash5", bytearray([0xFF, 0xFF, 0x0, 0x0, 0xFF, 0xFF, 0x0, 0x0])) + assert len(cache) == 2 + self.assert_hash_in_cache(cache, "hash3") + self.assert_hash_in_cache(cache, "hash5") + self.assert_hash_not_in_cache(cache, "hash1") + self.assert_hash_not_in_cache(cache, "hash2") + self.assert_hash_not_in_cache(cache, "hash4")