Response object caching needs access to response headers

[AnthonySteele]

I've been having a play with the caching interface, and while it solved a problem on the website at the time (caching needs to be at the right point in the control flow so that we only cache successful deserialised responses), I don't think that we solve the general case.

In particular responses describe how (and if) they should be cached using response headers, I.e. “Cache-Control: max-age=60, private”

IResponseCache.Set should take a Response object not a RequestData object in order to give the cache access to the response headers. It would still have the request via the response.OriginalRequest property.

I can update the sample code in the wiki to match if we make this change

[gregsochanik]

Sounds good to me.

Would also open up ability to base logic on other Cache related headers i.e. ETag / If-None-Matched or Last-Modified / If-Modified-Since, if the IResponseCache has access to request/response headers.

That's of course, once the api router supports them!

[AnthonySteele]

Yep, the cache would be able to look at all of the response headers, and react as needed. I don't think that the caching is widely used. How much of a breaking change/version bump is this?

[raoulmillais]

For what it's worth, I really regret putting caching into both the node and c# clients It's a big source of bugs and too easy for consumers to use wrongly. Varnish (or some other caching proxy) on the same box or on a box which is very close with the client pointing at is much easier and less error prone in my experience.

[AnthonySteele]

Choice is either to supply sample code that does it well enough here https://github.com/7digital/SevenDigital.Api.Wrapper/wiki/Response-caching which would mean using response headers.

or to remove it entirely.

[gregsochanik]

@raoulmillais fair enough - it does add extra complexity, but its usage is completely optional and up to a user to implement (however badly that may be :smiley: ). I don't think there's any concrete caching logic in the api-wrapper.

I use it in a couple of places and find it useful in conjunction with MemoryCache for a quick cache implementation, and would like to be able to use it to deliver fewer hits to locker-api / playlist-api, so would love access to the cache headers.

Backwards compatibility-wise it's a bit trickier... if you added an overloaded method it'd only be a minor version bump, but then which method does the internal engine use? Would be messy and I can't think of a way at the mo how it could be gracefully done.

@AnthonySteele, as you say, the best idea would be to change the method to accept Response, but then that's a breaking change and would require a full version increment. I guess it comes down to whether we think it's worth it.

[AnthonySteele]

To be honest the C# Api wrapper and schema doesn't get a whole lot of use, but it is also useful as a reference on how to use the 7digital api. As such, showing what to do with the response headers is useful.

[AnthonySteele]

Have a look at this: https://github.com/AnthonySteele/SevenDigital.Api.Wrapper/tree/cache_response_headers The two new classes are InMemoryResponseCache and CacheHeaderReader

• the IResponseCache.Set method takes a Response object with headers as discussed above.
• InMemoryResponseCache is an implementation of IResponseCache that forwards to the .Net MemoryCache.
• It uses CacheHeaderReader to work out if and for how long responses should be cached. This is separate code to the in-memory cache as it could be used for other cache backends. The aim is to correctly parse the header that the 7digital api router sends, e.g. Cache-Control: max-age=21600, and also some common simple variations like “no-store” and to not throw exceptions in other cases , even junk in the header.
• it's not battle-tested yet.

Should the InMemoryResponseCache be included as the default cache in the wrapper over NullCache? i.e. default to caching with opt-out rather than opt-in. If not, why not?

This is sample code for other implementers as well as “ready to use code”, i.e. we can point 3rd parties looking for guidelines here. It tells the reader that you can and should cache 7digital responses based on the value in “Cache-Control: max-age=”

[gregsochanik]

Will be looking a this today, just a quick question to clarify....

My understanding of the current setup, is that if you want to use a cache, you specify UsingCache(IResponseCache).

When you say "Should the InMemoryResponseCache be included as the default cache in the wrapper over NullCache?", do you mean used as the default cache if a user explicitly specifies UsingCache() (i.e. without supplying an IResponseCache implementation as a method param)?

Or do you intend a default IResponseCache implementation to always be used with each request, and the logic for deciding whether or not caching is relevant exists within this (i.e. within the CacheHeaderReader)?

[AnthonySteele]

Your understanding of how to specify a cache is correct.

Yes, I am asking if we should have InMemoryResponseCache used as the default cache unless a user explicitly specifies something else. I can't think of any reason why it's a bad idea. If we send these cache control response headers, then shouldn't we trust them a bit? Is it a sensible default to believe the response headers?

Technically there is always a cache instance in place, except that the default cache is currently NullResponseCache which does nothing. This is the Null object Pattern .

The cache is responsible for deciding if a response should be cached or not, so the "logic for deciding whether or not caching is relevant" does indeed get called from the cache, where InMemoryResponseCache calls CacheHeaderReader. A different cache implementation (e.g. storing in redis or memcached) could also use CacheHeaderReader.

[gregsochanik]

Ahh - cool, hadn't checked the code so didn't know about the NullResponseCache always being in place.

Then yes, I don't see any reason why we shouldn't default to opt-out either. I'll give it a go!

[raoulmillais]

Would it not be sensible to have it opt-in until it's been run in production on apps for a while? You can make it default when you're happy if you consider the migration path now. The reason I keep piping up on this thread is that historically caching logic has been a very rich source of bugs

[AnthonySteele]

Ok, let's leave it opt-in for now, consider making it opt-out after some production use.

[gregsochanik]

Agreed, that would be more sensible.

[AnthonySteele]

@raoulmillais I took care to test that the cache header reading would always fail safe - i.e. if it can't find or parse the cache-control header it will not throw an exception just treats the response as not cacheable. What other sources of error are there to look out for?

I have been using this document a bit: http://www.mobify.com/blog/beginners-guide-to-http-cache-headers/

[gregsochanik]

Merged PR so closing