ArgumentError: marshal data too short when nil is cached

[drobtravels]

According to the Cache Store Guide it is valid to cache nil, and this happens occasionally due to my use of fetch. However it seems it is attempting to marshal nil, which causes an error

Rails.cache.write('foo', nil)
# => "OK"
Rails.cache.read('foo')
# ArgumentError: marshal data too short

This has been tested in Rails 4.2.3 and 4.2.4, with Ruby 2.2.2. It works fine using other cache stores, such as memory_store

[sorentwo]

Right you are! Working on a patch, will have it out soon.

[sorentwo]

@droberts84 This should be fixed now. Would you mind giving it a try from master and letting me know?

[drobtravels]

First of all, thanks for the quick fix!

This fixes the initial issue, however I found a related quirk. It appears it doesn't actually cache the nil value, but instead behaves similar to a cache miss. See the example below

def check_cache
  Rails.cache.fetch('foo') do
    puts 'miss'
    nil
  end
end

check_cache
# miss
# => nil

check_cache
# miss
# => nil

Personally I prefer this behavior, however its not consistent with other cache stores. Since nil is a valid cache value, you would not expect a cache miss for the second fetch. I tested this same code against memory_store which did not print miss.

[sorentwo]

Yes, I can see how that's a problem. In the case of a single fetch we could work around this by checking for existence rather than the return value. That approach won't work for the *multi methods though. There everything is fetched in bulk and the missing keys all come back as nil. It is technically possible to alter read_multi to check keys before fetching, but it makes the method more complex and certainly slower. I'm not sure that I am willing to accept that trade off right now.

This may be a quirk that needs to be documented and lived with. What do you think?

[drobtravels]

Sounds good to me. This quirk actually works in my favor (fetch will miss and cause a retry, which is generally desirable). Are you keeping the current behavior in single fetch or adding the workaround? Personally I think consistency among both methods would be best, even if its not the standard, but that is of course your call.

Thanks again for your quick action on this issue.

On Thu, Sep 3, 2015 at 1:24 PM Parker Selbert notifications@github.com wrote:

Yes, I can see how that's a problem. In the case of a single fetch we could work around this by checking for existence rather than the return value. That approach won't work for the *multi methods though. There everything is fetched in bulk and the missing keys all come back as nil. It is technically possible to alter read_multi to check keys before fetching, but it makes the method more complex and certainly slower. I'm not sure that I am willing to accept that trade off right now.

This may be a quirk that needs to be documented and lived with. What do you think?

— Reply to this email directly or view it on GitHub https://github.com/sorentwo/readthis/issues/15#issuecomment-137519662.

[sorentwo]

I definitely want to keep consistency between the fetch methods. In this case that means ignoring nil for fetch and fetch_multi.

[Napolskih]

Hi I am also concerned about the problem of "caching nil". Maybe solve this problem by caching NullObject, by analogy with the way it's done here: https://github.com/mperham/dalli/pull/559

I honestly do not understand the problem with the fetch_multi.

In my opinion, the fact that an empty value is not cached reduces the efficiency of the application.