Cache entries are not stored correctly on Windows, resulting in excessive requests

[Nohus]

Scenario

A server returns a resource that can be cached for 50 seconds, and an application requests it 100 times, once every second.

Expectation

1. First request is sent, server returns 200, OkHttp stores the response in the cache.
2. 49 next requests are served from the cache.
3. Cache entry has expired, OkHttp sends a conditional request. Server responds with a 304 Not Modified. OkHttp updates the cache entry.
4. 49 next requests are served from the cache.

Total requests sent to the network: 2.

This correctly happens on Linux and macOS.

The bug

Here is what happens on Windows:

1. First request is sent, server returns 200, OkHttp stores the response in the cache.
2. 49 next requests are served from the cache.
3. Cache entry has expired, OkHttp sends a conditional request. Server responds with a 304 Not Modified. OkHttp fails to update the cache entry.
4. OkHttp sends 49 conditional requests, never succeeds in updating the cache and keeps the stale response in the cache forever, resulting in needing a network request every time going forward.

Total requests sent to the network: 51

Reproducer

The below test succeeds on Linux but fails on Windows.

import okhttp3.Cache
import okhttp3.OkHttpClient
import okhttp3.Request
import java.io.File
import java.io.IOException
import java.net.HttpURLConnection
import java.text.DateFormat
import java.text.SimpleDateFormat
import java.util.Date
import java.util.Locale
import java.util.TimeZone
import java.util.concurrent.TimeUnit
import mockwebserver3.MockResponse
import mockwebserver3.MockWebServer

fun main() {
  val server = MockWebServer()
  server.start()
  // First response, will return a 200 and immediately expire
  server.enqueue(
    MockResponse
      .Builder()
      .addHeader("Last-Modified: " + formatDate(0, TimeUnit.SECONDS))
      .addHeader("Cache-Control: max-age=0")
      .body("A")
      .build(),
  )
  // Second response, will return a 304 and set a new longer age
  server.enqueue(
    MockResponse
      .Builder()
      .addHeader("Cache-Control: max-age=30")
      .addHeader("Allow: GET, HEAD")
      .code(HttpURLConnection.HTTP_NOT_MODIFIED)
      .build(),
  )
  // Third response, should never be requested
  server.enqueue(
    MockResponse
      .Builder()
      .body("B")
      .build(),
  )

  val cacheDirectory = File("cache-test")
  cacheDirectory.deleteRecursively()
  val client = OkHttpClient.Builder()
    .cache(Cache(File("cache-test"), 50 * 1024 * 1024))
    .build()

  // Test
  repeat(3) {
    val request = Request.Builder()
      .url(server.url("/a"))
      .build()
    client.newCall(request).execute().use { response ->
      if (!response.isSuccessful) throw IOException("Unexpected code $response")
      if (response.body.string() != "A") throw IOException("Cache wasn't used correctly")
      println("Success")
    }
  }

  server.close()
}

private fun formatDate(
  delta: Long,
  timeUnit: TimeUnit,
): String = formatDate(Date(System.currentTimeMillis() + timeUnit.toMillis(delta)))

private fun formatDate(date: Date): String {
  val rfc1123: DateFormat = SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss zzz", Locale.US)
  rfc1123.timeZone = TimeZone.getTimeZone("GMT")
  return rfc1123.format(date)
}

Cause

CacheInterceptor calls cache.update() when receiving a 304
Cache calls snapshot.edit()
Snapshot calls DiskLruCache.edit()

That edit method reaches:

if (entry != null && entry.lockingSourceCount != 0) {
      return null // We can't write this file because a reader is still reading it.
}

Returns null and the update fails. That condition only evaluates to true on Windows.

[Nohus]

Looks like existing unit tests in CacheTest catch this, if you replace fileSystem.emulateUnix() with fileSystem.emulateWindows() in the setUp() method.

[yschimke]

Adding a test in #9339