Cache hits cause empty response content

[tobiashenkel]

After an upgrade of cachecontrol from 0.12.6 to 0.12.8 we experience empty response bodies on cache hits.

I've created a script that can reproduce the issue:

#!/usr/bin/env python3
import sys

import cachecontrol
import requests
from cachecontrol.cache import DictCache
from cachecontrol.heuristics import BaseHeuristic


class NoAgeHeuristic(BaseHeuristic):
    def update_headers(self, response):
        if "cache-control" in response.headers:
            del response.headers["cache-control"]


cache_adapter = cachecontrol.CacheControlAdapter(
    DictCache(), cache_etags=True, heuristic=NoAgeHeuristic()
)


session = requests.Session()
session.mount("https://", cache_adapter)

for i in range(2):
    response = session.get(
        "https://api.github.com/repos/sigmavirus24/github3.py/pulls/1033"
    )
    print(f'Content length: {len(response.content)}')
    if len(response.content) == 0:
        sys.exit(1)

When bisecting using the reproducer this issue seems to get introduced by add2e52

[awilkins]

This is causing python-poetry/poetry#4688 among others.

Have been debugging this in a container, currently I think the critical line is in update_cached_response()

Here we can see that cached_response.read() does not return data (body == b''), despite the response containing the cache data (see data, which is a live property, but there is a _data lower down)

This results in the headers being updated in the cache (and really just the timestamp here because the headers haven't changed), but the body gets set to an empty byte array.

My main thought is the only substantive changes in the above changeset are

• The .read() in update_cached_response() consumes the response stream where previously this didn't happen
• The removed chunk of dumps() in serialize.py no longer refreshes the stream after it's called from update_cached_response() (which no longer passes the default None for body)

Have mucked about trying to fix it but so far the best I can do is it not splatting the cached content - it still returns a response with content = b''

[ionrock]

@awilkins I have a branch that reverted the changes in that PR and added a test. I'm pretty sure the problem was that long ago we removed some code to allow resetting the internal file handle for the body content. Do you mind testing the branch to see if that solves it?

Your example script was working from what I could tell. Thanks!

[awilkins]

Yeah, this fixes it. Consumption of the stream in the response was the thing I was targetting and I also tried reverting this bit of the patch, but apparently I did it wrong (or wasn't clearing the disk-based cache in between or something, was testing this on a client in a container with a remote debugger attached after too much coffee).

Passes my test as well which was this, in test_adapter.py

    def test_get_cache_with_etag(self, url, sess):
        etag_url = urljoin(url, "/etag")
        r1 = sess.get(etag_url)
        assert r1.from_cache is False
        r2 = sess.get(etag_url)
        assert r2.from_cache is True
        assert r1.content == r2.content
        assert r2.content != b"

[awilkins]

Apologies if this is pushy, any chance of a release from this? A cache library that can't use ETagged resources is pretty critically broken.

[ionrock]

@awilkins No worries, thanks for the ping.

https://pypi.org/project/CacheControl/0.12.9/