Refactor globals in CacheConfig

synapse/config/cache.py

@@ -14,28 +14,40 @@
 # limitations under the License.
 
 import os
-from typing import Callable, Dict, Optional, Union
+from typing import Callable, Dict
 
 from ._base import Config, ConfigError
 
+# The prefix for all cache factor-related environment variables
 _CACHES = {}
 _CACHE_PREFIX = "SYNAPSE_CACHE_FACTOR"
 
-# Wrap all global vars into a single object to eliminate `global` calls
+
-CACHE_PROPERTIES = {
+class CacheProperties(object):
-    "prefix": _CACHE_PREFIX,
+    def __init__(self):
-    "default_size_factor": float(os.environ.get(_CACHE_PREFIX, 0.5)),
+        # The default size factor for all caches
-    # Callback to ensure that all caches are the correct size, registered when the
+        self.default_size_factor = 0.5
-    # configuration has been loaded.
+        self.resize_all_caches = None
-    "ensure_correct_cache_sizing": None,
-}
 
 
-def add_resizable_cache(cache_name, cache_resize_callback):
+properties = CacheProperties()
+
+
+def add_resizable_cache(cache_name: str, cache_resize_callback: Callable):
+    """Register a cache that's size can dynamically change
+
+    Args:
+        cache_name: A reference to the cache
+        cache_resize_callback: A callback function that will be ran whenever
+            the cache needs to be resized
+    """
     _CACHES[cache_name.lower()] = cache_resize_callback
 
-    if CACHE_PROPERTIES["ensure_correct_cache_sizing"]:
+    # Ensure all loaded caches are resized
-        CACHE_PROPERTIES["ensure_correct_cache_sizing"]()  # type: ignore[operator]
+    # This method should only run once the config has been read,
+    # as it uses variables from it
+    if properties.resize_all_caches:
+        properties.resize_all_caches()
 
 
 class CacheConfig(Config):
@@ -43,15 +55,10 @@ class CacheConfig(Config):
     _environ = os.environ
 
     @staticmethod
-    def _reset():
+    def reset():
-        """Resets the caches to their defaults.
+        """Resets the caches to their defaults. Used for tests."""
-
+        properties.default_size_factor = float(os.environ.get(_CACHE_PREFIX, 0.5))
-        Used for tests.
+        properties.resize_all_caches = None
-        """
-        CACHE_PROPERTIES["default_size_factor"] = float(
-            os.environ.get(_CACHE_PREFIX, 0.5)
-        )
-        CACHE_PROPERTIES["ensure_correct_cache_sizing"] = None
         _CACHES.clear()
 
     def generate_config_section(self, **kwargs):
@@ -73,34 +80,31 @@ class CacheConfig(Config):
 
     def read_config(self, config, **kwargs):
         self.event_cache_size = self.parse_size(config.get("event_cache_size", "10K"))
+        self.cache_factors = dict()  # type: Dict[str, float]
 
         cache_config = config.get("caches", {})
 
         self.global_factor = cache_config.get(
-            "global_factor", CACHE_PROPERTIES["default_size_factor"]
+            "global_factor", properties.default_size_factor
         )
         if not isinstance(self.global_factor, (int, float)):
             raise ConfigError("caches.global_factor must be a number.")
 
         # Set the global one so that it's reflected in new caches
-        CACHE_PROPERTIES["default_size_factor"] = self.global_factor
+        properties.default_size_factor = self.global_factor
 
         # Load cache factors from the environment, but override them with the
         # ones in the config file if they exist
         individual_factors = {
-            key[len(CACHE_PROPERTIES["prefix"]) + 1 :].lower(): float(val)  # type: ignore[arg-type]
+            key[len(_CACHE_PREFIX) + 1 :].lower(): float(val)
             for key, val in self._environ.items()
-            if key.startswith(CACHE_PROPERTIES["prefix"] + "_")  # type: ignore[operator]
+            if key.startswith(_CACHE_PREFIX + "_")
         }
-
         individual_factors_config = cache_config.get("per_cache_factors", {}) or {}
         if not isinstance(individual_factors_config, dict):
             raise ConfigError("caches.per_cache_factors must be a dictionary")
-
         individual_factors.update(individual_factors_config)
 
-        self.cache_factors = {}  # type: Dict[str, float]
-
         for cache, factor in individual_factors.items():
             if not isinstance(factor, (int, float)):
                 raise ConfigError(
@@ -108,11 +112,16 @@ class CacheConfig(Config):
                 )
             self.cache_factors[cache.lower()] = factor
 
-        # Register the global callback so that the individual cache sizes get set.
+        # Resize all caches (if necessary) with the new factors we've loaded
-        def ensure_cache_sizes():
+        properties.resize_all_caches = self.resize_all_caches
-            for cache_name, callback in _CACHES.items():
+        self.resize_all_caches()
-                new_factor = self.cache_factors.get(cache_name, self.global_factor)
-                callback(new_factor)
 
-        CACHE_PROPERTIES["ensure_correct_cache_sizing"] = ensure_cache_sizes
+    def resize_all_caches(self):
-        CACHE_PROPERTIES["ensure_correct_cache_sizing"]()  # type: ignore[operator]
+        """Ensure all cache sizes are up to date
+
+        For each cache, run the mapped callback function with either
+        a specific cache factor or the default, global one.
+        """
+        for cache_name, callback in _CACHES.items():
+            new_factor = self.cache_factors.get(cache_name, self.global_factor)
+            callback(new_factor)

synapse/util/caches/expiringcache.py

@@ -18,9 +18,9 @@ from collections import OrderedDict
 
 from six import iteritems, itervalues
 
+from synapse.config import cache as cache_config
 from synapse.metrics.background_process_metrics import run_as_background_process
 from synapse.util.caches import register_cache
-from synapse.config import cache as cache_config
 
 logger = logging.getLogger(__name__)
 
@@ -57,7 +57,7 @@ class ExpiringCache(object):
         self._cache_name = cache_name
 
         self._original_max_size = max_len
-        self._max_size = int(max_len * cache_config.CACHE_PROPERTIES["default_size_factor"])
+        self._max_size = int(max_len * cache_config.properties.default_size_factor)
 
         self._clock = clock
 

synapse/util/caches/lrucache.py

@@ -16,8 +16,8 @@
 import threading
 from functools import wraps
 
-from synapse.util.caches.treecache import TreeCache
 from synapse.config import cache as cache_config
+from synapse.util.caches.treecache import TreeCache
 
 
 def enumerate_leaves(node, depth):
@@ -79,7 +79,7 @@ class LruCache(object):
 
         # Save the original max size, and apply the default size factor.
         self._original_max_size = max_size
-        self.max_size = int(max_size * cache_config.CACHE_PROPERTIES["default_size_factor"])
+        self.max_size = int(max_size * cache_config.properties.default_size_factor)
 
         list_root = _Node(None, None, None, None)
         list_root.next_node = list_root

synapse/util/caches/stream_change_cache.py

@@ -14,8 +14,8 @@
 # limitations under the License.
 
 import logging
-from typing import Dict, Iterable, List, Mapping, Optional, Set
 import math
+from typing import Dict, Iterable, List, Mapping, Optional, Set
 
 from six import integer_types
 

tests/config/test_cache.py

@@ -30,7 +30,8 @@ class TestConfig(RootConfig):
 
 class CacheConfigTests(TestCase):
     def setUp(self):
-        CacheConfig._reset()
+        # Reset caches before each test
+        TestConfig().caches.reset()
 
     def test_individual_caches_from_environ(self):
         """
@@ -71,6 +72,7 @@ class CacheConfigTests(TestCase):
         is loaded.
         """
         cache = LruCache(100)
+
         add_resizable_cache("foo", cache.set_cache_factor)
         self.assertEqual(cache.max_size, 50)