fix: merge SetProviderAndWait fixes into next#475
fix: merge SetProviderAndWait fixes into next#475dd-oleksii wants to merge 5 commits intoopen-feature:nextfrom
Conversation
Signed-off-by: Gabe Gorelick <gabe@hightouch.io>
…#469) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Gabe Gorelick <gabe@hightouch.io>
Summary of ChangesHello @dd-oleksii, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates critical fixes and improvements related to provider registration and lifecycle management, specifically addressing the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the provider setting logic, particularly around SetProviderAndWait. The changes introduce a clearer separation of concerns for provider initialization and shutdown, using channels to manage asynchronous operations. This improves the robustness of setting providers. My review found a minor inconsistency in error handling for SetProviderAndWait compared to SetNamedProviderAndWait, for which I've suggested a fix to improve consistency and error reporting.
OpenFeature specification defines SetProviderAndWait as a waiting version of SetProvider, or as a shortcut for waiting on provider ready event. However, currently SetProvider and SetProviderAndWait exhibit non-trivial behavior differences besides waiting. SetProvider runs initialization asynchronously and potentially concurrently with shutdown of the old provider. The API is not blocked and the application author may initialize other providers concurrently, run evaluations, etc. 🐛: in this mode, the error from initializer is ignored when updating the provider state, so fatal/error states may be not set properly. SetProviderAndWait runs initialization synchronously while holding exclusive api.mu lock. This almost completely locks OpenFeature SDK: the application author cannot initialize other providers (for different domains), configure context or hooks, evaluate feature flags, or shutdown SDK. If a provider initialization blocks forever'ish, the SDK remains unusable and is unrecoverable. Another difference is that old provider is shutdown only after new provider has successfully initialized. 🐛: if the new provider fails to initialize, the old provider is already unset in API but will never be shutdown, and the new provider is not registered with api.eventExecutor (so if it comes back online after some time, nobody listens to its events, and the state will go out of sync if old provider continues emitting events). 🐛: in both modes, given that shutdown is run concurrently with updating subscriptions in eventExecutor, it is possible for the old provider to override the state of the new provider: 1. init finishes, emits provider ready event (directly from goroutine), updates state 2. old provider emits some event during shutdown (e.g., PROVIDER_ERROR or PROVIDER_STALE), eventExecutor receives the event and updates the state to error/stale 3. new provider is registered with eventExecutor but the state is already wrong. This PR introduces a couple of changes: Make initialization flow consistent across both modes: always initialize async but make "AndWait" methods wait for initialization outside of critical section. Make init respect returned error. Always call shutdown on old provider (if it is no longer used). Always register new provider with event executor. Do this before we start init/shutdown, so the old provider cannot influence state of the new provider. Make event executor registration non-erroring by making shutdown channel buffered (there were no good way to recover from registration error). Signed-off-by: Oleksii Shmalko <oleksii.shmalko@datadoghq.com>
87af314 to
5a69fbb
Compare
Signed-off-by: Oleksii Shmalko <oleksii.shmalko@datadoghq.com>
5a69fbb to
bf2f8f9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #475 +/- ##
=======================================
Coverage ? 86.15%
=======================================
Files ? 21
Lines ? 1936
Branches ? 0
=======================================
Hits ? 1668
Misses ? 231
Partials ? 37
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:
|
erka
left a comment
There was a problem hiding this comment.
Thank you @dd-oleksii but maybe it makes sense to focus on the original PR and the main branch for now. An extra PR might not add much value in fixing the issue and could increase the scope, especially with potential merge conflicts from other changes anyway.
This PR
Merge of #474 along with latest changes from
mainintonext. cc @erkaRelated Issues
#474
#460
Notes
Follow-up Tasks
How to test