Okhttp: HTTP/2 uses non-daemon threads

[Vampire]

The following one-line program does not close properly on Java 9:

public static void main(String[] args) throws IOException {
    System.out.println(new OkHttpClient.Builder().build().newCall(new Request.Builder().url("https://google.de").build()).execute());
}

It hangs for a very significant amount of time after the request is already finished before it allows the program to quit.

This a little modified version works flawlessly and allows the program to quit immediately after the request is finished.

public static void main(String[] args) throws IOException {
    System.out.println(new OkHttpClient.Builder().protocols(Collections.singletonList(Protocol.HTTP_1_1)).build().newCall(new Request.Builder().url("https://google.de").build()).execute());
}

Running on Java 8 also fixes the problem, as there HTTP 1.1 is used, not HTTP 2.0.

The problem is a hanging non-deamon thread called OkHttp google.de.
Here you have the stacktrace of the thread when the program should quit:

"OkHttp google.de@1876" prio=5 tid=0x11 nid=NA runnable
  java.lang.Thread.State: RUNNABLE
      at java.net.SocketInputStream.socketRead0(SocketInputStream.java:-1)
      at java.net.SocketInputStream.socketRead(SocketInputStream.java:116)
      at java.net.SocketInputStream.read(SocketInputStream.java:171)
      at java.net.SocketInputStream.read(SocketInputStream.java:141)
      at sun.security.ssl.SSLSocketInputRecord.read(SSLSocketInputRecord.java:425)
      at sun.security.ssl.SSLSocketInputRecord.bytesInCompletePacket(SSLSocketInputRecord.java:65)
      at sun.security.ssl.SSLSocketImpl.bytesInCompletePacket(SSLSocketImpl.java:918)
      - locked <0x792> (a java.lang.Object)
      at sun.security.ssl.AppInputStream.read(AppInputStream.java:144)
      - locked <0x793> (a sun.security.ssl.AppInputStream)
      at okio.Okio$2.read(Okio.java:140)
      at okio.AsyncTimeout$2.read(AsyncTimeout.java:237)
      at okio.RealBufferedSource.request(RealBufferedSource.java:68)
      at okio.RealBufferedSource.require(RealBufferedSource.java:61)
      at okhttp3.internal.http2.Http2Reader.nextFrame(Http2Reader.java:95)
      at okhttp3.internal.http2.Http2Connection$ReaderRunnable.execute(Http2Connection.java:608)
      at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
      at java.lang.Thread.run(Thread.java:844)

I find it quite strange that this thread is hanging on a socketRead operation, while the request is already finished successfully.

[JakeWharton]

Dupe of #4009?

[Vampire]

No, that's a different thread factory with a different name.
And while making the thread a daemon one might solve the issue that it prevents the program from closing, but isn't the real issue that it waits on a socket read while the request is already finished?

[dave-r12]

This is by design. For HTTP/2, a dedicated thread is created to read the incoming frames. That is the thread you see reading off the socket.

See the javadoc on OkHttpClient for shutting down resources: https://square.github.io/okhttp/3.x/okhttp/okhttp3/OkHttpClient.html.

[Vampire]

That doc you linked talks about daemon threads. If it would be a daemon thread, it would not prevent the JVM from exiting.
If the socket read is by design, then maybe the thread should be daemon?

[Vampire]

Actually you should probably only ever use daemon threads in the whole library. I don't think there is any use-case to block JVM exit from OkHttp.

[Vampire]

And even if I do the cleanup suggested in what you linked @dave-r12, the behavior didn't change, because of new Thread(readerRunnable).start(); // Not a daemon thread. that creates that non-daemon thread that prevents the JVM from shutting down. while the docs you linked say OkHttp also uses daemon threads for HTTP/2 connections. These will exit automatically if they remain idle. which is not true in this case.

[yschimke]

Ultimately, I think what you are hitting is a fundamental part of the design

https://github.com/square/okhttp/issues/1890

Unlike the connection pool, the dispatcher isn't a natural daemon. A simple main program that makes an async request should complete its request!

So there are cases that would merit a bug fix, but assuming a well written program that completes or cancels requests and does proper cleanup should shutdown in a timely manner. n.b. I've used OkHttp extensively in command line scripting and haven't had an issue with shutdown. It feels like you want OkHttp to never block your shutdown no matter how it's used, while part of the design is that it should complete valid work, not corrupt cache directories when possible etc.

That readerRunnable thread should complete very quickly after a clean shutdown. If you can get it to block shutdown even after you cleanly close OkHttp resources, we should definitely fix that.

[Vampire]

@yschimke would you mind telling me how?
In my understanding the following code does not do an async request and should be correct, request is finished, as execute() executes is sync, the result is printed and then the program hangs with that thread for quite some time.

public static void main(String[] args) throws IOException {
    System.out.println(new OkHttpClient.Builder().build().newCall(new Request.Builder().url("https://google.de").build()).execute());
}

In response to daves comment I also tried

public static void main(String[] args) throws IOException {
    OkHttpClient okHttpClient = new OkHttpClient.Builder().build();
    System.out.println(okHttpClient.newCall(new Request.Builder().url("https://google.de").build()).execute());
    okHttpClient.dispatcher().executorService().shutdown();
    okHttpClient.connectionPool().evictAll();
}

which has the same behavior.

I agree that a started request should be finished fully or canceled by the consumer code for an orderly shutdown, but my code should fulfill that, shouldn't it?

[yschimke]

This program shuts down immediately with Java 9. The difference is cleanly closing the response also.

import java.io.IOException;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;

public class X {
  public static void main(String[] args) throws IOException {
    OkHttpClient okHttpClient = new OkHttpClient.Builder().build();
    Response response =
        okHttpClient.newCall(new Request.Builder().url("https://google.de").build()).execute();
    System.out.println(response);
    response.close();
    okHttpClient.dispatcher().executorService().shutdown();
    okHttpClient.connectionPool().evictAll();
  }
}

[Vampire]

Ah, ok, forgot to close the response, yes.
But even with the response closed like

OkHttpClient okHttpClient = new OkHttpClient.Builder().build();
try(Response response = okHttpClient.newCall(new Request.Builder().url("https://google.de").build()).execute()) {
    System.out.println(response);
}

The JVM does not exit.
I have to manually do

OkHttpClient okHttpClient = new OkHttpClient.Builder().build();
try(Response response = okHttpClient.newCall(new Request.Builder().url("https://google.de").build()).execute()) {
    System.out.println(response);
}
okHttpClient.connectionPool().evictAll();

To get it to close properly.
Why do you need to evict the connection pool manually?
This feels somewhat awkward.
Shouldn't the first code have a state of it should complete valid work already?

[yschimke]

Well, I'm glad I could help you with closing the sync response. I'll leave the debate about the connection pool to others.

[Vampire]

Yeah, in our real code the response was closed properly by using a try-with-resources block, that was just faulty test code :-)

[Vampire]

When having a long running program where you don't explicitly end the program it is even worse, because you don't know when you have to shutdown OkHttp manually.

[yschimke]

Maybe the correct fix is never calling new Thread(...), and we always use a ThreadPoolExecutor e.g. from the Dispatcher which ultimately could be overridden by a motivated enough client. But the defaults left as is.

[Vampire]

I still wonder why by default a non-daemon thread should block on a socket read while the request is properly finished and its response closed.
This doesn't sound like a sensible default to do.

[JakeWharton]

What if there's a server push or control frame sent?

On Tue, May 29, 2018, 4:43 AM Björn Kautler [e-mail]
wrote:

I still wonder why by default a non-daemon thread should block on a socket
read while the request is properly finished and its response closed.
This doesn't sound like a sensible default to do.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
https://github.com/square/okhttp/issues/4029#issuecomment-392699656, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEEclGMh8ohXWmYxbIylQfVK1iQAzKks5t3Qo_gaJpZM4UPEoy
.

[Vampire]

@JakeWharton I'm not familiar with HTTP 2.0 specifics, but is this relevant?
I do a synchronous request to the server.
I close the response.
And then the program should end.

As I said,

• it is probably ok that the actual request that is triggered by client code is done in a non-daemon thread as it should be properly finished
• it might also be ok to have the socket read waiting for whatever if that is how HTTP 2.0 works
• but that waiting should probably be done in a daemon thread instead

Also, what does happen if a server push or something else is sent but the client code is not interested. Does anything actually happen with the data? Maybe the socket read should only be done in a non-daemon thread or done at all if there is actually some consumer for the information?

[JakeWharton]

Server pushes and control frames are not surfaced at the application layer
and are handled entirely inside the client.

On Tue, May 29, 2018 at 9:09 AM Björn Kautler [e-mail]
wrote:

@JakeWharton https://github.com/JakeWharton I'm not familiar with HTTP
2.0 specifics, but is this relevant?
I do a synchronous request to the server.
I close the response.
And then the program should end.

As I said,

• it is probably ok that the actual request that is triggered by
  client code is done in a non-daemon thread as it should be properly finished
• it might also be ok to have the socket read waiting for whatever if
  that is how HTTP 2.0 works
• but that waiting should probably be done in a daemon thread instead

Also, what does happen if a server push or something else is sent but the
client code is not interested. Does anything actually happen with the data?
Maybe the socket read should only be done in a non-daemon thread or done at
all if there is actually some consumer for the information?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/square/okhttp/issues/4029#issuecomment-392769508, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEEUW62ljHU_i-fPY6pSHRHGwAPOS1ks5t3UiAgaJpZM4UPEoy
.

[Vampire]

But should they prevent orderly shutdown of the application?
For waiting whether there might eventually be some server push or control frame?
If you manually evict, they are also not waited for any longer.
Wouldn't it be enough to wait for their eventually arrival on a daemon thread?

[567legodude]

So I'm the person who encountered this issue and showed it to Vampire. In my use case, the program has an explicit close where I want to end everything, but this thread in question stays afterwards and I am having to manually terminate the process to use the file again.

[yschimke]

@567legodude So that should be working as shown in the example program I gave above. If you close all responses cleanly as part of processing them, and then shutdown the client (dispatcher and connection pool). There shouldn't be any threads left hanging around in that case.

[567legodude]

I don't interface with okhttp directly. A library I'm using creates the connections.

[Vampire]

And that library now does the close explicitly now too as soon as the PR is merged, as it also has a clean exit point @567legodude. The point is, that in many other situations there is not such a clear exit point, like e. g. in my own Bot, so there I would need to forbid HTTP 2.0 or use a different http library than the library I use brings in anyway.
The point is, that a simple three-liner that does the request, outputs the result and closes the result should cleanly exit immediately, without the need to manually evict socket-reading threads.

[swankjesse]

Unfortunately I don’t think we can change a thread from daemon to non-daemon at runtime. We’d really need to do that to make HTTP/2 work reasonably. Otherwise a program that’s awaiting an async response would exit prematurely.

Another option is to migrate our HTTP/2 work to NIO, which changes the performance tradeoffs: higher latency but more concurrency.

[yschimke]

Which is why I suggested never using raw threads. Ultimately any motivated developer could override to get the behaviour they want.

[Vampire]

@swankjesse no you cannot change daemonness after the thread is started.

You would needed to either start a non-daemon thread to do the request blocking and then start a new daemon thread (or better one from a thread pool), doing the waiting non-blocking.

Or maybe if it is easier, or if both things need to be done in the same thread, do both in one daemon thread, but at the beginning start a second non-daemon thread. This one would simply wait for a semaphore and die as soon as it toggles and will be toggled as soon as the actual request is finished. It's sole purpose would be to prevent premature exiting of the JVM.

[Vampire]

@yschimke of course you can always make the code what you want, at worst with some bytecode rewriting or forking the project.

But I'd prefer the lib to behave nicely by default, as all users would benefit from that. ;-)

[yschimke]

@Vampire the discussion is going around in circles. It seems like you really don't believe that the behaviour is by design and intended to be a sensible default that works for many different client uses. I hope it's clear that I can understand why this behaviour isn't great for you and not ignoring you. I'm trying to suggest a way that we could allow you to cleanly override the defaults and take on responsibility for any non-standard behaviour e.g. cache corruption in your app.

But I personally find the discussion not very productive anymore and will leave you all to it.

[Vampire]

@yschimke

discussion not very productive anymore

Why not?
Reasonings are told, suggestions are made.
Possibilities are discussed.
I think it still is productive.

and tone a bit sarcastic

If you had this feeling I'm sorry, there was no bit of sarcasm in my post, I meant what I wrote as I wrote it.

It seems like you really don't believe that the behaviour is by design and intended to be a sensible default that works for many different client uses.

Sure I believe that, but also designs can have bugs / flaws.
And unless you want to make it hard for your users, it should be rethought whether this could be improved.
Even the docs say otherwise. They say "with these three calls you can clear up some resources, but you don't need to" and "for HTTP 2.0 there are some daemon threads started".
Nothing suggests that non-daemon threads are started that you have to manuall cancel to get a clean shutdown.
And even if it would be documented, it would still be a design flaw in my eyes, as a program that does one request synchronously and properly closes the response should not need any further action to cleanly shutdown.

I'm trying to suggest a way that we could allow you to cleanly override the defaults and take on responsibility for any non-standard behaviour e.g. cache corruption in your app.

Thanks, but wouldn't it be nicer to have a good behavior that will _not_ corrupt caches?
Also if with overriding the defaults you mean something like making the daemonness configurable, this would help in my situation as I only do synchronous requests, but maybe not for people that do asynchronous requests, which I will probably also switch to now that I know that OkHttp can do it.

Actually there are already three productive suggestions for a solution here, the one @swankjesse suggested and the two I suggested in response.

[yschimke]

This was pre-coffee, so maybe I was too sensitive. I’ll leave you all to it.

[swankjesse]

We can attempt some refactoring internal to Http2Connection to make the reader a daemon. We’ll need to confirm that when we do that, there’s also an application thread so that a program doesn’t exit while it’s waiting for a callback. And it might break if we later change policy on how threads map to sockets.

The daemon / non-daemon APIs are not very helpful here. It’s particularly awkward to unit test that the program won’t exit prematurely, for example.

[Vampire]

We’ll need to confirm that when we do that, there’s also an application thread so that a program doesn’t exit while it’s waiting for a callback.

You don't like my suggested approaches?
Checking and relying on the consumer having an application thread is flaky, as the consumers non-daemon application thread could die every time between checking and end of request.
You could simply make this documented behavior, that the requests are done in daemon threads and the consuming application has to take care that the application does not prematurely end before the request was handled.
Or you could block for example with one of the approaches I suggested.(?)

[magneticflux-]

I experienced this issue while using Retrofit on a command-line application. The workaround I used at the end of my program is:

    // ... Do stuff, injected with Guice
    // Eventually, I have to block to get results of my queries
    val client = factory.injector.getInstance(classOf[OkHttpClient])
    client.dispatcher().executorService().shutdown()
    // End of the entire program

which is a bit clunky. I'd rather just require blocking for async methods that must complete, since that seems more logical to me from a client-side standpoint and also just seems like better practice in general.

Just from skimming the HTTP/2 spec, would it be possible to make an HTTP/2 call only complete when it reaches the "closed" or even just "half-closed (local)" state here? That would mean that clients would have to wait for all HTTP/2 connections to be closed before exiting their program to guarantee that the connections were successful, which makes sense to me.

[swankjesse]

@magneticflux- one thing that makes this difficult is that we can’t turn a thread from daemon to non-daemon or vice-versa.

[magneticflux-]

@swankjesse I guess I'm confused by why they have to be non-daemon in the first place. Is it only to support making async calls that are guaranteed to complete before JVM shutdown without having to block at the end of the program?

[Saladoc]

I have also run into this problem. Maybe there could just be a method OkHttpClient#shutdown that does all the internal cleanup and also interrupts any current threads? Or shutdown and shutdownNow?

I get that the threads not being daemon threads is a design decision and don't feel versed enough with the matter at hand to argue whether it's a good decision or not. But I want a way to stop the client without worrying to much about its internal workings. Simply shutting down the ExecutorService won't do any good as that will not stop currently running threads so it is not feasible as a workaround.

[vadzim-marchanka]

I have the same issue. I do not use this library directly but it is used by another one that I use. It is not clear how I need to stop this threads.

[m190]

I am facing the same issue. Are there official guide how to avoid it? Thanks

Updated:
I've looked at OkHttpClient Javadoc, and from mentioned shutdown hints, this one seems to be working for me: client.connectionPool().evictAll();

[Lewuathe]

We have seen the same issue. Our workaround is to set HTTP 1.1 explicitly in the case when HTTP 2 is not mandatory.

OkHttpClient.Builder builder = new OkHttpClient.Builder();
builder.protocols(Arrays.asList(Protocol.HTTP_1_1));

[fastily]

I use okhttp for jwiki under the hood, and I don't allow clients to interact with the underlying OkHttpClient.

I've worked around this hanging issue the same way as @Lewuathe, forcing OkHttpClient to use HTTP 1.1. Understood that the HTTP/2 implementation is working as designed, but it would be nice to be able to set a flag when building an OkHttpClient that retains the old behavior.

[peterbecker]

I'm running into this one as well. May I point out that the documentation actually explicitly claims the opposite behaviour:

OkHttp also uses daemon threads for HTTP/2 connections. These will exit automatically if they remain idle.

[swankjesse]

We built a new TaskRunner abstraction that will allow HTTP/2 threads to be daemons in 4.3.

[swankjesse]

Fixed.

[slagathor2094]

I'm still experiencing hanging threads in Java 11. I'm using version 4.4.0.

Is anyone else still seeing this issue?

[Lewuathe]

That's true for my case too. Even after we start using okhttp 4.3.0, the client still hangs with Java 11.
See: https://github.com/prestosql/presto/issues/1169#issuecomment-570786824

[yschimke]

Will be fixed in 4.5.

[jgangemi]

will this be backported to 3.x?

[swankjesse]

No.