feat: all executors now create daemon threads to reduce shutdown time #1715
Conversation
Signed-off-by: jarebudev <[email protected]>
Summary of ChangesHello @jarebudev, 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 SDK's thread management by configuring all internal 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 changes the threads created by executors in EventProvider and EventSupport to be daemon threads. The goal is to prevent these threads from blocking application shutdown. While this change can improve shutdown behavior in cases where the SDK's shutdown() methods are not called, it introduces a significant risk. Daemon threads can be terminated abruptly by the JVM, without guaranteeing the execution of finally blocks. This could lead to issues like resource leaks (e.g., locks not being released) or incomplete critical operations in event handlers. I've left specific comments in EventProvider.java and EventSupport.java detailing these risks. I recommend reconsidering this change and keeping the threads as non-daemon to enforce proper lifecycle management and ensure that cleanup operations are always executed.
| private final ExecutorService emitterExecutor = | ||
| Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-event-emitter-thread")); | ||
| Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-event-emitter-thread", true)); |
There was a problem hiding this comment.
Changing the threads to daemon threads introduces a risk. Daemon threads can be abruptly terminated by the JVM on shutdown, which does not guarantee the execution of finally blocks.
In the task submitted to emitterExecutor (lines 93-104), there is a finally block that calls awaitable.wakeup() and a try-with-resources statement that releases a read lock. If this thread is terminated abruptly, the Awaitable might never be woken up, and the lock might not be released.
While the shutdown() method attempts a graceful shutdown, using daemon threads can hide lifecycle management issues where shutdown() is not called. Given the critical operations within the finally block, it might be safer to use non-daemon threads to ensure cleanup logic is always executed, forcing the SDK user to manage the provider lifecycle correctly.
There was a problem hiding this comment.
I don't think we care about a lock not getting released when the JVM is terminated
| private final ExecutorService taskExecutor = | ||
| Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-event-handler-thread")); | ||
| Executors.newCachedThreadPool(new ConfigurableThreadFactory("openfeature-event-handler-thread", true)); |
There was a problem hiding this comment.
Similar to my comment on EventProvider.java, changing the threads for taskExecutor to daemon threads has risks. The tasks run by this executor are user-provided event handlers (handler.accept(eventDetails) on line 132).
If these handlers perform operations that require cleanup in finally blocks (e.g., closing files, releasing locks, committing transactions), using daemon threads means this cleanup is not guaranteed to run if the JVM shuts down. This could lead to resource leaks or data corruption.
It would be safer to use non-daemon threads to ensure that handlers can complete their work, or at least their cleanup routines, during a graceful shutdown. This would enforce correct lifecycle management by the user of the SDK.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1715 +/- ##
============================================
+ Coverage 92.47% 93.73% +1.25%
- Complexity 527 530 +3
============================================
Files 52 52
Lines 1276 1276
Branches 112 112
============================================
+ Hits 1180 1196 +16
+ Misses 60 46 -14
+ Partials 36 34 -2
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:
|
2 participants
This PR
Following on from #1633, where a
ConfigurableThreadFactorywas introduced to allow Threads created by an Executor to be named and either be created as daemon/non-daemon, this PR now changes the SDK so that all executors will create daemon threads.Related Issues
Resolves #1580
Notes
Follow-up Tasks
How to test