n8 / multi_fetch_fragments

Multi-fetch Fragments makes rendering and caching a collection of template partials easier and faster.
http://ninjasandrobots.com/rails-faster-partial-rendering-and-caching
MIT License
539 stars 63 forks source link

Rails read_multi call takes parameter list not an array #1

Closed tomfakes closed 11 years ago

tomfakes commented 11 years ago

Hey Nate, I just added this to project of mine, and found a small bug.

The Rails.cache.read_multi() call actually takes a list of parameters, not an array. The Dalli cache store that I'm using then fails to return any results.

The easy fix is to splat the keys array:

result_hash = Rails.cache.read_multi(*(keys_to_collection_map.keys))

Then it works like a champ!

n8 commented 11 years ago

Thanks Tom! I totally see what you're getting at and makes sense. I'll make the change.

But I'm intellectually curious though why this would work for me as is with Dalli and Memcached right now:

Rails.cache.read_multi(keys_to_collection_map.keys)
=> {"lists/84-20121211192943/show"=>"\n\n<div style=\"padding-top: 10px; margin-bottom: 20px\"
Rails.cache.read_multi(keys_to_collection_map.keys).size
=> 6

Splatting it definitely works though too like you mentioned:

Rails.cache.read_multi(*(keys_to_collection_map.keys))
=> {"lists/84-20121211192943/show"=>"\n\n<div style=\"p

Wonder if it's a Rails version or Dalli version?

tomfakes commented 11 years ago

This current app is using Dalli 2.5.0 (the latest is 2.6.0) and Rails 3.1.3

I was a bit surprised about these parameters too - I think it's so you can pass in options too, but then that could be done with read_multi(array, options = {})

There was a Dalli bug about this that claims to be fixed to handle arrays.

n8 commented 11 years ago

Tom, what version of Ruby are you using? I'm trying to replicate this so I can really understand what's going on here. I've got Rails 3.1.3 and Dalli 2.5.0 running on my test app, but I don't think I'm still seeing the buggy behavior yet. Still looking into this a tad more before I commit the splat.

tomfakes commented 11 years ago

I'm using 1.9.3-p286 with the falcon patches to make it load faster.

I have another app using this same ruby, Rails 3.2.8 and Dalli 2.2.0 and that works fine.

There might be something else in the app causing the problem - its not my app, and its got a lot of other gems in there that might be intercepting the cache calls and need the parameters to be to spec

toreriklinnerud commented 11 years ago

I can reproduce the problem in development mode with no cache store defined, Rails 3.2.9

config.action_controller.perform_caching = true in development.rb

irb(main):002:0> Rails.cache.method(:read_multi).source_location => ["/Users/tel/.rbenv/versions/1.9.3-p0-perf/lib/ruby/gems/1.9.1/gems/activesupport-3.2.9/lib/active_support/cache.rb", 339]

irb(main):001:0> Rails.cache.write('a', 1) => true irb(main):002:0> Rails.cache.write('b', 2) => true irb(main):003:0> Rails.cache.read_multi(['a', 'b']) => {} irb(main):004:0> Rails.cache.read_multi(*['a', 'b']) => {"a"=>1, "b"=>2}

tomfakes commented 11 years ago

Ah - I could have been using the default cache instead of Dalli for the development environment. That makes more sense!

DouweM commented 11 years ago

I'm actually having the same problem with Dalli 2.6.3 when testing caching in development. From Dalli's source, it seems like it too expects a list:

      def read_multi(*names)
        names.extract_options!
        mapping = names.inject({}) { |memo, name| memo[expanded_key(name)] = name; memo }

And even if it didn't, if Rails's read_multi expects a list instead of an array, we should follow its lead.

DouweM commented 11 years ago

This has been fixed in my pull request #13.