-
Notifications
You must be signed in to change notification settings - Fork 43
fix: Handle error state in Init() with TaskCompletionSource #571
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.
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Summary of ChangesHello @askpt, 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 enhances the error handling and initialization process of the Highlights
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.
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 refactors the initialization logic in InProcessResolver to use TaskCompletionSource instead of CountdownEvent, which is a good modernization. It also aims to fix a hang in Init() by handling RpcException. My review identifies a critical flaw in the error handling strategy: other types of exceptions are not handled, which can still lead to Init() hanging. I've provided a comprehensive suggestion to make the error handling more robust, ensuring that all exceptions are managed correctly and that the provider initialization is resilient.
src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/InProcessResolver.cs
Show resolved
Hide resolved
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
…) for improved test reliability Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/InProcessResolver.cs
Show resolved
Hide resolved
| { | ||
| // This is a transient error, so we signal that Init() can complete, | ||
| // but the provider is in an error state. We will then retry. | ||
| tcs.TrySetResult(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should set the state before or after we emitted the error event.
I am afraid that we might consider the provider to be in a non error state in the time from tcs.TrySetResult(true); to ProviderEvent?.Invoke(this, flagdEvent);, but I am also afraid to deadlock something (we had a roughly similar situation in the java flagd, which did result in a deadlock), because the event is not getting properly propagated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point and thank you for your insight coming from Java experience. Fixed with 7489afc
…race condition Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva 2493377+askpt@users.noreply.github.com
This PR
This pull request refactors the initialization and event handling logic in the
InProcessResolverto simplify thread management and improve reliability. The main change is replacing the use ofCountdownEventand a background thread with a more modernTaskCompletionSource-based approach for signaling initialization completion.Initialization and event handling improvements:
CountdownEventand manual thread management inInit()with aTaskCompletionSource<bool>andTask.Run, simplifying the initialization flow and reducing complexity.HandleEventsto accept aTaskCompletionSource<bool>instead of aCountdownEvent, and to signal initialization completion by callingTrySetResult(true)when the first event is received or on error. [1] [2] [3]Notes
This should fix the flaky tests because we were not "signalling" correctly when the exception happens.