-
Notifications
You must be signed in to change notification settings - Fork 102
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: release StatsDSender executor #115
Conversation
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.
Thank you very much for this fix, really appreciated! 🙇
It looks pretty much good to go, just having a thought about whether or not this should be a daemon thread.
@Override public Thread newThread(final Runnable runnable) { | ||
final Thread result = delegate.newThread(runnable); | ||
result.setName("StatsD-Sender-" + result.getName()); | ||
result.setDaemon(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.
I'm kind of torn about making this a daemon thread or not. On one hand this is an IO intensive thread, and we might also want to make sure it shuts down in orderly fashion, on the other hand this runs next to the application and one could argue shouldn't really be a user thread....
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.
I don't have a strong option about it as well. But I thought it might be a good idea to make it a daemon since com.timgroup.statsd.NonBlockingStatsDClient
is also creating daemon threads.
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.
Let's merge this as-is! Thank you very much @hanny24 🙇
@truthbk Are you planning to do a new release some time soon?, thanks. |
@prudhvi early next week! |
This PR fixes resource leak caused by ad47973. There is an executor in
StatsDSender
that was never been shutdown.