-
Notifications
You must be signed in to change notification settings - Fork 68
fix(multiprovider): Properly shutdown multiprovider initialization thread pool #1676
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
fix(multiprovider): Properly shutdown multiprovider initialization thread pool #1676
Conversation
Summary of ChangesHello @alxckn, 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 robustness of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
0524360 to
fa592fd
Compare
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 correctly refactors the provider initialization logic to prevent resource leaks from the thread pool by using a try-finally block. This ensures the ExecutorService is always shut down, even in the case of an exception. The change is a solid improvement for application stability. I have one suggestion to improve the error handling logic within the new try block.
...ltiprovider/src/main/java/dev/openfeature/contrib/providers/multiprovider/MultiProvider.java
Show resolved
Hide resolved
Signed-off-by: Alexandre Chakroun <[email protected]>
Signed-off-by: Alexandre Chakroun <[email protected]>
Signed-off-by: Alexandre Chakroun <[email protected]>
5ecda1f to
98aee38
Compare
| for (FeatureProvider provider : providers.values()) { | ||
| tasks.add(() -> { | ||
| provider.initialize(evaluationContext); | ||
| return 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.
Since we always return true and never use this value, could we get rid of this value entirely? Maybe with a Future<Void>?
| // Wait for all provider initializations to complete. | ||
| // If any provider.initialize() throws, result.get() will throw ExecutionException. | ||
| for (Future<Boolean> result : results) { | ||
| result.get(); |
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.
Maybe we should check for result.isCancelled(), and also add a sensible timeout result.get(timeout, TimeUnit.MILLISECONDS);.
|
@alxckn The multi provider was moved to the java-sdk repository: https://github.com/open-feature/java-sdk/tree/main/src/main/java/dev/openfeature/sdk/multiprovider |
|
@chrfwow @liran2000 thanks for reviewing sorry I hadn't noticed the migration, and happy to see it's been internalized and already fixed :) We'll migrate to fix our issue |
|
@alxckn it's migrated but not released! We have a few new things / fixes in Java though, so I'll try to get a release out soon (if not today): open-feature/java-sdk#1770 |
This PR
This pull request refactors the initialization logic in the
MultiProviderclass to ensure proper resource management for the thread pool used during provider initialization. The main change is moving the creation and shutdown of theExecutorServiceinto a try-finally block, which guarantees that the thread pool is always shut down, even if an exception occurs.We observed issues with some of our apps that fail to shutdown properly because of a hanging non-daemon thread, the changes proposed in this PR fix the issue (tested on our previously failing app).