search

aliostad / CacheCow

An implementation of HTTP Caching in .NET Core and 4.5.2+ for both the client and the server
MIT License
847 stars 172 forks source link

CacheCow.Client - honor Age header #257

Closed PiotrTecza closed 3 years ago

PiotrTecza commented 3 years ago

Great library @aliostad . I'm using it for client-side caching in conjunction with Microsoft.AspNetCore.Mvc.ResponseCacheAttribute for server-side caching. I've noticed that it doesn't honor the Age header when deciding if a cache entry is Stale.

Example: Server response comes with the following headers: Cache-Control: public,max-age=60 Age: 59

With the current logic, the above response will be client cached for the next 60 sec even though this cache entry will be invalidated by the server in 1 second. Is it on purpose or could the logic be updated to honor the Age header?

aliostad commented 3 years ago

Thanks @PiotrTecza is this something you see in a real-life situation? If so is the API you are testing against is public? If so I would like to use it for such cases. Age header is rarely sent back as far as I have seen hence did not implement it.

PiotrTecza commented 3 years ago

Unfortunately, this is one of our internal APIs in JustEat. I have created a GitHub repo with the default Asp.Net Core WebAPI project and added ResponseCache middleware. Behaviour is exactly the same, every cached response is served with the Age header. Tested with System.Net.Http.HttpClient and Postman v7.36.1 (please make sure Send no-cache header is disabled in settings)

aliostad commented 3 years ago

Sure, that is fine. Of course, this is part of the spec just not implemented. I will try to get this working soon - before next week.

PiotrTecza commented 3 years ago

Amazing, thanks a lot @aliostad

aliostad commented 3 years ago

OK I found a resource

https://fonts.gstatic.com/s/muli/v22/7Aulp_0qiz-aVz7u3PJLcUMYOFnOkEk30e6fwniDtzM.woff

aliostad commented 3 years ago

This should be working now with version 2.8. Let me know if there is any issue.

PiotrTecza commented 3 years ago

Hi @aliostad, I believe it still does not work as expected. I had a quick look at the implementation and I think the problem is that there's no logic in ResponseValidator delegate that would take Age header into account when calculating if a cache entry is Stale or not. So for the example above, it still keeps the entry in the cache for 60 sec, reporting that cache is OK, and then it returns MustRevalidate result.

aliostad commented 3 years ago

Sure, I see that I need some behavioural tests now. I will work on it soon.

aliostad commented 3 years ago

Hi, this should have been fixed now.

PiotrTecza commented 3 years ago

I can confirm this is now working as expected, thanks @aliostad.

aliostad commented 3 years ago

Awesome! Thanks for using and improving it.