fix: isolate provider event handler dispatch#599
Conversation
There was a problem hiding this comment.
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.
13de63c to
56e5045
Compare
56e5045 to
fb4e9a1
Compare
|
Updated in the latest push: Re-verified after the change:
|
gruebel
left a comment
There was a problem hiding this comment.
overall looks good and addresses the mentioned issues. added just a few comments, thanks 🍻
0db2460 to
5dfdfd9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Small follow-up after the Codecov report: I added a focused regression test for the read-only client-dispatch no-op path, so Verification after the new commit:
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. |
|
I made some small test improvements, and also shut down the executor on exit. |
0f441d7 to
2bd3e58
Compare
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>
2bd3e58 to
a049b00
Compare
* 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(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>
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
ThreadPoolExecutor.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 passeduvx ruff check openfeature/_event_support.py tests/test_client.py tests/test_api.py-> passeduvx ruff format --check openfeature/_event_support.py tests/test_client.py tests/test_api.py-> passedgit diff --check-> passedFixes #596