Skip to content
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

Daemonize OkHttp dispatcher threads. #792

Closed
wants to merge 1 commit into from
Closed

Daemonize OkHttp dispatcher threads. #792

wants to merge 1 commit into from

Conversation

GrahamDennis
Copy link
Contributor

No description provided.

@GrahamDennis GrahamDennis requested a review from a team as a code owner August 24, 2018 01:54
@uschi2000
Copy link
Contributor

What's the rationale here, @GrahamDennis ?

@GrahamDennis
Copy link
Contributor Author

What's happening is that the CachedThreadPool can cache the threads for 60s after they've last been used. So the threads can delay termination of a process by up to 60s if the process doesn't call System.exit.

My rationale for this change is that even if an OkHttp client is using one of these threads, a non-daemonized thread should be blocking on the response. But I suppose if you're instead only being notified when the response comes back, then these threads could be feasibly be the only non-daemonized threads running, in which case this would be the wrong change to make.

@uschi2000
Copy link
Contributor

uschi2000 commented Aug 27, 2018 via email

@GrahamDennis
Copy link
Contributor Author

You're right that it doesn't matter either way for servers... However I've been using this in some CLIs. It's for the benefit of a CLI that I initially wanted to make this change.

However, I've since realised that what I should be doing is calling System.exit when my CLI is done instead of relying on the JVM to clean up when all non-daemon threads are complete. So I think we can close this out as I don't need it anymore. And I'm actually not sure that my proposed change is correct, because if you were running a CLI and were asynchronously listening for a HTTP response, the OkHttp thread could be the only non-daemonised thread, and I think we would not want the JVM to terminate in that situation.

@dansanduleac
Copy link
Contributor

FWIW, the Dispatcher's default ExecutorService implementation also creates non-daemon threads, so I just copied that when creating this executionExecutor.

@carrino carrino mentioned this pull request Mar 19, 2019
@carterkozak carterkozak reopened this Mar 19, 2019
@carterkozak
Copy link
Contributor

lots of conflicts, we should remake this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants