Skip to content

Commit 3944ca0

Browse files
committed
fixup: pr feedback
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
1 parent 556afed commit 3944ca0

5 files changed

Lines changed: 84 additions & 12 deletions

File tree

openfeature/_api.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,13 +104,19 @@ def set_evaluation_context(self, evaluation_context: EvaluationContext) -> None:
104104
raise GeneralError(error_message="No api level evaluation context")
105105
self._evaluation_context = evaluation_context
106106

107+
def clear_evaluation_context(self) -> None:
108+
self.set_evaluation_context(EvaluationContext())
109+
107110
# --- Transaction context ---
108111

109112
def set_transaction_context_propagator(
110113
self, transaction_context_propagator: TransactionContextPropagator
111114
) -> None:
112115
self._transaction_context_propagator = transaction_context_propagator
113116

117+
def clear_transaction_context_propagator(self) -> None:
118+
self.set_transaction_context_propagator(NoOpTransactionContextPropagator())
119+
114120
def get_transaction_context(self) -> EvaluationContext:
115121
return self._transaction_context_propagator.get_transaction_context()
116122

openfeature/_event_support.py

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import atexit
44
import threading
55
import typing
6+
import weakref
67
from collections import defaultdict
78
from collections.abc import Callable
89
from concurrent.futures import ThreadPoolExecutor
@@ -46,9 +47,11 @@ def __init__(self) -> None:
4647
)
4748

4849
self._client_lock = threading.RLock()
49-
self._client_handlers: dict[
50+
# WeakKeyDictionary so clients are not retained by the registry; this
51+
# avoids memory leaks and a reference cycle between API and clients.
52+
self._client_handlers: weakref.WeakKeyDictionary[
5053
OpenFeatureClient, dict[ProviderEvent, list[EventHandler]]
51-
] = defaultdict(lambda: defaultdict(list))
54+
] = weakref.WeakKeyDictionary()
5255

5356
def run_client_handlers(
5457
self, client: OpenFeatureClient, event: ProviderEvent, details: EventDetails
@@ -74,17 +77,21 @@ def add_client_handler(
7477
self, client: OpenFeatureClient, event: ProviderEvent, handler: EventHandler
7578
) -> None:
7679
with self._client_lock:
77-
handlers = self._client_handlers[client][event]
78-
handlers.append(handler)
80+
handlers_by_event = self._client_handlers.setdefault(
81+
client, defaultdict(list)
82+
)
83+
handlers_by_event[event].append(handler)
7984

8085
self._run_immediate_handler(client, event, handler)
8186

8287
def remove_client_handler(
8388
self, client: OpenFeatureClient, event: ProviderEvent, handler: EventHandler
8489
) -> None:
8590
with self._client_lock:
86-
handlers = self._client_handlers[client][event]
87-
handlers.remove(handler)
91+
handlers_by_event = self._client_handlers.get(client)
92+
if handlers_by_event is None:
93+
return
94+
handlers_by_event[event].remove(handler)
8895

8996
def add_global_handler(
9097
self,

openfeature/api.py

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,6 @@
99
from openfeature.provider import FeatureProvider
1010
from openfeature.provider.metadata import Metadata
1111
from openfeature.transaction_context import TransactionContextPropagator
12-
from openfeature.transaction_context.no_op_transaction_context_propagator import (
13-
NoOpTransactionContextPropagator,
14-
)
1512

1613
__all__ = [
1714
"add_handler",
@@ -90,7 +87,7 @@ def set_evaluation_context(evaluation_context: EvaluationContext) -> None:
9087

9188

9289
def clear_evaluation_context() -> None:
93-
set_evaluation_context(EvaluationContext())
90+
_default_api.clear_evaluation_context()
9491

9592

9693
def set_transaction_context_propagator(
@@ -100,7 +97,7 @@ def set_transaction_context_propagator(
10097

10198

10299
def clear_transaction_context_propagator() -> None:
103-
set_transaction_context_propagator(NoOpTransactionContextPropagator())
100+
_default_api.clear_transaction_context_propagator()
104101

105102

106103
def get_transaction_context() -> EvaluationContext:

openfeature/provider/_registry.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,14 @@
2727

2828

2929
def _register_binding(provider: FeatureProvider, owner: ProviderRegistry) -> None:
30+
try:
31+
weakref.ref(provider)
32+
except TypeError as exc:
33+
raise TypeError(
34+
f"Provider {type(provider).__name__!r} cannot be tracked because "
35+
"it is not weak-referenceable. If your provider class uses "
36+
"__slots__, add '__weakref__' to the slots list."
37+
) from exc
3038
with _binding_lock:
3139
existing = _provider_bindings.get(provider)
3240
if existing is not None and existing is not owner:

tests/test_isolated_api.py

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
"""Tests for isolated OpenFeature API instances (spec section 1.8)."""
22

3+
import gc
34
import inspect
45
import time
6+
import weakref
57
from unittest.mock import MagicMock
68

79
import pytest
@@ -12,7 +14,7 @@
1214
from openfeature.event import ProviderEvent, ProviderEventDetails
1315
from openfeature.hook import Hook
1416
from openfeature.isolated import OpenFeatureAPI, create_api
15-
from openfeature.provider import FeatureProvider, ProviderStatus
17+
from openfeature.provider import FeatureProvider, Metadata, ProviderStatus
1618
from openfeature.provider.no_op_provider import NoOpProvider
1719
from openfeature.transaction_context import ContextVarsTransactionContextPropagator
1820

@@ -46,8 +48,10 @@ def test_isolated_instance_is_openfeature_api():
4648
_ISOLATED_API_PUBLIC_METHODS = (
4749
"add_handler",
4850
"add_hooks",
51+
"clear_evaluation_context",
4952
"clear_hooks",
5053
"clear_providers",
54+
"clear_transaction_context_propagator",
5155
"get_client",
5256
"get_evaluation_context",
5357
"get_hooks",
@@ -189,6 +193,37 @@ def test_provider_can_be_rebound_after_being_released():
189193
assert api2.get_provider() is provider
190194

191195

196+
def test_set_provider_rejects_non_weak_referenceable_provider():
197+
"""Providers must be weak-referenceable so the SDK can track bindings
198+
without leaking memory; surfacing this requirement up front (rather than
199+
silently skipping the spec 1.8.4 check) avoids hard-to-diagnose bugs."""
200+
201+
# A direct ``object`` subclass with ``__slots__`` and no ``__weakref__``
202+
# entry; instances are not weak-referenceable. Implements the
203+
# ``FeatureProvider`` protocol structurally rather than via inheritance
204+
# (which would inherit ``__weakref__`` from the parent class).
205+
class NotWeakReferenceable:
206+
__slots__ = ()
207+
208+
def attach(self, on_emit):
209+
pass
210+
211+
def detach(self):
212+
pass
213+
214+
def get_metadata(self):
215+
return Metadata(name="not-weak-referenceable")
216+
217+
def get_provider_hooks(self):
218+
return []
219+
220+
provider = NotWeakReferenceable()
221+
api_instance = create_api()
222+
223+
with pytest.raises(TypeError, match="weak-referenceable"):
224+
api_instance.set_provider(provider) # type: ignore[arg-type]
225+
226+
192227
# --- Isolated state: hooks ---
193228

194229

@@ -320,6 +355,25 @@ def test_isolated_event_handlers_do_not_affect_global():
320355
assert handler.call_count == 0
321356

322357

358+
def test_client_handlers_do_not_retain_clients():
359+
"""Registering an event handler on a client must not keep the client
360+
(or its owning API) alive once the user drops their references."""
361+
api_instance = create_api()
362+
provider = NoOpProvider()
363+
api_instance.set_provider(provider)
364+
365+
client = api_instance.get_client(domain="ephemeral")
366+
client.add_handler(ProviderEvent.PROVIDER_CONFIGURATION_CHANGED, MagicMock())
367+
368+
client_ref = weakref.ref(client)
369+
assert client_ref() is client
370+
371+
del client
372+
gc.collect()
373+
374+
assert client_ref() is None
375+
376+
323377
# --- Provider lifecycle on isolated instances ---
324378

325379

0 commit comments

Comments
 (0)