Inconsistent fetch nil behavior #294

Closed
luxx opened this Issue Nov 27, 2012 · 7 comments

Projects

None yet

4 participants

@luxx
luxx commented Nov 27, 2012

Example code, run in rails console

#
# expected result, prints "fetching" one time
#
cache.flush
key = "t_nil";
p "value from get = #{cache.get(key).inspect}";
r = cache.fetch(key){ puts "fetching #{key}"; 5 };
r = cache.fetch(key){ puts "fetching #{key}"; 3 };
r = cache.fetch(key){ puts "fetching #{key}"; 3 };
r = cache.fetch(key){ puts "fetching #{key}"; 3 };
r = cache.fetch(key){ puts "fetching #{key}"; 3 };
p "value from fetch = #{r.inspect}";
p "value from get = #{cache.get(key).inspect}";
""

#
# unexpected result, prints "fetching" five times
#
cache.flush
key = "t_nil";
p "value from get = #{cache.get(key).inspect}";
r = cache.fetch(key){ puts "fetching #{key}"; nil };
r = cache.fetch(key){ puts "fetching #{key}"; 3 };
r = cache.fetch(key){ puts "fetching #{key}"; 3 };
r = cache.fetch(key){ puts "fetching #{key}"; 3 };
r = cache.fetch(key){ puts "fetching #{key}"; 3 };
p "value from fetch = #{r.inspect}";
p "value from get = #{cache.get(key).inspect}";
""

Two issues here:

  1. In dalli client, nil and missing value are not distinguished from each other, so if a fetch block returns nil, calling the same fetch block will result in the block being run once more on the next call. If the philosophy is "you should not cache nil, it represents no value" then that may be working as intended. I personally disagree with this approach and find it annoying because I like to cache expensive method calls where nil is a perfectly acceptable return value.

  2. If nil gets cached on the first fetch, all subsequent fetch blocks will be run, even if they return a non-nil value, and that non-nil value will not write over the nil value. The API seems to be making nil considerably more special than a missing cached value, so I am guessing that is a bug.

Thoughts?

@luxx
luxx commented Nov 27, 2012

here is an rspec test from my project showing the issue

  context "native fetch does not necessarily work as expected" do
    let(:key){"test_key"}
    before(:each){cache.flush}
    subject{cache}

    it "block WILL NOT execute on subsequent fetches" do
      counter = 0
      subject.get(key).should be_nil
      5.times { subject.fetch(key){ counter +=1 ; "a valuable value" } }

      subject.get(key).should eq "a valuable value"
      counter.should eq 1
    end

    it "block WILL execute on subsequent fetches when the value of the first fetch IS NIL" do
      counter = 0
      subject.get(key).should be_nil
      subject.fetch(key){ counter +=1 ; nil }
      5.times { subject.fetch(key){ counter +=1 ; "a valuable value" } }

      # this is where things get unintuitive, the block ran multiple times
      subject.get(key).should be_nil
      counter.should eq 6
    end
  end
@the8472
the8472 commented Feb 28, 2013

If the philosophy is "you should not cache nil, it represents no value" then that may be working as intended.

Not to mention that this would be inconsistent with the other ActiveSupport cache stores, which do cache nil values. E.g. the memory store uses Hash#key? internally to check for presence regardless of value.

So at least for the ActiveSupport adapter this behavior seems inappropriate.

I have run into the issue myself and written a monkeypatch for it, but it is quite ugly due to the way reads and writes are handled internally, as opposed to the other cache stores that wrap values in a CacheEntry object.

So yeah, i'd like to see this fixed, it affects performance.

@mperham
Collaborator
mperham commented May 27, 2013

You can always use the NullObject pattern to store an application-defined value but I'm also happy to see PRs which fix this for good.

The memcached gem raises an exception when the value is not found, to me this is a big performance problem as exceptions are expensive.

@rcook
rcook commented Jul 22, 2013

Here's a Gist of how I'm doing this using the null object pattern:

https://gist.github.com/rcook/6057142

Is this a reasonable workaround for now?

@the8472
the8472 commented Jul 23, 2013

Creating a new NilObject every time seems a waste, you could put the instance into a class-constant too.

And this only works fine if you always use fetch. read, write, multi-read, etc. obviously won't know about this.

@the8472
the8472 commented Jul 23, 2013

And it doesn't work with the LocalStore strategy, since that only checks for Cache::Entry objects internally, not for nil, but the dalli store doesn't use those.

@the8472
the8472 commented Oct 9, 2013

I'm using a custom rails cache store to fix this issue. On a few pages that are heavy on "try lookup, use fallback if nil" logic this provides a significant speedup: https://gist.github.com/the8472/6899185

For maximum performance under Rails 3.2 one can also monkey-patch ActiveSupport::Cache::Store::Entry to skip marshaling so LocalCache entries don't need to be unmarshaled since Dalli::Store takes care of that for us. Rails 4.0 fixes that.

@wjordan wjordan added a commit to wjordan/dalli that referenced this issue Sep 14, 2015
@wjordan wjordan support the fetch operation with nil values, fixes #294 3ff7aec
@wjordan wjordan added a commit to wjordan/dalli that referenced this issue Sep 16, 2015
@wjordan wjordan Add `:cache_nils` option to support nil values in `DalliStore#fetch` …
…and `Dalli::Client#fetch`.

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