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

Move the hook for reading response data into the cache up one level. #102

Closed
wants to merge 1 commit into from

Conversation

toolforger
Copy link
Contributor

Old code used to hook the HTTPResponse._fp object, on the code path that deals
just with non-chunked responses. Since _fp isn't responsible for detecting the
end of chunked responses, it cannot know when to create the cache entry.

New code hooks the HTTPResponse itself, on the stream() function, that's the
common code path for chunked and non-chunked responses.

Fixes #95.

Old code used to hook the HTTPResponse._fp object, on the code path that deals
just with non-chunked responses. Since _fp isn't responsible for detecting the
end of chunked responses, it cannot know when to create the cache entry.

New code hooks the HTTPResponse itself, on the stream() function, that's the
common code path for chunked and non-chunked responses.

Fixes psf#95.
@toolforger
Copy link
Contributor Author

I'm willing to add tests, but I don't know the best way to do that.
Adding a GitHub URL would work for now, but that will stop testing what it is expected to test as soon as GitHub changes from chunked to non-chunked responses. I guess a Mock would be better (I'd need some instructions how to best build such a mock though).
Also, I don't know which tests should be repeated for chunked and for nonchunked responses. I'm pretty sure the existing test suite could benefit from that.

@JustinAzoff
Copy link

I tried this out, but it looks like it breaks things somehow. Things are being cached, and controller.cache_response looks like it is being called correctly. buf.getvalue() that gets passed to cache_response is returning the expected content. but then on the second request, this happens:

   return resp.json()
 File "/../env/lib/python2.7/site-packages/requests/models.py", line 788, in json
    if not self.encoding and len(self.content) > 3:
TypeError: object of type 'NoneType' has no len()

@jaraco
Copy link
Contributor

jaraco commented Mar 16, 2016

I can confirm Justin's finding. After applying 8628678, Response.content is None for cached responses, which causes requests for .json() to fail also:

Traceback (most recent call last):
  File "check-cache.py", line 39, in <module>
    do_req(2)
  File "check-cache.py", line 32, in do_req
    print("Got value", res.json())
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages/requests-2.9.1-py3.5.egg/requests/models.py", line 808, in json
    return complexjson.loads(self.text, **kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/json/__init__.py", line 319, in loads
    return _default_decoder.decode(s)
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/json/decoder.py", line 339, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/json/decoder.py", line 357, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

@ionrock
Copy link
Contributor

ionrock commented Mar 23, 2016

I've gone with #106 with a couple small changes. Closing this one out.

@ionrock ionrock closed this Mar 23, 2016
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 this pull request may close these issues.

None yet

4 participants