Cache returns stale response body data on when ETag is the same (Windows only)

[bitwiseman]

Environment

OkHttp 4.2.2
Only happens on Windows. On linux and MacOS, the correct (expected) behavior occurs.

Description

On Windows, the OkHttp Cache does not update response body data for a request when a later response has the same ETag as a previous response.

Steps

1. Request a URL /a/b
   • Response Header: 404, no ETag
   • Response Body: '{ "message": "Not Found" }'
   • Cache Updated
2. Request a URL /a/b with "If-Modified-Since" header
   • Response Header: 304, ETag "C"
   • Cache automatically converts - client sees:
     • Response Header: 404, ETag "C"
     • Response Body: '{ "message": "Not Found" }'
3. Request a URL /a/b with "Cache-Control: no-cache" to force retrieval of new data
   • Response Header: 200, ETag "C"
   • Response Body: '{ data: "..." }'
   • NOTE: Yes, that is the same ETag with different data - not good, but cache should still work
   • Cache Updated? (I think this is where the problem starts).
4. Request a URL /a/b with "If-None-Match" equal to ETag "C"

Expected Result

• Response Header: 304, ETag "C"
• Cache automatically converts - client sees:
  • Response Header: 200, ETag "C"
  • Response Body: '{ data: "..." }'

Actual Result

• Response Header: 304, ETag "C"
• Cache automatically converts - client sees:
  • Response Header: 200, ETag "C"
  • Response Body: '{ "message": "Not Found" }'

Automated Test

I know it is not great to point to a test on an external project but the test fully automated and will repro the issue every time:
https://github.com/github-api/github-api/blob/967512629821b706e8d06ef9b3e57061f397d073/src/test/java/org/kohsuke/github/extras/okhttp3/GitHubCachingTest.java#L59

To run automated repro:

1. clone the repository on Windows with JDK 8+
2. remove the assumeFalse() call
3. run mvn install

Notes

This issue was found and can be reproduced using https://github.com/github-api/github-api .
It is exposed due to a bug in GitHub server responses (see hub4j/github-api#669) but it is still a bug in this OkHttp Cache behavior on Windows.

[swankjesse]

The bug is that in step 3 the cache should be updated with the new response, but it is not being updated?

[bitwiseman]

@swankjesse
Yes, I believe the bug is in step 3.
In step 4 the (correct) updated response header from step 3 is returned with the (incorrect) outdated response body from step step 1.
So, the bug is that the cache is only partially updated in step 3.

[swankjesse]

Can you send a PR with a failing test? Copy-pasting this test is a great place to start:

https://github.com/square/okhttp/blob/master/okhttp/src/test/java/okhttp3/CacheTest.java#L981

[swankjesse]

Thanks for your help @bitwiseman! I’ve set up VirtualBox and Windows 10 to reproduce locally. The sad news is we’ve designed OkHttp’s cache with assumptions of POSIX file systems, and Windows doesn’t satisfy these assumptions.

In particular:

• We can’t delete a file that has active readers. This happens when a cache write updates a file that’s still being written.
• We can’t atomically rename a file on top of an existing file. This happens when a cache write updates an existing cache entry. It may also happen with the journal.

I think the easiest solution is to recover (more) gracefully when Windows’ file systems prevent us from doing what we can do reliably on POSIX. We won’t be able update the cache when it’s actively being read, though we should be able to invalidate the stale entry to-be-replaced.

I’d like to start by figuring out exactly which operations in our test suite Windows rejects, and adding tests that use our FaultyFileSystem to synthesize these. That way we can quickly iterate on Windows’ behavior without having to drag out a Windows VM or PC. We’ll also need to keep CI on Windows so we can be sure we aren’t missing anything.

I’m surprised this project has gone so long with such problems in our caching on Windows. Fixing that will be a worth focusing on over the next few weeks.

[bitwiseman]

@swankjesse
Thanks! Sorry, I wasn't able to get a test working for this. Thanks for getting the local repro.