Okhttp: Must-revalidate does not revalidate

[PaulWoitaschek]

I have an i ssue where OkHttp does not revalidate requests. (3.10.0)
If the cache-control says "must-revalidate", the request should be revalidated.

In that case, the CacheStrategy should return a not null network request:

@Test
fun shouldRevalidate() {
  val nowMillis = 1527845183942 // =Fri Jun 01 2018 09:26:23
  val request = Request.Builder()
      .method("GET", null)
      .url("https://my.url.com/v1/item/d002fbef-7061-4d92-9958-7b128cf85855")
      .build()

  val handshake = Handshake.get(
      TlsVersion.TLS_1_2,
      CipherSuite.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, emptyList(), emptyList()
  )
  val cacheCandidate = Response.Builder()
      .code(200)
      .addHeader("date", "Fri, 01 Jun 2018 07:45:40 GMT")
      .addHeader("cache-control", "private, must-revalidate")
      .addHeader("last-modified", "Thu, 31 May 2018 08:04:53 GMT")
      .sentRequestAtMillis(1527839142145) //=Fri Jun 01 2018 07:45:42
      .receivedResponseAtMillis(1527839142295)  //=Fri Jun 01 2018 07:45:42 
      .request(request)
      .protocol(Protocol.HTTP_1_1)
      .handshake(handshake)
      .message("")
      .build()

  val cacheStrategy = CacheStrategy.Factory(nowMillis, request, cacheCandidate)
      .get()
  assertThat(cacheStrategy.networkRequest).isNotNull()
}

[yschimke]

One small note: OkHttpClient.Builder().build() without replacing an existing client isn't doing anything.

[dave-r12]

Could you build the test case with the OkHttpClient and MockWebServer?? Here's an example: https://github.com/square/okhttp/blob/master/okhttp-tests/src/test/java/okhttp3/CacheTest.java#L1204

[PaulWoitaschek]

Not sure this is the equivalent:

class Test {

  @JvmField
  @Rule
  val server = MockWebServer()

  private val cache = Cache(File("."), Long.MAX_VALUE)
  private val client = OkHttpClient.Builder()
      .cache(cache)
      .build()

  @Test
  fun test() {
    server.enqueue(
        MockResponse()
            .setBody("A")
            .addHeader("date", formatDate(400, TimeUnit.MILLISECONDS))
            .addHeader("cache-control", "private, must-revalidate")
            .addHeader("last-modified", formatDate(-1, TimeUnit.DAYS))
    )
    server.enqueue(MockResponse().setBody("B"))

    assertEquals("A", body(server.url("/")))
    Thread.sleep(1000)
    assertEquals("B", body(server.url("/")))
  }

  private fun body(url: HttpUrl): String? {
    val request = Request.Builder()
        .url(url)
        .build()
    return client.newCall(request)
        .execute()
        .body()
        ?.string()
  }

  private fun formatDate(millisDiff: Long, timeUnit: TimeUnit): String {
    val time = System.currentTimeMillis() + timeUnit.toMillis(millisDiff)
    val date = Date(time)
    return HttpDate.format(date)
  }
}

I think the problem here lies in the fact that the server time is slightly more in the future than the client time.

Notice the 1000ms sleep time - we are after the date sent from the server.

[PaulWoitaschek]

Btw it also fails without the sleep. I think if must-revalidate is present the revalidation must be made in every case.

[yschimke]

HttpDate is to second precision so millisecond offsets seem problematic especially with real times where millis may be > 600 and change results.

But I suspect you are right, care to implement a PR?

[PaulWoitaschek]

For an implementation: Would a shortcut that returns a null cacheResponse in getCandidate if the must-revalidate is present be sufficient?

[yschimke]

My understanding is that this flag means when cached and not stale, normal caching rules can be used. When cached but stale, clients and the cache can't override the stale checks and use the cache results. is that your understanding?

[swankjesse]

@PaulWoitaschek Unfortunately I think there’s ambiguity in what must-revalidate header means. My reading of the spec suggests that must-revalidate is short for _must-revalidate if the response is stale_ and not _must revalidate unconditionally_. By experiment Firefox and Safari agree with this interpretation, but Chrome does not.

5.2.2.1.  must-revalidate

   The "must-revalidate" response directive indicates that once it has
   become stale, a cache MUST NOT use the response to satisfy subsequent
   requests without successful validation on the origin server.

   The must-revalidate directive is necessary to support reliable
   operation for certain protocol features.  In all circumstances a
   cache MUST obey the must-revalidate directive; in particular, if a
   cache cannot reach the origin server for any reason, it MUST generate
   a 504 (Gateway Timeout) response.

   The must-revalidate directive ought to be used by servers if and only
   if failure to validate a request on the representation could result
   in incorrect operation, such as a silently unexecuted financial
   transaction.

This gist has a sample server that you can use to test out your own browsers.
https://gist.github.com/swankjesse/c04a0181da527ddb70b4dee49b804d13

I think if your server is configured to use must-revalidate when stale responses are unacceptable, that server is misconfigured and at least 2 major browsers will not do what you want. Instead you want a different header: max-age=0. This one does what I think you intend.

[PaulWoitaschek]

Okay thank you for the helpful response.

So you are saying no action is required?
Why is this marked for the 3.11 milestone?

[swankjesse]

Yup. I wanted to investigate this for 3.11 cause I'd expected us to need a code change.