-
Notifications
You must be signed in to change notification settings - Fork 51
feat: all executors now create daemon threads to reduce shutdown time #1715
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
feat: all executors now create daemon threads to reduce shutdown time #1715
Conversation
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
|
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 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1715 +/- ##
============================================
+ Coverage 92.11% 93.22% +1.10%
- Complexity 614 617 +3
============================================
Files 55 55
Lines 1446 1446
Branches 161 161
============================================
+ Hits 1332 1348 +16
+ Misses 68 54 -14
+ Partials 46 44 -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:
|
fc4464d to
a09ca64
Compare
Signed-off-by: jarebudev <23311805+jarebudev@users.noreply.github.com>
a09ca64 to
d89cbe1
Compare
|



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