fix: call shutdown on providers before removing them #126
Conversation
Add tests demonstrating that shutdown() is not called on providers when: - OpenFeature::shutdown() is called - A provider is replaced via set_provider/set_named_provider Refs open-feature#124 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixes open-feature#124 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @nicklasl, 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 OpenFeature API by introducing a mechanism for graceful shutdown of feature providers. It ensures that any resources held by a provider are properly released when the provider is no longer in use, either due to being replaced or when the API itself is shutting down. This change improves resource management and prevents potential leaks or unexpected behavior from stale provider instances. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly adds the shutdown lifecycle method to FeatureProviders and ensures it's called when providers are replaced or cleared. The added tests are comprehensive and verify the new behavior effectively.
I've found a critical race condition in the implementation of set_default and set_named in provider_registry.rs. The current approach of removing the old provider, releasing the lock, and then re-acquiring it to insert the new one opens a window for concurrent modifications to corrupt the state. My review includes suggestions to fix this by using an atomic replacement pattern.
| pub async fn set_default<T: FeatureProvider>(&self, mut provider: T) { | ||
| let mut map = self.providers.write().await; | ||
| map.remove(""); | ||
| // Shutdown the old provider before replacing it. | ||
| if let Some(old_provider) = self.providers.write().await.remove("") { | ||
| old_provider.get().shutdown().await; | ||
| } | ||
|
|
||
| provider | ||
| .initialize(self.global_evaluation_context.get().await.borrow()) | ||
| .await; | ||
|
|
||
| map.insert(String::default(), FeatureProviderWrapper::new(provider)); | ||
| self.providers | ||
| .write() | ||
| .await | ||
| .insert(String::default(), FeatureProviderWrapper::new(provider)); | ||
| } |
There was a problem hiding this comment.
There's a potential race condition here. The provider map is unlocked between removing the old provider and inserting the new one. This could lead to issues if set_default is called concurrently from multiple threads. For example, one thread could set a provider that is immediately overwritten by another, and the first provider's shutdown method would never be called.
To fix this, you can perform the replacement atomically using HashMap::insert, which returns the old value. This also has the benefit of initializing the new provider before it's inserted into the registry.
| pub async fn set_default<T: FeatureProvider>(&self, mut provider: T) { | |
| let mut map = self.providers.write().await; | |
| map.remove(""); | |
| // Shutdown the old provider before replacing it. | |
| if let Some(old_provider) = self.providers.write().await.remove("") { | |
| old_provider.get().shutdown().await; | |
| } | |
| provider | |
| .initialize(self.global_evaluation_context.get().await.borrow()) | |
| .await; | |
| map.insert(String::default(), FeatureProviderWrapper::new(provider)); | |
| self.providers | |
| .write() | |
| .await | |
| .insert(String::default(), FeatureProviderWrapper::new(provider)); | |
| } | |
| pub async fn set_default<T: FeatureProvider>(&self, mut provider: T) { | |
| provider | |
| .initialize(self.global_evaluation_context.get().await.borrow()) | |
| .await; | |
| let old_provider = self | |
| .providers | |
| .write() | |
| .await | |
| .insert(String::default(), FeatureProviderWrapper::new(provider)); | |
| if let Some(old_provider) = old_provider { | |
| old_provider.get().shutdown().await; | |
| } | |
| } |
1 participant
Summary
shutdown()method toFeatureProvidertraitshutdown()on providers before removing them inset_default,set_named, andclearFixes #124
Test plan
shutdown()is called on providers🤖 Generated with Claude Code