Make threads daemons

[carrino]

#792

Don't keep the jvm alive if everything else has shut down. This is producing a regression on one of our internal services that fails to start and never shuts down so doesn't get restarted and just hangs.

[iamdanfox]

@carrino why not just revive that PR you linked and continue the discussion there?

The one case undesirable case I can think of is if a dying server wanted to kick off some kind of POST request, e.g. to ship some diagnostics somewhere or release some resources, a request might exist only on the Okhttp Dispatcher threads, which if they were daemon, would then just be dropped...

Other than that, it sounds like a reasonable safe change that shouldn't hurt Foundry services...

[carterkozak]

This sounds reasonable to me, I imagine our services are taking longer than necessary to stop while they wait the full timeout for idle threads to go away.

[carrino]

I think the case you laid out isn't really useful in practice. I think if I call post, i would wait for the result to ensure it's complete and not rely on the implementation detail that we are using threads under the covers.

[carterkozak]

In practice it could be helpful, you would enqueue an action which triggers a callback. If you're not blocking on a future, the application could exit. The problem is the daemon threads live a full minute while idle, and don't differentiate blocking shutdown between running vs idle.
That said, I think this should use daemon threads because we're not worried about that case when the server shuts down. Our clients tend to use futures to either batch awaiting results or simply wait on the future, not to asynchronously trigger execution.
Additionally spark requires us to block the main thread until we're done, in which case it doesn't matter at all whether we call these threads daemons or not, so this could not break anything that is not already broken on spark.

[carrino]

    /**
     * The {@link ScheduledExecutorService} used for scheduling call retries. This thread pool is distinct from OkHttp's
     * internal thread pool and from the thread pool used by {@link #executionExecutor}.
     * <p>
     * Note: In contrast to the {@link java.util.concurrent.ThreadPoolExecutor} used by OkHttp's {@link
     * #executionExecutor}, {@code corePoolSize} must not be zero for a {@link ScheduledThreadPoolExecutor}, see its
     * Javadoc.
     */
    private static final ScheduledExecutorService schedulingExecutor = Tracers.wrap(Executors.newScheduledThreadPool(
            NUM_SCHEDULING_THREADS, Util.threadFactory("conjure-java-runtime/OkHttp Scheduler", true)));

[carrino]

this never drops to 0

[henryptung]

Would add https://square.github.io/okhttp/3.x/okhttp/okhttp3/OkHttpClient.html docs about immediate shutdown, in particular ConnectionPool.evictAll() (probably to gracefully shut down idle connections with other servers rather than just vanishing).

[stale]

This issue has been automatically marked as stale because it has not been touched in the last 60 days. Please comment if you'd like to keep it open, otherwise it'll be closed in 7 days time.

[iamdanfox]

@carrino is this still important to you? I know we've built a bunch of bullet-proofing into Witchcraft so that shutdowns are guaranteed to finish within 2 minutes - perhaps that would help with your original use case?

[carterkozak]

I thought we fixed this?

[iamdanfox]

Me too, but I don't have tests that prove it (or a use-case to really motivate spending time writing those tests).