Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

ActiveSupport#Cache :race_condition_ttl option handling not optimal #14157

Open
wants to merge 3 commits into from

3 participants

@niclas

nil.to_i ==> 0, e.g. second condition is always evaluated here...

activesupport/lib/active_support/cache.rb
@@ -563,7 +563,7 @@ def find_cached_entry(key, name, options)
def handle_expired_entry(entry, key, options)
if entry && entry.expired?
- race_ttl = options[:race_condition_ttl].to_i
+ race_ttl = options[:race_condition_ttl].try(:to_i)

consider something like this instead:

time_since_expired = Time.now.to_f - entry.expires_at
race_ttl = options.fetch(:race_condition_ttl, (time_since_expired - 1))
if time_since_expired <= race_ttl
#do work

This eliminates the whole nil check premise.

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

It would be ideal if there was a test case that noted the change in behavior as well

@patricksrobertson

This is also related to the PR #10001 and should be closed at the same time.

@niclas

After considering PR #10001 this is my new proposal. I think the code gets more readable if time calcs is avoided for zero/nil cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
6 activesupport/lib/active_support/cache.rb
@@ -564,7 +564,7 @@ def find_cached_entry(key, name, options)
def handle_expired_entry(entry, key, options)
if entry && entry.expired?
race_ttl = options[:race_condition_ttl].to_i
- if race_ttl && (Time.now.to_f - entry.expires_at <= race_ttl)
+ if race_ttl > 0 && entry.time_since_expired <= race_ttl
# When an entry has :race_condition_ttl defined, put the stale entry back into the cache
# for a brief period while the entry is begin recalculated.
entry.expires_at = Time.now + race_ttl
@@ -639,6 +639,10 @@ def expires_at=(value)
@expires_in = nil
end
end
+
+ def time_since_expired
+ Time.now.to_f - expires_at if expired?
+ end
# Returns the size of the cached value. This could be less than
# <tt>value.size</tt> if the data is compressed.
View
26 activesupport/test/caching_test.rb
@@ -409,7 +409,7 @@ def test_race_condition_protection
end
assert_equal "baz", result
end
-
+
def test_race_condition_protection_is_limited
time = Time.now
@cache.write('foo', 'bar', :expires_in => 60)
@@ -420,7 +420,7 @@ def test_race_condition_protection_is_limited
end
assert_equal "baz", result
end
-
+
def test_race_condition_protection_is_safe
time = Time.now
@cache.write('foo', 'bar', :expires_in => 60)
@@ -437,6 +437,17 @@ def test_race_condition_protection_is_safe
assert_nil @cache.read('foo')
end
+ def test_race_condition_protection_is_truly_disarmed_for_nil
+ time = Time.now
+ Time.stubs(:now).returns(time)
+ @cache.write('foo', 'bar', :expires_in => 60)
+ @cache.expects(:delete_entry)
+ Time.stubs(:now).returns(time + 60)
+ @cache.fetch('foo', :race_condition_ttl => nil) do
+ 'baz'
+ end
+ end
+
def test_crazy_key_characters
crazy_key = "#/:*(<+=> )&$%@?;'\"\'`~-"
assert @cache.write(crazy_key, "1", :raw => true)
@@ -1029,6 +1040,17 @@ def test_expired
assert entry.expired?, 'entry is expired'
end
+ def test_time_since_expired
+ time = Time.now
+ Time.stubs(:now).returns(time)
+ entry = ActiveSupport::Cache::Entry.new("value", :expires_in => 60)
+ assert_equal nil, entry.time_since_expired
+ Time.stubs(:now).returns(time + 60)
+ assert_equal 0.0, entry.time_since_expired
+ Time.stubs(:now).returns(time + 61)
+ assert_equal 1.0, entry.time_since_expired
+ end
+
def test_compress_values
value = "value" * 100
entry = ActiveSupport::Cache::Entry.new(value, :compress => true, :compress_threshold => 1)
Something went wrong with that request. Please try again.