Repository navigation
The web socket ping thread should be a daemon thread #3339
Description
What kind of issue is this?
- Bug report. If you’ve found a bug, spend the time to write a failing test. Bugs with tests
get fixed. Here’s an example: https://gist.github.com/swankjesse/981fcae102f513eb13ed
Our project apache-spark-on-k8s/spark uses fabric8io/kubernetes-client, which in turn uses okhttp. There was an edge case where our main thread got an exception. We expected the JVM process terminate properly. But to our surprise, it just hung indefinitely because OkHttp WebSocket thread was not a daemon thread. This seems like a bug?
"OkHttp WebSocket https://kubernetes.default.svc/..." #51 prio=5 os_prio=0 tid=0x00007f1cd401e000 nid=0x63 waiting on condition [0x00007f1cafbaa000]
Note the main thread code is outside our control and we can't add try catch block and close the client explicitly. For more details, please see apache-spark-on-k8s/spark#216 comment. To work around this, we had to disable ping by setting the ping period to zero.
I believe the code that sets up this thread pool is at RealWebSocket.java:
public void initReaderAndWriter(
String name, long pingIntervalMillis, Streams streams) throws IOException {
synchronized (this) {
this.streams = streams;
this.writer = new WebSocketWriter(streams.client, streams.sink, random);
this.executor = new ScheduledThreadPoolExecutor(1, Util.threadFactory(name, false));
if (pingIntervalMillis != 0) {
executor.scheduleAtFixedRate(
new PingRunnable(), pingIntervalMillis, pingIntervalMillis, MILLISECONDS);
}
if (!messageAndCloseQueue.isEmpty()) {
runWriter(); // Send messages that were enqueued before we were connected.
}
}
Notice it is calling Util.threadFactory(name, false). The second parameter false sets a new thread to be non-daemon.
This also seems a regression. Before this commit, it used to set the thread to be a daemon by passing true to Util.threadFactory. Please see below.
- /** A shared executor for all web sockets. */
- private static final ExecutorService executor = new ThreadPoolExecutor(0,
- Integer.MAX_VALUE, 60, SECONDS, new SynchronousQueue<Runnable>(),
- Util.threadFactory("OkHttp WebSocket", true));
-
Thanks. And please let me know what you think.
Activity
swankjesse commentedon May 10, 2017
Imagine a simple console application that starts a websocket, connects to a service, and prints the incoming messages. If the websocket here used a daemon thread, the program would immediately exit!
I think a better solution is to cancel the websocket when it is no longer wanted.
kimoonkim commentedon May 10, 2017
Thanks for the quick reply.
We would do so if we can. But our Spark code is a generic framework that user app calls as a library. And we started using okhttp inside the framework code.
The user app code is outside our control. And it's the main thread of user app that sees an exception. We can't intercept when this happens and cancel the websocket because again we can't modify the user code, like adding 'try catch'. To matters worse, this Spark framework has been in use by many existing apps and we are just porting the existing framework to a different platform, so we can't change the contract between this framework and app code and ask apps to change their code.
I see that's a legit use case. Then maybe we can make this daemon/non-daemon choice optional? Or maybe allow a custom executor to be used? We don't mind the default choice to be non-daemon as long as there is an option to override.
swankjesse commentedon May 11, 2017
Triggering behavior based on daemon/non-daemon threads suggests you’re going to run into other problems.
For example, with HTTP/2 we also need a non-daemon thread to consume incoming response bodies and cache pushes.
Have you considered using an UncaughtExceptionHandler? That might be a lot more reliable.
kimoonkim commentedon May 11, 2017
Yes, we were chasing down other non-daemon threads too. Good to know the HTTP/2 behavior. I am glad I asked :-)
@ash211 @foxish @mccheah
Ah, thanks for the suggestion. Maybe that's the right way to deal with this issue. I see our project uses UncaughtExceptionHandler in other places. Maybe we can use it also where we interact with okhttp.
swankjesse commentedon Jun 11, 2017
No further action to take on this.