[fix] ScheduledExecutors use daemon threads, to avoid blocking shutdowns #989
Conversation
| @@ -114,7 +114,7 @@ | |||
| * Javadoc. | |||
| */ | |||
| private static final ScheduledExecutorService schedulingExecutor = Tracers.wrap(Executors.newScheduledThreadPool( | |||
| NUM_SCHEDULING_THREADS, Util.threadFactory("conjure-java-runtime/OkHttp Scheduler", false))); | |||
| NUM_SCHEDULING_THREADS, Util.threadFactory("conjure-java-runtime/OkHttp Scheduler", true))); | |||
There was a problem hiding this comment.
there are other executors in this class; you probably want to daemonize all of them!
There was a problem hiding this comment.
maybe, but this is the one that is causing me troubles
There was a problem hiding this comment.
Please either update all executors, or add comments describing why they need to use non-daemon threads
There was a problem hiding this comment.
Why doesn't this apply to the cached executor which also uses non-daemon threads? If anything has made a remote call I imagine that would block shutdown for a full minute as well even if actions are not running.
If we haven't made calls using conjure-java-runtiem clients, it's odd that we would create this executor. Should creation of these executors be deferred until a client is created using memoized suppliers?
|
Can you please update the title and message describing what has changed for a better commit log. |
carrino
force-pushed
the
bugfix/daemon-threads
branch
from
b016359 to
b789d38
Compare
|
comment added |
|
I think we've now accounted for all the executors constructed in this library.
|
iamdanfox
changed the title
iamdanfox
changed the title
|
👍 |
Before this PR
This library leaves non-daemon threads behind so services never shut down
After this PR
==COMMIT_MSG==
ScheduledExecutors use daemon threads, so the JVM isn't prevented from shutting down
==COMMIT_MSG==
Possible downsides?
Very minimal downsides for uses who fire and forget requests and shut down their server and expect the library to keep the JVM alive and retry under the covers.