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 ownersMay 27, 2026 18:34

@gemini-code-assistgemini-code-assistBot left a comment

Copy link
Copy Markdown

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 threadopenfeature/_event_support.py Outdated
Comment threadopenfeature/_event_support.py Outdated
@Lucas-FManager
Lucas-FManagerforce-pushed the code/openfeature-event-handler-isolation-596 branch from 13de63c to 56e5045CompareMay 27, 2026 18:37
@Lucas-FManagerLucas-FManager changed the title Isolate provider event handler dispatchfix: isolate provider event handler dispatchMay 27, 2026
@Lucas-FManager
Lucas-FManagerforce-pushed the code/openfeature-event-handler-isolation-596 branch from 56e5045 to fb4e9a1CompareMay 27, 2026 18:38
@Lucas-FManager

Copy link
Copy Markdown
ContributorAuthor

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

@gruebelgruebel left a comment

Copy link
Copy Markdown
Member

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 threadopenfeature/_event_support.py Outdated
Comment threadopenfeature/_event_support.py
Comment threadtests/test_client.py
@Lucas-FManager
Lucas-FManagerforce-pushed the code/openfeature-event-handler-isolation-596 branch from 0db2460 to 5dfdfd9CompareMay 28, 2026 23:47
@codecov

codecovBot commented May 29, 2026

Copy link
Copy Markdown

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 
FlagCoverage Δ
unittests98.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
ContributorAuthor

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 gruebelMay 29, 2026 19:02
@toddbaert
toddbaertforce-pushed the code/openfeature-event-handler-isolation-596 branch from 0f441d7 to 2bd3e58CompareMay 29, 2026 19:20
Lucas-FManagerand 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
toddbaertforce-pushed the code/openfeature-event-handler-isolation-596 branch from 2bd3e58 to a049b00CompareMay 29, 2026 19:23
@toddbaert
toddbaert self-requested a review May 29, 2026 19:24
@gruebel
gruebel merged commit 0a96426 into open-feature:mainMay 29, 2026
15 checks passed
gruebel pushed a commit to balgaly/python-sdk that referenced this pull request Jun 6, 2026
* Isolate provider event handlers
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
* Address event handler review feedback
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
* test: cover event dispatch noop path
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
* fixup: drain executor at exit and relax non-blocking test timing margin
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
---------
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
gruebel added a commit that referenced this pull request Jun 7, 2026
* test(e2e): add Behave step definitions for context merging
Add the missing E2E step definitions so the contextMerging.feature
scenarios from the OpenFeature spec run against python-sdk.
Fixes#500
Changes:
- Bump spec submodule to 130df3eb so contextMerging.feature is copied in
during the `poe e2e` task.
- Add tests/features/environment.py with a before_scenario hook that
resets provider/hook/API-context/transaction-context state, so
scenarios cannot leak state between features.
- Add tests/features/steps/context_merging_steps.py:
- RetrievableContextProvider captures the merged EvaluationContext
it receives, so assertions can inspect what the SDK merged.
- Step definitions for all scenarios in contextMerging.feature:
single-level insert, multi-level insert, and per-key overwrite
precedence across API / Transaction / Client / Invocation /
Before Hooks.
- Client-level context is set via direct attribute assignment on
OpenFeatureClient.context (no new setter), since merging already
honors client.context (openfeature/client.py:422-429).
Runs clean: 4 features / 50 scenarios / 233 steps.
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* docs: fix inaccuracies in README code examples (#592)
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* fix: correctly reset api state on shutdown (#589)
correctly reset api state on shutdown
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* chore(main): release 0.9.0 (#555)
Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* chore(deps): update pre-commit hook tox-dev/pyproject-fmt to v2.21.2 (#601)
* chore(deps): update pre-commit hook tox-dev/pyproject-fmt to v2.21.2
* upper bound toml-fmt-common till fixed
Signed-off-by: gruebel <anton.gruebel@gmail.com>
---------
Signed-off-by: gruebel <anton.gruebel@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: gruebel <anton.gruebel@gmail.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* chore(deps): update astral-sh/setup-uv action to v8 (#603)
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* chore(deps): update codecov/codecov-action action to v6 (#604)
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* chore(deps): update googleapis/release-please-action action to v5 (#605)
* chore(deps): update googleapis/release-please-action action to v5
* fix config
Signed-off-by: gruebel <anton.gruebel@gmail.com>
---------
Signed-off-by: gruebel <anton.gruebel@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: gruebel <anton.gruebel@gmail.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* chore(deps): update pre-commit hook pre-commit/mirrors-mypy to v2 (#606)
* chore(deps): update pre-commit hook pre-commit/mirrors-mypy to v2
* update config
Signed-off-by: gruebel <anton.gruebel@gmail.com>
---------
Signed-off-by: gruebel <anton.gruebel@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: gruebel <anton.gruebel@gmail.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* chore(deps): update dependency prek to >=0.4.3,<0.5.0 (#607)
* chore(deps): update dependency prek to >=0.4.3,<0.5.0
* adjust CI
Signed-off-by: gruebel <anton.gruebel@gmail.com>
---------
Signed-off-by: gruebel <anton.gruebel@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: gruebel <anton.gruebel@gmail.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* feat!: make set_provider non-blocking, add set_provider_and_wait (#595)
* feat!: make set_provider non-blocking, add set_provider_and_wait
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
* fix: ruff format signature collapse in api.py
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
* fix: use threading.Event in error event test to avoid flaky busy-wait
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
* fixup: pr feedback and additional checks
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
* fix: check active registration in stale-init guard, not _provider_status
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
* fixup: edge shutdown race
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
---------
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* fix: isolate provider event handler dispatch (#599)
* Isolate provider event handlers
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
* Address event handler review feedback
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
* test: cover event dispatch noop path
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
* fixup: drain executor at exit and relax non-blocking test timing margin
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
---------
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* test: fix flaky event handler test (#609)
fix flaky event handler test
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* chore(deps): update pre-commit hook astral-sh/ruff-pre-commit to v0.15.15 (#608)
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* chore(main): release 0.10.0 (#602)
* chore(main): release 0.10.0
Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com>
* docs: clarify non-blocking set_provider behavior in changelog
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
---------
Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* fix CR comments
Signed-off-by: gruebel <anton.gruebel@gmail.com>
* cleanup
Signed-off-by: gruebel <anton.gruebel@gmail.com>
---------
Signed-off-by: gruebel <anton.gruebel@gmail.com>
Signed-off-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
Co-authored-by: Jonathan Norris <jonathan.norris@dynatrace.com>
Co-authored-by: Anton Grübel <anton.gruebel@gmail.com>
Co-authored-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: Nguyen Cat Luong <pkiphone.anhluong@gmail.com>
Co-authored-by: Lucas-FManager <265058144+Lucas-FManager@users.noreply.github.com>
Sign up for freeto 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

@Lucas-FManager@toddbaert@gruebel