From 759a1c4d34c9d32dc65bfbb8c8b0a682a6aa206b Mon Sep 17 00:00:00 2001 From: Sven Schmit Date: Tue, 30 Apr 2024 17:43:02 -0700 Subject: [PATCH 1/5] use context managers for locks --- eppo_client/read_write_lock.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/eppo_client/read_write_lock.py b/eppo_client/read_write_lock.py index ef03431..6736839 100644 --- a/eppo_client/read_write_lock.py +++ b/eppo_client/read_write_lock.py @@ -1,6 +1,5 @@ import threading - -# Copied from: https://www.oreilly.com/library/view/python-cookbook/0596001673/ch06s04.html +from contextlib import contextmanager class ReadWriteLock: @@ -40,3 +39,19 @@ def acquire_write(self): def release_write(self): """Release a write lock.""" self._read_ready.release() + + @contextmanager + def reader(self): + try: + self.acquire_read() + yield + finally: + self.release_read() + + @contextmanager + def writer(self): + try: + self.acquire_write() + yield + finally: + self.release_write() From d25e4e45282ff9ee50813cc5a98df8e59d7c447b Mon Sep 17 00:00:00 2001 From: Sven Schmit Date: Tue, 30 Apr 2024 17:43:25 -0700 Subject: [PATCH 2/5] add lock to get_keys, use dict instead of lrucache --- eppo_client/__init__.py | 5 +---- eppo_client/configuration_store.py | 22 +++++++--------------- eppo_client/constants.py | 3 --- test/configuration_store_test.py | 4 ++-- 4 files changed, 10 insertions(+), 24 deletions(-) diff --git a/eppo_client/__init__.py b/eppo_client/__init__.py index d0185aa..695c646 100644 --- a/eppo_client/__init__.py +++ b/eppo_client/__init__.py @@ -5,7 +5,6 @@ ExperimentConfigurationRequestor, ) from eppo_client.configuration_store import ConfigurationStore -from eppo_client.constants import MAX_CACHE_ENTRIES from eppo_client.http_client import HttpClient, SdkParams from eppo_client.models import Flag from eppo_client.read_write_lock import ReadWriteLock @@ -31,9 +30,7 @@ def init(config: Config) -> EppoClient: apiKey=config.api_key, sdkName="python", sdkVersion=__version__ ) http_client = HttpClient(base_url=config.base_url, sdk_params=sdk_params) - config_store: ConfigurationStore[Flag] = ConfigurationStore( - max_size=MAX_CACHE_ENTRIES - ) + config_store: ConfigurationStore[Flag] = ConfigurationStore() config_requestor = ExperimentConfigurationRequestor( http_client=http_client, config_store=config_store ) diff --git a/eppo_client/configuration_store.py b/eppo_client/configuration_store.py index fec7689..72f1cdf 100644 --- a/eppo_client/configuration_store.py +++ b/eppo_client/configuration_store.py @@ -1,5 +1,4 @@ from typing import Dict, Optional, TypeVar, Generic -from cachetools import LRUCache from eppo_client.read_write_lock import ReadWriteLock @@ -7,27 +6,20 @@ class ConfigurationStore(Generic[T]): - def __init__(self, max_size: int): - self.__cache: LRUCache = LRUCache(maxsize=max_size) + def __init__(self): + self.__cache: Dict[str, T] = {} self.__lock = ReadWriteLock() def get_configuration(self, key: str) -> Optional[T]: - try: - self.__lock.acquire_read() - return self.__cache[key] - except KeyError: - return None # key does not exist - finally: - self.__lock.release_read() + with self.__lock.reader(): + return self.__cache.get(key, None) def set_configurations(self, configs: Dict[str, T]): - try: - self.__lock.acquire_write() + with self.__lock.writer(): self.__cache.clear() for key, config in configs.items(): self.__cache[key] = config - finally: - self.__lock.release_write() def get_keys(self): - return list(self.__cache.keys()) + with self.__lock.reader(): + return list(self.__cache.keys()) diff --git a/eppo_client/constants.py b/eppo_client/constants.py index ed6806b..a13e8c8 100644 --- a/eppo_client/constants.py +++ b/eppo_client/constants.py @@ -1,6 +1,3 @@ -# configuration cache -MAX_CACHE_ENTRIES = 1000 # arbitrary; the caching library requires a max limit - # poller SECOND_MILLIS = 1000 MINUTE_MILLIS = 60 * SECOND_MILLIS diff --git a/test/configuration_store_test.py b/test/configuration_store_test.py index a88ceab..72eef3f 100644 --- a/test/configuration_store_test.py +++ b/test/configuration_store_test.py @@ -5,7 +5,7 @@ TEST_MAX_SIZE = 10 -store: ConfigurationStore[str] = ConfigurationStore(max_size=TEST_MAX_SIZE) +store: ConfigurationStore[str] = ConfigurationStore() mock_flag = Flag( key="mock_flag", variation_type=VariationType.STRING, @@ -40,7 +40,7 @@ def test_evicts_old_entries_when_max_size_exceeded(): def test_evicts_old_entries_when_setting_new_flags(): - store: ConfigurationStore[str] = ConfigurationStore(max_size=TEST_MAX_SIZE) + store: ConfigurationStore[str] = ConfigurationStore() store.set_configurations({"flag": mock_flag, "second_flag": mock_flag}) assert store.get_configuration("flag") == mock_flag From 94b597f29a0d68fd90e91d5759e8b21733878b2d Mon Sep 17 00:00:00 2001 From: Sven Schmit Date: Tue, 30 Apr 2024 19:49:25 -0700 Subject: [PATCH 3/5] :broom: --- eppo_client/read_write_lock.py | 2 ++ eppo_client/version.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/eppo_client/read_write_lock.py b/eppo_client/read_write_lock.py index 6736839..f596e54 100644 --- a/eppo_client/read_write_lock.py +++ b/eppo_client/read_write_lock.py @@ -1,6 +1,8 @@ import threading from contextlib import contextmanager +# Adapted from: https://www.oreilly.com/library/view/python-cookbook/0596001673/ch06s04.html + class ReadWriteLock: """A lock object that allows many simultaneous "read locks", but diff --git a/eppo_client/version.py b/eppo_client/version.py index 0552768..131942e 100644 --- a/eppo_client/version.py +++ b/eppo_client/version.py @@ -1 +1 @@ -__version__ = "3.0.1" +__version__ = "3.0.2" From 7a3ab80f774c31b76da24f3c36b89e1a9f732672 Mon Sep 17 00:00:00 2001 From: Sven Schmit Date: Thu, 2 May 2024 16:09:08 -0700 Subject: [PATCH 4/5] clean up the RWLock --- eppo_client/__init__.py | 10 ++-------- eppo_client/read_write_lock.py | 10 ++-------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/eppo_client/__init__.py b/eppo_client/__init__.py index 695c646..884ae50 100644 --- a/eppo_client/__init__.py +++ b/eppo_client/__init__.py @@ -38,8 +38,7 @@ def init(config: Config) -> EppoClient: is_graceful_mode = config.is_graceful_mode global __client global __lock - try: - __lock.acquire_write() + with __lock.writer(): if __client: # if a client was already initialized, stop the background processes of the old client __client._shutdown() @@ -49,8 +48,6 @@ def init(config: Config) -> EppoClient: is_graceful_mode=is_graceful_mode, ) return __client - finally: - __lock.release_write() def get_instance() -> EppoClient: @@ -64,11 +61,8 @@ def get_instance() -> EppoClient: """ global __client global __lock - try: - __lock.acquire_read() + with __lock.reader(): if __client: return __client else: raise Exception("init() must be called before get_instance()") - finally: - __lock.release_read() diff --git a/eppo_client/read_write_lock.py b/eppo_client/read_write_lock.py index f596e54..3b55297 100644 --- a/eppo_client/read_write_lock.py +++ b/eppo_client/read_write_lock.py @@ -15,21 +15,15 @@ def __init__(self): def acquire_read(self): """Acquire a read lock. Blocks only if a thread has acquired the write lock.""" - self._read_ready.acquire() - try: + with self._read_ready: self._readers += 1 - finally: - self._read_ready.release() def release_read(self): """Release a read lock.""" - self._read_ready.acquire() - try: + with self._read_ready: self._readers -= 1 if not self._readers: self._read_ready.notify_all() - finally: - self._read_ready.release() def acquire_write(self): """Acquire a write lock. Blocks until there are no From 8a0093fc99b663121591ff61529a87fc300e3cc6 Mon Sep 17 00:00:00 2001 From: Sven Schmit Date: Thu, 2 May 2024 16:10:22 -0700 Subject: [PATCH 5/5] set config rather than copy --- eppo_client/configuration_store.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/eppo_client/configuration_store.py b/eppo_client/configuration_store.py index 72f1cdf..bdd57eb 100644 --- a/eppo_client/configuration_store.py +++ b/eppo_client/configuration_store.py @@ -16,9 +16,7 @@ def get_configuration(self, key: str) -> Optional[T]: def set_configurations(self, configs: Dict[str, T]): with self.__lock.writer(): - self.__cache.clear() - for key, config in configs.items(): - self.__cache[key] = config + self.__cache = configs def get_keys(self): with self.__lock.reader():