Skip to content

Commit 2bf4a34

Browse files
fix: thread safety issues (#614)
* fix: thread-safety issues Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com> * minor corrections Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com> * add explanation comment Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com> * remove leftover files Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com> * lint fixes Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com> * implement feedback Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com> * implement pr feedback Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com> --------- Signed-off-by: Lea Konvalinka <lea.konvalinka@dynatrace.com>
1 parent 79bb01b commit 2bf4a34

6 files changed

Lines changed: 68 additions & 23 deletions

File tree

‎openfeature/client.py‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import logging
2+
import threading
23
import typing
34
from collections.abc import Awaitable, Mapping, Sequence
45
from dataclasses import dataclass
@@ -86,6 +87,7 @@ def __init__(
8687
self.version = version
8788
self.context = context or EvaluationContext()
8889
self.hooks = hooks or []
90+
self._hooks_lock = threading.RLock()
8991

9092
@property
9193
def provider(self) -> FeatureProvider:
@@ -98,7 +100,8 @@ def get_metadata(self) -> ClientMetadata:
98100
return ClientMetadata(domain=self.domain)
99101

100102
def add_hooks(self, hooks: list[Hook]) -> None:
101-
self.hooks = self.hooks + hooks
103+
with self._hooks_lock:
104+
self.hooks = self.hooks + hooks
102105

103106
def get_boolean_value(
104107
self,
@@ -468,8 +471,9 @@ def _establish_hooks_and_provider(
468471

469472
def _assert_provider_status(
470473
self,
474+
provider: FeatureProvider,
471475
) -> OpenFeatureError | None:
472-
status = self.get_provider_status()
476+
status = provider_registry.get_provider_status(provider)
473477
if status == ProviderStatus.NOT_READY:
474478
return ProviderNotReadyError()
475479
if status == ProviderStatus.FATAL:
@@ -589,7 +593,7 @@ async def evaluate_flag_details_async(
589593
)
590594

591595
try:
592-
if provider_err := self._assert_provider_status():
596+
if provider_err := self._assert_provider_status(provider):
593597
error_hooks(
594598
flag_type,
595599
provider_err,
@@ -765,7 +769,7 @@ def evaluate_flag_details(
765769
)
766770

767771
try:
768-
if provider_err := self._assert_provider_status():
772+
if provider_err := self._assert_provider_status(provider):
769773
error_hooks(
770774
flag_type,
771775
provider_err,

‎openfeature/hook/__init__.py‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from __future__ import annotations
22

3+
import threading
34
import typing
45
from collections.abc import Mapping, MutableMapping, Sequence
56
from datetime import datetime
@@ -24,6 +25,7 @@
2425
]
2526

2627
_hooks: list[Hook] = []
28+
_hooks_lock = threading.RLock()
2729

2830

2931
# https://openfeature.dev/specification/sections/hooks/#requirement-461
@@ -152,13 +154,15 @@ def supports_flag_value_type(self, flag_type: FlagType) -> bool:
152154

153155

154156
def add_hooks(hooks: list[Hook]) -> None:
155-
global _hooks
156-
_hooks = _hooks + hooks
157+
with _hooks_lock:
158+
global _hooks
159+
_hooks = _hooks + hooks
157160

158161

159162
def clear_hooks() -> None:
160-
global _hooks
161-
_hooks = []
163+
with _hooks_lock:
164+
global _hooks
165+
_hooks = []
162166

163167

164168
def get_hooks() -> list[Hook]:

‎openfeature/provider/__init__.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -261,5 +261,6 @@ def emit_provider_stale(self, details: ProviderEventDetails) -> None:
261261
self.emit(ProviderEvent.PROVIDER_STALE, details)
262262

263263
def emit(self, event: ProviderEvent, details: ProviderEventDetails) -> None:
264-
if hasattr(self, "_on_emit"):
265-
self._on_emit(self, event, details)
264+
on_emit = getattr(self, "_on_emit", None)
265+
if on_emit is not None:
266+
on_emit(self, event, details)

‎openfeature/transaction_context/__init__.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import threading
2+
13
from openfeature.evaluation_context import EvaluationContext
24
from openfeature.transaction_context.context_var_transaction_context_propagator import (
35
ContextVarsTransactionContextPropagator,
@@ -21,25 +23,27 @@
2123
_evaluation_transaction_context_propagator: TransactionContextPropagator = (
2224
NoOpTransactionContextPropagator()
2325
)
26+
_propagator_lock = threading.RLock()
2427

2528

2629
def set_transaction_context_propagator(
2730
transaction_context_propagator: TransactionContextPropagator,
2831
) -> None:
2932
global _evaluation_transaction_context_propagator
30-
_evaluation_transaction_context_propagator = transaction_context_propagator
33+
with _propagator_lock:
34+
_evaluation_transaction_context_propagator = transaction_context_propagator
3135

3236

3337
def clear_transaction_context_propagator() -> None:
3438
set_transaction_context_propagator(NoOpTransactionContextPropagator())
3539

3640

3741
def get_transaction_context() -> EvaluationContext:
38-
return _evaluation_transaction_context_propagator.get_transaction_context()
42+
propagator = _evaluation_transaction_context_propagator
43+
return propagator.get_transaction_context()
3944

4045

4146
def set_transaction_context(evaluation_context: EvaluationContext) -> None:
42-
global _evaluation_transaction_context_propagator
43-
_evaluation_transaction_context_propagator.set_transaction_context(
44-
evaluation_context
45-
)
47+
with _propagator_lock:
48+
propagator = _evaluation_transaction_context_propagator
49+
propagator.set_transaction_context(evaluation_context)

‎tests/test_client.py‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@
44
import types
55
import uuid
66
from concurrent.futures import ThreadPoolExecutor
7-
from unittest.mock import MagicMock
7+
from unittest.mock import MagicMock, Mock
88

99
import pytest
1010

1111
from openfeature import _event_support, api
12+
from openfeature import client as client_module
1213
from openfeature.api import (
1314
add_hooks,
1415
clear_hooks,
@@ -20,7 +21,7 @@
2021
from openfeature.client import OpenFeatureClient, _typecheck_flag_value
2122
from openfeature.evaluation_context import EvaluationContext
2223
from openfeature.event import EventDetails, ProviderEvent, ProviderEventDetails
23-
from openfeature.exception import ErrorCode, OpenFeatureError
24+
from openfeature.exception import ErrorCode, OpenFeatureError, ProviderFatalError
2425
from openfeature.flag_evaluation import FlagResolutionDetails, FlagType, Reason
2526
from openfeature.hook import Hook
2627
from openfeature.provider import FeatureProvider, ProviderStatus
@@ -291,9 +292,10 @@ def test_provider_should_return_error_status_if_failed():
291292
async def test_should_shortcircuit_if_provider_is_not_ready(
292293
no_op_provider_client, monkeypatch
293294
):
294-
# Given
295295
monkeypatch.setattr(
296-
no_op_provider_client, "get_provider_status", lambda: ProviderStatus.NOT_READY
296+
provider_registry,
297+
"get_provider_status",
298+
lambda provider: ProviderStatus.NOT_READY,
297299
)
298300
spy_hook = MagicMock(spec=Hook)
299301
no_op_provider_client.add_hooks([spy_hook])
@@ -321,9 +323,10 @@ async def test_should_shortcircuit_if_provider_is_not_ready(
321323
async def test_should_shortcircuit_if_provider_is_in_irrecoverable_error_state(
322324
no_op_provider_client, monkeypatch
323325
):
324-
# Given
325326
monkeypatch.setattr(
326-
no_op_provider_client, "get_provider_status", lambda: ProviderStatus.FATAL
327+
provider_registry,
328+
"get_provider_status",
329+
lambda provider: ProviderStatus.FATAL,
327330
)
328331
spy_hook = MagicMock(spec=Hook)
329332
no_op_provider_client.add_hooks([spy_hook])
@@ -768,3 +771,32 @@ def test_should_noop_if_provider_does_not_support_tracking(monkeypatch):
768771
set_provider(provider)
769772
client = get_client()
770773
client.track(tracking_event_name="test")
774+
775+
776+
def test_assert_provider_status_uses_passed_provider_not_current_registry_state():
777+
fatal_provider = NoOpProvider()
778+
ready_provider = NoOpProvider()
779+
780+
registry_mock = Mock()
781+
registry_mock.get_provider_status.side_effect = lambda p: (
782+
ProviderStatus.FATAL if p is fatal_provider else ProviderStatus.READY
783+
)
784+
registry_mock.get_provider.return_value = ready_provider
785+
786+
original = client_module.provider_registry
787+
client_module.provider_registry = registry_mock
788+
try:
789+
c = OpenFeatureClient(domain=None, version=None)
790+
assert c.provider is ready_provider, (
791+
"test setup: self.provider should resolve via the patched registry"
792+
)
793+
794+
err = c._assert_provider_status(fatal_provider)
795+
assert isinstance(err, ProviderFatalError), (
796+
"status check used self.provider (READY) instead of the captured "
797+
"fatal_provider — TOCTOU regression"
798+
)
799+
registry_mock.get_provider_status.assert_any_call(fatal_provider)
800+
assert c._assert_provider_status(ready_provider) is None
801+
finally:
802+
client_module.provider_registry = original

0 commit comments

Comments
 (0)