Add http cache support to Agent

[chris-]

The web.Agent should support satisfying requests from a (local) cache using the HTTP caching headers. This patch starts adding support for this using the etag / last-modified / if-modified-since / if-match / if-none-match headers. I intend to add features /headers as needed, but these should cover the most useful.

I would appreciate some pointers.

[therve]

(In [32010]) Branching to 'caching-agent-5126'

[therve]

(In [32011]) Apply patch

Refs #5126

[therve]

(In [32012]) Cosmetics

Refs #5126

[itamar]

The current cache API is problematic, since it assumes cached content will fit in memory, or even won't use a lot of memory. What happens when you get cache headers for a 500MB file?

[therve]

Thanks a lot for the patch, it looks very promising. Meta-comment: there were a lot of small aesthetic changes to do: please read the coding standard more carefully, I won't do the changes myself the next time :).

1. All your test methods need docstrings.

2. As dumb as it is, you should have some specific testing for MemoryCache.

3. I've been wondering if the methods of IHTTPCache shouldn't return Deferred instead. It would allow to integrate easily with the rest of Twisted protocols (Memcache for example).

4. The call to delete the cache is not strictly necessary, as you call put just after.

5. _CachingProtocol should use a list or a StringIO for buffer, not a string.

6. It feels that cache entries should be more structured than a simple dict.

7. As Itamar mentioned, the fact that "content" is a simple string is problematic. I'm not particularly inspired, but I think that using a file-like object should fix it.

[exarkun]

(In [32014]) Spelling, whitespace; interface methods do not have self.

refs #5126

[therve]

#612 was closed as a duplicate of this one.

[therve]

(In [37440]) Branching to 'caching-agent-5126-2'

[therve]

OK, this is ready for another round of review. Most of my changes were against the cache interface: I made get return a Deferred, removed put/delete, and made content management more callbacks based so that it would be more flexible.

The content is now managed by an external protocol cache. One thing I'm wondering is whether the interface between the cache and its protocol should formalized, the current branch making it up to the implementation.

[tom.prince]

1. test_agent.MemoryCacheAgent
   • missing docstring
   • MemoryCacheAgentTests?
   • dataReceived without put seems odd

   • self.assertIdentical(None, self.successResultOf(protocol.made))
     

can be

Error: Failed to load processor pythonS

No macro or processor named 'pythonS' found

successResultOf is probably new enough that the definitive style isn't determined. But the assertion is testing a detail of SimpleAgentProtocol that isn't relevant here.

2. IHTTPCache
   • The interface seems too narrow. There is no way to tell the cache that the body of an entry is complete.
   • The use of dataReceived and deliverBody with unusual signatures feels strange to me. But it is perhaps reasonable. The other option might be to have get and put return something that had those methods (with their usual signature), but that seems like it would require more code for little or now benefit.
   • It lacks a way to remove cached entries/bodies. (Incidentally, instead of updating the cache with new contents, it concatenates all the cached responses.
3. Once the body has been cached, we can store the length.
4. I haven't looked at the relevant RFCs, but should we be using the cached headers, with the cached response, or is the server required to generate all the appropriate response headers, if we have a cache hit.

[tom.prince]

self.successResultOf(protocol.made)

[tom.prince]

(apperntly, I didn't review the tip of the branch)

• 1.3 and 2.1 is fixed

5. The parenthetical in 2.3 is fixed (but there are no tests of this)
6. Maybe use a tuple or dict, instead of having parallel _storage and _cache dicts.
7. getProtocol is confusingly named, since it is used to put a body.
8. Is the url enough, as a key? I would think that Accept-* headers, among other things, will affect the response. What do RFCs and proxies/browsers do about this?
9. The ticket description mentions adding other features/headers. Is IHTTPCache enough for those? I think the answer to 8 will answer this question as well.
10. Might a client want some way to find out when/whether a cache entry has successfully been written? This is perhaps outside the scope of IHTTPCache.

[tom.prince]

<glyph> tomprince: contentStorageProtocol?        
<glyph> Needs a better docstring, too.  "Manage received content"?

[therve]

HTTP/1.1 specifies the Vary header which seems to help caches differentiate how to store the response. But my understanding is that it's mostly useful if you write a complete cache, ie not asking the server for 304 response for relying on expire fields, which we don't. In our case, the different headers would generate a different response, so at worse we would uselessly cache responses that wouldn't be used if you change accept headers between requests.