Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible bug when writing response to the disk cache in case of HTTP_NOT_MODIFIED, when only metadata is updated #1256

Closed
casaflowa opened this issue May 4, 2022 · 1 comment · Fixed by #1261

Comments

@casaflowa
Copy link

casaflowa commented May 4, 2022

Describe the bug
This is the code snipped in question (HttpUriFetcher):

// Write the response to the disk cache.
if (response.code == HTTP_NOT_MODIFIED && cacheResponse != null) {
    // Only update the metadata.
    val combinedResponse = response.newBuilder()
        .headers(combineHeaders(CacheResponse(response).responseHeaders, response.headers))
        .build()
    fileSystem.write(editor.metadata) {
        CacheResponse(combinedResponse).writeTo(this)
    }
}

and in particular this line: .headers(combineHeaders(CacheResponse(response).responseHeaders, response.headers))
From what I see are CacheResponse(response).responseHeaders and response.headers referencing the same object.
I assume cacheResponse.responseHeaders and response.headers should be combined instead.

Changing cacheResponse.responseHeaders and response.headers would probably fix my problem too, which depends on loading images from disk, but first checks if the image has been updated by asking the server (must-revalidate / etag).

I found this issue, because in my case cacheResponse holds the info about cache-control = must-revalidate (which is already stored in the metadata), which gets lost when metadata is overwritten, since the response (with code HTTP_NOT_MODIFIED) doesn't contain the cache-control headers anymore.
=> My temporary fix is adding this cache-control headers to the response of the must-revalidate-Request.

Version
io.coil-kt:coil-base:2.0.0-rc02

@colinrtwhite
Copy link
Member

Great catch! Thanks for the report. Fixed here: #1261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants