Fix for expiration intervals longer than 30 days #436

Merged
merged 3 commits into from Feb 27, 2014

Projects

None yet

5 participants

@leonid-shevtsov
Contributor

I have found out (the hard way) that memcached silently doesn't store values with expiration over 30 days. (previously: #55 #251 #357)

Upon studying the code, I've decided that the best way to handle this is to transparently rewrite long expiration intervals into expiration timestamps, as the Memcached API asks.

Tests are provided, I've also tested the code integrated into my own environment, a debug message is logged when the translation happens, and nothing changes in the documented API.

What will change in existing apps is

  • apps that unknowingly use bad expiration intervals (such as 1 year) will suddenly start caching as intended
  • apps that knowingly exploit the "bug" will get wrong expiration timestamps in the far future.

I think compliance to APIs is worth breaking the latter. An safer alternative could be to provide both :expires_in and :expires_on options, and throw exceptions on invalid timestamps and intervals, but Rails, for one, doesn't even have :expires_on.

@mperham
Collaborator
mperham commented Feb 26, 2014

We already have a test_server file, would you move your tests into it?

@mperham
Collaborator
mperham commented Feb 26, 2014

And update the changelog please

@leonid-shevtsov
Contributor

Done!

@mperham mperham merged commit dfd1bfb into petergoldstein:master Feb 27, 2014

1 check failed

default The Travis CI build failed
Details
@eirc
eirc commented Jul 10, 2014

THANK YOU FOR THIS!! (also found out about it the hard way...)

@ctweney

This change produces a change in API behavior that should have been reserved for a major version upgrade (e.g. 2.8.x or 3.x).

Our project uses dalli and operated on the documented behavior of memcached, which is that expirations greater than 30 days worth of seconds are reinterpreted as Unix epochs. This fix makes dalli add today's epoch to the passed value, creating an expiration much longer than expected. My team just wasted hours of confusing debugging discovering this change, which I can't find any documentation for apart from this commit.

Please be more careful with API changing commits in the future!

Our code that expected the transparent memcached behavior:
https://github.com/ets-berkeley-edu/calcentral/blob/master/lib/cache/cacheable.rb#L105

Collaborator

Yes, this was dumb. It should have a max translation value of, e.g., one year. After that, treat it as an exact timestamp.

Contributor

@mperham but that would just make the logic even more complicated. As I wrote in the rationale in #436, some code will break, but ultimately less people will get confused and write broken code in the future.

What went wrong is a) I didn't add a note to the README and b) yes, in hindsight, it should have been reserved for a major version bump. But I implore you not to add more conditions.

Memcached's interpretation of large expiration inputs isn't a bug, it's a documented feature. I thought it was a bug, and reported as such, and was gently scolded for not reading the documentation:

https://code.google.com/p/memcached/issues/detail?id=330&can=1&q=expiration

https://github.com/memcached/memcached/blob/master/doc/protocol.txt#L79

You can argue that it's a stupid feature (and I would agree with you) but it's the official documented policy of the product, and memcached purists will be expecting dalli to behave in the same way.

@annaswims

For anyone else that stumbles across this problem, Rails adds 5 minutes to the ttl for reasons that I don't understand. So, :expires_on => 30.days ends up being being greater than 30 days rails/rails@13d8777

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment