fix: Handle closed EventChannel from multiprovider#461
fix: Handle closed EventChannel from multiprovider#461beeme1mr merged 5 commits intoopen-feature:mainfrom
Conversation
Summary of ChangesHello @saleemrashid, 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 resolves an intermittent state management problem within the multiprovider, preventing it from incorrectly reporting a 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.
Code Review
The pull request modifies openfeature/event_executor.go to enhance robustness when handling events. Specifically, a check if !ok { return } was added when receiving from v.EventChannel() within the startListeningAndShutdownOld function. This ensures that the goroutine gracefully exits if the event channel is closed, preventing potential panics or unexpected behavior.
The multiprovider closes the event channel under certain circumstances but the event processor was not handling this. This resulted in triggerEvent being called with zero-value events, for which stateFromEvent returns NotReady, resulting in the multiprovider being briefly READY and then becoming NOT_READY. Signed-off-by: Saleem Rashid <dev@saleemrashid.com>
510f98b to
d0a596a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
=======================================
Coverage 83.11% 83.11%
=======================================
Files 27 27
Lines 2097 2097
=======================================
Hits 1743 1743
Misses 305 305
Partials 49 49
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:
|
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
|
@saleemrashid What a bug... Could you please merge this PR into your branch? |
improve test coverage
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
fix race condition
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where a closed event channel from a multiprovider would cause an infinite loop of zero-value events. The fix involves checking the second return value from the channel receive operation to detect when the channel is closed, and the associated goroutine is now only started if the provider is an EventHandler. A comprehensive test case is added to verify this fix. My main feedback is to maintain consistency in the testing style by avoiding the introduction of a new testing library and instead using the existing helper functions and standard library assertions.
|
@toddbaert @sahidvelji could you please take a look? |
This PR
This fixes a bug where the multiprovider is briefly
READY, then becomesNOT_READY.The multiprovider closes the event channel under certain circumstances, e.g.:
But the
eventExecutorgoroutine copying from the provider's event channel was not checking if it was closed, so it was constantly reading zero-value events. This resulted intriggerEventbeing called constantly with zero-value events, for whichstateFromEventreturnsNotReady.