Skip to content

fix: isolate provider event handler dispatch#599

Merged
gruebel merged 4 commits into
open-feature:mainfrom
Lucas-FManager:code/openfeature-event-handler-isolation-596
May 29, 2026
Merged

fix: isolate provider event handler dispatch#599
gruebel merged 4 commits into
open-feature:mainfrom
Lucas-FManager:code/openfeature-event-handler-isolation-596

Conversation

@Lucas-FManager
Copy link
Copy Markdown
Contributor

Problem

Provider event handlers currently execute synchronously while the handler registry lock is held. That means a handler exception can stop later handlers, and a slow handler can block the provider/client path that emitted the event.

Approach

  • Snapshot global/client handlers under the lock, then release the lock before dispatch.
  • Dispatch handlers through a shared ThreadPoolExecutor.
  • Wrap each handler invocation so one failing handler is logged and does not prevent the next handler from running.
  • Avoid the nested client-handler lock pattern in provider dispatch by snapshotting matching clients first.
  • Update event tests to wait for async handler dispatch and add coverage for error isolation and non-blocking emission.

Security

This avoids running arbitrary user event-handler code while internal handler registries are locked. It also keeps handler exceptions contained to the callback boundary instead of propagating into SDK/provider control flow.

Verification

  • uv run pytest tests -q -> 158 passed
  • uvx ruff check openfeature/_event_support.py tests/test_client.py tests/test_api.py -> passed
  • uvx ruff format --check openfeature/_event_support.py tests/test_client.py tests/test_api.py -> passed
  • git diff --check -> passed

Fixes #596

@Lucas-FManager Lucas-FManager requested review from a team as code owners May 27, 2026 18:34
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces asynchronous event handling using a ThreadPoolExecutor to execute OpenFeature event handlers, ensuring they do not block the emitter or other handlers. It also refactors locking mechanisms to release locks before handler execution and wraps handler execution in try-except blocks to prevent a single failing handler from halting subsequent ones. The review feedback recommends avoiding unnecessary mutation and potential memory leaks in defaultdict lookups by checking if keys exist before accessing them in both client and global handler registries.

Comment thread openfeature/_event_support.py Outdated
Comment thread openfeature/_event_support.py Outdated
@Lucas-FManager Lucas-FManager force-pushed the code/openfeature-event-handler-isolation-596 branch from 13de63c to 56e5045 Compare May 27, 2026 18:37
@Lucas-FManager Lucas-FManager changed the title Isolate provider event handler dispatch fix: isolate provider event handler dispatch May 27, 2026
@Lucas-FManager Lucas-FManager force-pushed the code/openfeature-event-handler-isolation-596 branch from 56e5045 to fb4e9a1 Compare May 27, 2026 18:38
@Lucas-FManager
Copy link
Copy Markdown
Contributor Author

Updated in the latest push: run_client_handlers() now uses _client_handlers.get(client) and then handlers_by_event.get(event, ()), and run_global_handlers() uses _global_handlers.get(event, ()). That avoids mutating the defaultdict during read-only dispatch while preserving the async/error-isolated behavior.

Re-verified after the change:

  • uv run pytest tests -q -> 158 passed
  • uvx ruff check openfeature/_event_support.py tests/test_client.py tests/test_api.py -> passed
  • uvx ruff format --check openfeature/_event_support.py tests/test_client.py tests/test_api.py -> passed
  • git diff --check -> passed

Copy link
Copy Markdown
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

overall looks good and addresses the mentioned issues. added just a few comments, thanks 🍻

Comment thread openfeature/_event_support.py Outdated
Comment thread openfeature/_event_support.py
Comment thread tests/test_client.py
@Lucas-FManager Lucas-FManager force-pushed the code/openfeature-event-handler-isolation-596 branch from 0db2460 to 5dfdfd9 Compare May 28, 2026 23:47
@codecov
Copy link
Copy Markdown

codecov Bot commented May 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.34%. Comparing base (cbacef0) to head (a049b00).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #599      +/-   ##
==========================================
+ Coverage   98.28%   98.34%   +0.06%     
==========================================
  Files          45       45              
  Lines        2386     2477      +91     
==========================================
+ Hits         2345     2436      +91     
  Misses         41       41              
Flag Coverage Δ
unittests 98.34% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Lucas-FManager
Copy link
Copy Markdown
Contributor Author

Small follow-up after the Codecov report: I added a focused regression test for the read-only client-dispatch no-op path, so _event_support.py is locally covered at 100%.

Verification after the new commit:

  • uv run pytest tests/test_client.py tests/test_api.py -q -> 74 passed
  • uv run coverage run -m pytest tests/test_client.py tests/test_api.py -q; uv run coverage report -m openfeature/_event_support.py -> _event_support.py 100%
  • uvx prek run --all-files --show-diff-on-failure -> passed

The main build workflow is still waiting for maintainer approval for the fork run, so there is no new CI failure on the latest head yet.

Comment thread .pre-commit-config.yaml Outdated
@toddbaert
Copy link
Copy Markdown
Member

I made some small test improvements, and also shut down the executor on exit.

@toddbaert toddbaert requested a review from gruebel May 29, 2026 19:02
@toddbaert toddbaert force-pushed the code/openfeature-event-handler-isolation-596 branch from 0f441d7 to 2bd3e58 Compare May 29, 2026 19:20
Lucas-FManager and others added 4 commits May 29, 2026 15:23
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert toddbaert force-pushed the code/openfeature-event-handler-isolation-596 branch from 2bd3e58 to a049b00 Compare May 29, 2026 19:23
@toddbaert toddbaert self-requested a review May 29, 2026 19:24
@gruebel gruebel merged commit 0a96426 into open-feature:main May 29, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Event handler error isolation and async execution

3 participants