square / okhttp

Square’s meticulous HTTP client for the JVM, Android, and GraalVM.
https://square.github.io/okhttp/
Apache License 2.0
45.72k stars 9.15k forks source link

103 Early Hints leads to endless wait for headers #7936

Open stanislaw89 opened 1 year ago

stanislaw89 commented 1 year ago

Steps to reproduce.

Execute the code snipper below multiple times (it reproduces in ~ 1/4 of runs) using v4.11.0

fun main() {
  val client = OkHttpClient().newBuilder()
    .connectTimeout(5000, TimeUnit.MILLISECONDS)
    .readTimeout(5000, TimeUnit.MILLISECONDS)
    .writeTimeout(5000, TimeUnit.MILLISECONDS)
    .callTimeout(10000, TimeUnit.MILLISECONDS)
    .build()

  val request = Request.Builder()
    .url("https://theornateoracle.com/cart.php?action=add&product_id=456")
    .addHeader(
      "User-Agent", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) " +
      "AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36"
    )
    .cacheControl(CacheControl.FORCE_NETWORK)
    .get()
    .build()

  val call = client.newCall(request)
  println("Executing...")
  call.execute()
  println("Done")
}

The main thread hangs forever in waitForIo of Http2Stream#takeHeaders. I investigated a bit, and it looks like it only happens for 103 Early Hints response codes (inside the CallServerInterceptor#shouldIgnoreAndWaitForRealResponse if). Also, I figured out that the Http2Stream.StreamTimeout is called after some time, but it does not unblock the main thread.

yschimke commented 1 year ago

Yep - there is a race condition, and when it blocks we are treating them as trailers. I'll try to find the right fix.

yschimke commented 1 year ago

I could reproduce with 5.x, so if blocked use that until a release.

yschimke commented 1 year ago

I haven't reproduced on 5.x, and that server now doesn't return a 103.

I have some alternate servers, but I think this particular error relates to a 103 and then a response without a body.

https://early-hints.fastlylabs.com/test.png https://tradingstrategy.ai

So I don't have a good repro to confirm the fix.

yschimke commented 1 year ago

@swankjesse I think I understand why it's only happening when the response body is empty.

1) timeout - which calls closeInternal 2) closeInternal short circuits because source.finished == true. 3) takeHeaders loops another time, but now without a timeout

yschimke commented 11 months ago

Minimal fix in https://github.com/square/okhttp/pull/8054