Skip to content

fix: isolate event handler errors per spec 5.2.5 #600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mujiaho wants to merge 1 commit into from
Closed

fix: isolate event handler errors per spec 5.2.5 #600

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions openfeature/_event_support.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
from __future__ import annotations

import logging
import threading
import typing
from collections import defaultdict

logger = logging.getLogger(__name__)

from openfeature.event import (
EventDetails,
EventHandler,
Expand All @@ -30,13 +33,29 @@ def run_client_handlers(
) -> None:
with _client_lock:
for handler in _client_handlers[client][event]:
handler(details)
try:
handler(details)
except Exception:
logger.warning(
"Error in client handler %s for event %s",
getattr(handler, "__name__", repr(handler)),
event,
exc_info=True,
)
Comment on lines 35 to +44
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Executing user-defined callbacks/handlers while holding a lock (_client_lock) is a known anti-pattern. If a handler blocks, performs a slow operation, or attempts to acquire another lock (or re-acquire this lock from another thread), it can lead to deadlocks or severely degrade performance by blocking other threads.

To prevent this, we should copy the list of handlers while holding the lock, and then iterate and execute them outside of the lock context.

Additionally, using __qualname__ instead of __name__ provides a more descriptive name for class methods or nested functions during logging.

Suggested change
for handler in _client_handlers[client][event]:
handler(details)
try:
handler(details)
except Exception:
logger.warning(
"Error in client handler %s for event %s",
getattr(handler, "__name__", repr(handler)),
event,
exc_info=True,
)
handlers = list(_client_handlers[client][event])
for handler in handlers:
try:
handler(details)
except Exception:
logger.warning(
"Error in client handler %s for event %s",
getattr(handler, "__qualname__", getattr(handler, "__name__", repr(handler))),
event,
exc_info=True,
)



def run_global_handlers(event: ProviderEvent, details: EventDetails) -> None:
with _global_lock:
for handler in _global_handlers[event]:
handler(details)
try:
handler(details)
except Exception:
logger.warning(
"Error in global handler %s for event %s",
getattr(handler, "__name__", repr(handler)),
event,
exc_info=True,
)
Comment on lines 49 to +58
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similarly to run_client_handlers, executing user-defined global handlers while holding _global_lock can lead to deadlocks or performance issues if a handler blocks or performs slow operations.

We should copy the list of global handlers while holding the lock, and then execute them outside of the lock context. We can also use __qualname__ for better logging details.

Suggested change
for handler in _global_handlers[event]:
handler(details)
try:
handler(details)
except Exception:
logger.warning(
"Error in global handler %s for event %s",
getattr(handler, "__name__", repr(handler)),
event,
exc_info=True,
)
handlers = list(_global_handlers[event])
for handler in handlers:
try:
handler(details)
except Exception:
logger.warning(
"Error in global handler %s for event %s",
getattr(handler, "__qualname__", getattr(handler, "__name__", repr(handler))),
event,
exc_info=True,
)



def add_client_handler(
Expand Down Expand Up @@ -98,7 +117,15 @@ def _run_immediate_handler(
ProviderStatus.STALE: ProviderEvent.PROVIDER_STALE,
}
if event == status_to_event.get(client.get_provider_status()):
handler(EventDetails(provider_name=client.provider.get_metadata().name))
try:
handler(EventDetails(provider_name=client.provider.get_metadata().name))
except Exception:
logger.warning(
"Error in immediate handler %s for event %s",
getattr(handler, "__name__", repr(handler)),
event,
exc_info=True,
)
Comment on lines +123 to +128
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For consistency and more descriptive logging (especially for class methods or nested functions), use __qualname__ if available.

Suggested change
logger.warning(
"Error in immediate handler %s for event %s",
getattr(handler, "__name__", repr(handler)),
event,
exc_info=True,
)
logger.warning(
"Error in immediate handler %s for event %s",
getattr(handler, "__qualname__", getattr(handler, "__name__", repr(handler))),
event,
exc_info=True,
)



def clear() -> None:
Expand Down