Repository: Unleash/unleash-client-python
Pull request #231 (2023-01-05 07:37:25)

Feat/block multiple constructions
This introduces a new instance checking concept into the SDK, this is a result of regular misconfigurations across the board with the SDKs, which result in multiple polling instances hammering the server. By default the instance checking will raise a warning if the constructor is called more than once in a process with the same configuration. This also allows the consumer to change this to either not log the warning at all, in rare cases that multiple clients may be correct, or to raise the warning to an error and prevent multiple instantiations at all.

sighphyre: Build seems to be broken on this. Doesn't seem to be related to these changes (same thing happens on a new branch from main and the stuff that fails isn't related to this), but I am looking into it
coveralls: 
[![Coverage Status](https://coveralls.io/builds/55669165/badge)](https://coveralls.io/builds/55669165)

Coverage: 96.361% (+0.2%) from 96.184% when pulling **a0de8ebf475ef1a7b78e807d1d9980642834a619 on feat/block-multiple-constructions** into **d37ec32a436f62cc1e12c329418c225ce1a65c68 on main**.


Patch:
diff --git a/UnleashClient/__init__.py b/UnleashClient/__init__.py
index 9188054..71f8c6c 100644
--- a/UnleashClient/__init__.py
+++ b/UnleashClient/__init__.py
@@ -15,10 +15,13 @@ from UnleashClient.strategies import ApplicationHostname, Default, GradualRollou
     GradualRolloutSessionId, GradualRolloutUserId, UserWithId, RemoteAddress, FlexibleRollout
 from UnleashClient.constants import METRIC_LAST_SENT_TIME, DISABLED_VARIATION, ETAG
 from UnleashClient.loader import load_features
-from .utils import LOGGER
+from .utils import LOGGER, InstanceCounter, InstanceAllowType
 from .deprecation_warnings import strategy_v2xx_deprecation_check
 from .cache import BaseCache, FileCache
 
+
+INSTANCES = InstanceCounter()
+
 # pylint: disable=dangerous-default-value
 class UnleashClient:
     """
@@ -41,6 +44,7 @@ class UnleashClient:
     :param cache: Custom cache implementation that extends UnleashClient.cache.BaseCache.  When unset, UnleashClient will use Fcache.
     :param scheduler: Custom APScheduler object.  Use this if you want to customize jobstore or executors.  When unset, UnleashClient will create it's own scheduler.
     :param scheduler_executor: Name of APSCheduler executor to use if using a custom scheduler.
+    :param multiple_instance_mode: Determines how multiple instances being instantiated is handled by the SDK, when set to InstanceAllowType.BLOCK, the client constructor will fail when more than one instance is detected, when set to InstanceAllowType.WARN, multiple instances will be allowed but log a warning, when set to InstanceAllowType.SILENTLY_ALLOW, no warning or failure will be raised when instantiating multiple instances of the client. Defaults to InstanceAllowType.WARN
     """
     def __init__(self,
                  url: str,
@@ -61,7 +65,8 @@ class UnleashClient:
                  verbose_log_level: int = 30,
                  cache: Optional[BaseCache] = None,
                  scheduler: Optional[BaseScheduler] = None,
-                 scheduler_executor: Optional[str] = None) -> None:
+                 scheduler_executor: Optional[str] = None,
+                 multiple_instance_mode: InstanceAllowType = InstanceAllowType.WARN) -> None:
         custom_headers = custom_headers or {}
         custom_options = custom_options or {}
         custom_strategies = custom_strategies or {}
@@ -86,6 +91,8 @@ class UnleashClient:
         self.unleash_project_name = project_name
         self.unleash_verbose_log_level = verbose_log_level
 
+        self._do_instance_check(multiple_instance_mode)
+
         # Class objects
         self.features: dict = {}
         self.fl_job: Job = None
@@ -326,6 +333,20 @@ class UnleashClient:
             LOGGER.log(self.unleash_verbose_log_level, "Attempted to get feature flag/variation %s, but client wasn't initialized!", feature_name)
             return DISABLED_VARIATION
 
+    def _do_instance_check(self, multiple_instance_mode):
+        identifier = self.__get_identifier()
+        if identifier in INSTANCES:
+            msg = f"You already have {INSTANCES.count(identifier)} instance(s) configured for this config: {identifier}, please double check the code where this client is being instantiated."
+            if multiple_instance_mode == InstanceAllowType.BLOCK:
+                raise Exception(msg)
+            if multiple_instance_mode == InstanceAllowType.WARN:
+                LOGGER.error(msg)
+        INSTANCES.increment(identifier)
+
+    def __get_identifier(self):
+        api_key = self.unleash_custom_headers.get("Authorization") if self.unleash_custom_headers is not None else None
+        return f"apiKey:{api_key} appName:{self.unleash_app_name} instanceId:{self.unleash_instance_id}"
+
     def __enter__(self) -> "UnleashClient":
         self.initialize_client()
         return self
diff --git a/UnleashClient/utils.py b/UnleashClient/utils.py
index b60a453..70f7352 100644
--- a/UnleashClient/utils.py
+++ b/UnleashClient/utils.py
@@ -1,4 +1,6 @@
 import logging
+from enum import Enum
+from threading import RLock
 from typing import Any
 import mmh3  # pylint: disable=import-error
 from requests import Response
@@ -6,6 +8,36 @@ from requests import Response
 LOGGER = logging.getLogger('UnleashClient')
 
 
+class InstanceAllowType(Enum):
+    BLOCK = 1
+    WARN = 2
+    SILENTLY_ALLOW = 3
+
+
+class InstanceCounter:
+    def __init__(self):
+        self.instances = {}
+        self.lock = RLock()
+
+    def __contains__(self, key):
+        with self.lock:
+            return key in self.instances
+
+    def _reset(self):
+        self.instances = {}
+
+    def count(self, key):
+        with self.lock:
+            return self.instances.get(key) or 0
+
+    def increment(self, key):
+        with self.lock:
+            if key in self:
+                self.instances[key] += 1
+            else:
+                self.instances[key] = 1
+
+
 def normalized_hash(identifier: str,
                     activation_group: str,
                     normalizer: int = 100) -> int:
diff --git a/tox.ini b/tox.ini
index 8b33b1f..4623a92 100644
--- a/tox.ini
+++ b/tox.ini
@@ -4,9 +4,10 @@ envlist = py37,py38,py39,py310
 
 [testenv]
 deps = -rrequirements.txt
+allowlist_externals = sh
 commands =
     mypy UnleashClient --install-types --non-interactive
-    ./get-spec.sh
+    sh get-spec.sh
     pylint UnleashClient
     py.test tests/unit_tests
     py.test tests/specification_tests


Test patch:
diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py
index e676359..8714f71 100644
--- a/tests/unit_tests/test_client.py
+++ b/tests/unit_tests/test_client.py
@@ -7,8 +7,9 @@ import pytest
 import responses
 from apscheduler.schedulers.background import BackgroundScheduler
 from apscheduler.executors.pool import ThreadPoolExecutor
-from UnleashClient import UnleashClient
+from UnleashClient import UnleashClient, INSTANCES
 from UnleashClient.strategies import Strategy
+from UnleashClient.utils import InstanceAllowType
 from tests.utilities.testing_constants import URL, ENVIRONMENT, APP_NAME, INSTANCE_ID, REFRESH_INTERVAL, REFRESH_JITTER, \
     METRICS_INTERVAL, METRICS_JITTER, DISABLE_METRICS, DISABLE_REGISTRATION, CUSTOM_HEADERS, CUSTOM_OPTIONS, PROJECT_NAME, PROJECT_URL, \
     ETAG_VALUE
@@ -36,6 +37,11 @@ class EnvironmentStrategy(Strategy):
         return default_value
 
 
+@pytest.fixture(autouse=True)
+def before_each():
+    INSTANCES._reset()
+
+
 @pytest.fixture
 def cache(tmpdir):
     return FileCache(APP_NAME, directory=tmpdir.dirname)
@@ -535,3 +541,36 @@ def test_uc_custom_scheduler():
     responses.add(responses.GET, URL + FEATURES_URL, json=MOCK_ALL_FEATURES, status=200, headers={'etag': 'W/somethingelse'})
     time.sleep(6)
     assert len(unleash_client.features) >= 9
+
+
+def test_multiple_instances_blocks_client_instantiation():
+    with pytest.raises(Exception):
+        UnleashClient(URL, APP_NAME, multiple_instance_mode=InstanceAllowType.BLOCK)
+        UnleashClient(URL, APP_NAME, multiple_instance_mode=InstanceAllowType.BLOCK)
+
+
+def test_multiple_instances_with_allow_multiple_warns(caplog):
+    UnleashClient(URL, APP_NAME, multiple_instance_mode=InstanceAllowType.WARN)
+    UnleashClient(URL, APP_NAME, multiple_instance_mode=InstanceAllowType.WARN)
+    assert any(["You already have 1 instance" in r.msg for r in caplog.records])
+
+
+def test_multiple_instances_tracks_current_instance_count(caplog):
+    UnleashClient(URL, APP_NAME)
+    UnleashClient(URL, APP_NAME, multiple_instance_mode=InstanceAllowType.WARN)
+    UnleashClient(URL, APP_NAME, multiple_instance_mode=InstanceAllowType.WARN)
+    assert any(["You already have 1 instance" in r.msg for r in caplog.records])
+    assert any(["You already have 2 instance(s)" in r.msg for r in caplog.records])
+
+
+def test_multiple_instances_no_warnings_or_errors_with_different_client_configs(caplog):
+    UnleashClient(URL, "some-probably-unique-app-name")
+    UnleashClient(URL, "some-probably-unique-app-name", instance_id="some-unique-instance-id", refresh_interval="60")
+    UnleashClient(URL, "some-probably-unique-but-different-app-name", refresh_interval="60")
+    assert not all(["Multiple instances has been disabled" in r.msg for r in caplog.records])
+
+
+def test_multiple_instances_are_unique_on_api_key(caplog):
+    UnleashClient(URL, "some-probably-unique-app-name", custom_headers={"Authorization": "penguins"})
+    UnleashClient(URL, "some-probably-unique-app-name", custom_headers={"Authorization": "hamsters"})
+    assert not all(["Multiple instances has been disabled" in r.msg for r in caplog.records])