Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Transparently fix error with expiration intervals >30 days interprete…

…d as timestamps by Memcached
  • Loading branch information...
commit 5f6489710c0c726dee636323124f3baeead01064 1 parent 66e9266
@leonid-shevtsov leonid-shevtsov authored
Showing with 33 additions and 2 deletions.
  1. +18 −2 lib/dalli/server.rb
  2. +15 −0 test/test_ttl.rb
View
20 lib/dalli/server.rb
@@ -268,6 +268,7 @@ def send_multiget(keys)
def set(key, value, ttl, cas, options)
(value, flags) = serialize(key, value, options)
+ ttl = sanitize_ttl(ttl)
guard_max_value(key, value) do
req = [REQUEST, OPCODES[multi? ? :setq : :set], key.bytesize, 8, 0, 0, value.bytesize + key.bytesize + 8, 0, cas, flags, ttl, key, value].pack(FORMAT[:set])
@@ -278,6 +279,7 @@ def set(key, value, ttl, cas, options)
def add(key, value, ttl, options)
(value, flags) = serialize(key, value, options)
+ ttl = sanitize_ttl(ttl)
guard_max_value(key, value) do
req = [REQUEST, OPCODES[multi? ? :addq : :add], key.bytesize, 8, 0, 0, value.bytesize + key.bytesize + 8, 0, 0, flags, ttl, key, value].pack(FORMAT[:add])
@@ -288,6 +290,7 @@ def add(key, value, ttl, options)
def replace(key, value, ttl, cas, options)
(value, flags) = serialize(key, value, options)
+ ttl = sanitize_ttl(ttl)
guard_max_value(key, value) do
req = [REQUEST, OPCODES[multi? ? :replaceq : :replace], key.bytesize, 8, 0, 0, value.bytesize + key.bytesize + 8, 0, cas, flags, ttl, key, value].pack(FORMAT[:replace])
@@ -309,7 +312,7 @@ def flush(ttl)
end
def decr(key, count, ttl, default)
- expiry = default ? ttl : 0xFFFFFFFF
+ expiry = default ? sanitize_ttl(ttl) : 0xFFFFFFFF
default ||= 0
(h, l) = split(count)
(dh, dl) = split(default)
@@ -320,7 +323,7 @@ def decr(key, count, ttl, default)
end
def incr(key, count, ttl, default)
- expiry = default ? ttl : 0xFFFFFFFF
+ expiry = default ? sanitize_ttl(ttl) : 0xFFFFFFFF
default ||= 0
(h, l) = split(count)
(dh, dl) = split(default)
@@ -376,6 +379,7 @@ def version
end
def touch(key, ttl)
+ ttl = sanitize_ttl(ttl)
write_generic [REQUEST, OPCODES[:touch], key.bytesize, 4, 0, 0, key.bytesize + 4, 0, 0, ttl, key].pack(FORMAT[:touch])
end
@@ -456,6 +460,18 @@ def guard_max_value(key, value)
end
end
+ # https://code.google.com/p/memcached/wiki/NewCommands#Standard_Protocol
+ # > An expiration time, in seconds. Can be up to 30 days. After 30 days, is treated as a unix timestamp of an exact date.
+ MAX_ACCEPTABLE_EXPIRATION_INTERVAL = 30*24*60*60 # 30 days
+ def sanitize_ttl(ttl)
+ if ttl > MAX_ACCEPTABLE_EXPIRATION_INTERVAL
+ Dalli.logger.debug "Expiration interval too long for Memcached, converting to an expiration timestamp"
+ Time.now.to_i + ttl
+ else
+ ttl
+ end
+ end
+
def generic_response(unpack=false)
(extras, _, status, count) = read_header.unpack(NORMAL_HEADER)
data = read(count) if count > 0
View
15 test/test_ttl.rb
@@ -0,0 +1,15 @@
+require 'helper'
+
+describe Dalli::Server do
+ describe 'ttl translation' do
+ it 'does not translate ttls under 30 days' do
+ s = Dalli::Server.new('localhost')
+ assert_equal s.send(:sanitize_ttl, 30*24*60*60), 30*24*60*60
+ end
+
+ it 'translates ttls over 30 days into timestamps' do
+ s = Dalli::Server.new('localhost')
+ assert_equal s.send(:sanitize_ttl, 30*24*60*60 + 1), Time.now.to_i + 30*24*60*60+1
+ end
+ end
+end

4 comments on commit 5f64897

@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

@mperham
Owner

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

@leonid-shevtsov

@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.

@ctweney

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.

Please sign in to comment.
Something went wrong with that request. Please try again.