Change the fetch method to deal with recyclable key cache strategy #2288

cintamani · 2018-10-01T12:22:05Z

In order to keep compatibility between the AMS cache feature and with Rails > 5.1 cache versioning which introduces Recyclable Keys, we have to account for the version of the cache separately from the cache key.
This PR address the issue for application with Rails version > 5.1 where the cache_versioning setting is true

More info: #2287

bf4 · 2018-10-24T22:03:33Z

Would you mind rebasing off of 0-10-stable?

cintamani · 2018-10-26T13:51:25Z

@bf4 Hi there :) this is already rebased against 0-10-stable

bf4 · 2018-10-26T15:59:33Z

Looking at AppVeyor failures on Ruby 2.3

Finished in 2.835022s, 199.9985 runs/s, 345.6764 assertions/s.
  1) Failure:
ActiveModelSerializers::CacheTest#test_expiring_of_cache_at_update_of_record [C:/projects/active-model-serializers/test/cache_test.rb:165]:
--- expected
+++ actual
@@ -1,2 +1,2 @@
 # encoding: UTF-8
-"Bar"
+"Foo"

cintamani · 2018-11-07T11:17:38Z

@bf4 I'm trying to reproduce the issue running the tests in 2.3 ruby and 4.2 rails but without success (well, without a failure XD ) . I have added some more code based on what I have found here: #1168 for now

bf4 · 2018-11-13T04:21:11Z

hey, it passed!

test/cache_test.rb

bf4 · 2018-11-13T04:24:41Z

lib/active_model/serializer/concerns/caching.rb

@@ -229,6 +229,7 @@ def fetch_attributes(fields, cached_attributes, adapter_instance)
      def fetch(adapter_instance, cache_options = serializer_class._cache_options, key = nil)
        if serializer_class.cache_store
          key ||= cache_key(adapter_instance)
+          cache_options = (cache_options || {}).merge(version: object_cache_version) if object_cache_version


so this will always be merged in, hmm

Do you think it is likely to create issues in other parts of the code?

I'm not familiar enough with the Rails code changes to be really sure.

I'd look at https://blog.heroku.com/cache-invalidation-rails-5-2-dalli-store to get any ideas.

How does it fail without this PR?

@cintamani ?

@bf4 I have added a test that you can see failing without the rest of the fixes in this pr: https://github.com/rails-api/active_model_serializers/pull/2288/files#diff-337d3920ba1fab97fd9fdd688d7664f3R152 :)

fwiw, this could be

cache_options = (cache_options || {}).merge!(version: object_cache_version).tap(&:compact!)

Hash#compact removes members with nil keys

Hello @cintamani . As per the objective of this PR, do we really need to send the version option here to fix the issue of AMS introduced since rails-5.2.0 recyclable cache key when ActiveRecord::Base.cache_versioning = true.

If AMS is supposed to keep working with cache keys of the form model/id-timestamp as usual, we can just follow the steps inside #object_cache_key method as much like https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache.rb#L94 that you already did in the first commit of this PR: 7d498d2

If AMS ever decides to support recyclable cache keys like Rails in future, this PR rails/rails#29092 might help to implement the case.

Thanks.

do we really need to send the version option here ?

@wasifhossain are you saying you think this PR takes the wrong approach?

It's unclear to me if your review is based on usage of recyclable cache keys or not. I believe @cintamani is using this in production

test/cache_test.rb

bf4 · 2018-12-17T14:58:33Z

Ok, I'm happy to merge this when you're happy with it

cintamani · 2018-12-17T15:33:16Z

@bf4 thanks 🙇‍♀️ ASAP I have some time to test it one last time I'll merge it :)

bf4 · 2018-12-19T01:17:56Z

@cintamani would you like this merged now? (I'm not confident I understand your last comment as intended)

cintamani · 2019-01-10T15:58:25Z

@bf4 sorry I'm not very active here 🙇‍♀️I have tested it again in my local environment and it still looks good and working as intended. But consider we are not currently using this in production yet due to issues with the Dalli gem. If you are still up for it, I will merge this one in tomorrow.

bf4 · 2019-01-10T17:56:47Z

@cintamani Since the pace of development here is pretty slow, I'd prefer waiting for you to confirm it works for you in production before I merging this PR. That ok?

cintamani · 2019-01-14T11:15:28Z

@bf4 totally :) I'll keep you posted

maxrosecollins · 2019-01-22T15:53:49Z

Can we get this merged please?

None of my cached objects ever update, even with belongs_to :parent, touch: true

bf4 · 2019-01-22T17:21:46Z

@maxrosecollins have you confirmed it works for you in prod?

Obviously we can merge it. But I would like confirmation not only that tests pass, but that it solves the problem it's intended to. I'm not currently using AMS.

maxrosecollins · 2019-01-22T17:42:34Z

I tried to

gem 'active_model_serializers', github: "cintamani/active_model_serializers", branch: "patch-1"

I got this error

Fetching https://github.com/cintamani/active_model_serializers.git
fatal: Could not parse object '7427d893f4e07e45003e92e8303bf698764f68fc'.
Revision 7427d893f4e07e45003e92e8303bf698764f68fc does not exist in the repository https://github.com/cintamani/active_model_serializers.git. Maybe you misspelled it?

Which is strange because I can use the other branches :/
Any ideas where I went wrong?

bf4 · 2019-01-22T20:04:25Z

@maxrosecollins This isn't really the place to debug bundling a Gem from GitHub...

I see https://github.com/cintamani/active_model_serializers/tree/patch-1 is at 7427d89 so I'm not really sure what oddities in your Gemfile are causing the problem. Try specified the absolute git url, and it that doesn't work, probably try a forum or slack. git: "https://github.com/cintamani/active_model_serializers.git"

maxrosecollins · 2019-01-22T22:30:13Z

Sure... Well I have tried all of these different methods. Have you tried installing it?

…

On Tue, 22 Jan 2019 at 20:04, Benjamin Fleischer ***@***.***> wrote: @maxrosecollins <https://github.com/maxrosecollins> This isn't really the place to debug bundling a Gem from GitHub... I see https://github.com/cintamani/active_model_serializers/tree/patch-1 is at 7427d89 <7427d89> so I'm not really sure what oddities in your Gemfile are causing the problem. Try specified the absolute git url, and it that doesn't work, probably try a forum or slack. git: " https://github.com/cintamani/active_model_serializers.git" — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAv3PR4ju2nUbwI3eI7ILufhWZC7HFvLks5vF27NgaJpZM4XB80X> .

bf4 · 2019-01-22T23:05:35Z

@maxrosecollins like I said, this isn't the place to debug installing a gem from github. It makes the conversation on the topic of the PR hard to follow.

Also, though I'm not using AMS, I just added the prescribed line to my Gemfile and successfully installed it. So, whatever is happening on your computer is 99.99999% certain to be unrelated to AMS.

I wish I had the bandwidth to help you further and a place for that, but I don't right now. I am sorry.

I'm just one person taking time away from his work to try and help make sure changes like this PR help people (and not cause harm)

here my line in my Gemfile. I'm using Ruby 2.4.4, bundler 1.16.1, Rails 5.0.7

gem "active_model_serializers", git: "https://github.com/cintamani/active_model_serializers.git", branch: "patch-1"

If you found a bug in bundler, they'll ask you to make a test repo to demonstrate and you can post an issue there.

maxrosecollins · 2019-01-22T23:46:51Z

No worries.

I'll take a look and see if I can get it working.

cintamani · 2019-01-24T19:00:46Z

@maxrosecollins this version works for sure 7d498d2 let me know if you get the time to make this PR's code work. I am very full those days and last time I have checked I found the error you are mentioning too. Still didn't have time to look at it properly.

wasifhossain · 2019-01-25T03:41:47Z

@maxrosecollins this version works for sure 7d498d2 let me know if you get the time to make this PR's code work. I am very full those days and last time I have checked I found the error you are mentioning too. Still didn't have time to look at it properly.

I can confirm the fact that this commit 7d498d2 (1st commit of this PR) should work 🎉 . Working excellent 👌 on our production app with this cherry-pick gomalindo@823b558, i.e. cache key expires properly in both cases when ActiveRecord::Base.cache_versioning is set to true/false as the cache key is formed containing the dynamic part (updated_at).

Unfortunately, unlike that single commit, this PR does not work as expected :(

TL;DR collection_serializer cache keys don't expire at all using this PR, resulting in stale cache values

For those who are not concerned yet what this PR is trying to solve, I would humbly request to have a quick visit to these links

Recyclable cache keys rails/rails#29092: PR which introduced recyclable cache keys in rails 5.2
https://blog.heroku.com/cache-invalidation-rails-5-2-dalli-store: a recent issue (does not relate to this PR directly, but will help in debugging similar issues)

In short, when ActiveRecord::Base.cache_versioning = true (which will become default from Rails 6), updated_at is stored inside cache value, while cache key remains static over time (which is why its called recyclable cache key), e.g. products/1. In AMS, a cache key would look like: products/1/attributes.

But when ActiveRecord::Base.cache_versioning = false, AMS cache keys would look like products/1-20190110212435047688/attributes

To read/write the latest cache value, we need to provide the :version option in the ActiveSupport::Cache::Store#fetch method, which can be easily retrieved by calling ActiveRecord::Base#cache_version

Here are some examples to highlight the behavior of a recyclable cache key

# returns up-to-date cached value
def cached_product(product)
  Rails.cache.fetch(product.cache_key, version: product.cache_version) do
    product
  end
end

product = Product.first
product.name # => "Product 1"
v1 = product.cache_version # => "20190125020615012258"

# here calling this method will store the current product value inside `product.cache_key` ('products/1')
cached_product(product).name # => "Product 1"

# now reading the cache value with/without the cache version will return v1 value
Rails.cache.read(product.cache_key, version: v1).name # => "Product 1"
Rails.cache.read(product.cache_key).name # => "Product 1"

# let's update the product
product.update(name: 'Product 2')
product.name # => "Product 2"
v2 = product.cache_version # => "20190125022443338534"

# here calling this method will overwrite the cache value with the v2 product value
cached_product(product).name # => "Product 2"

# now reading the cache value with/without the cache version will return v2 value
Rails.cache.read(product.cache_key, version: v2).name # => "Product 2"
Rails.cache.read(product.cache_key).name # => "Product 2"

# but trying to read the cache value with `version: v1` will return nil as it's been overwritten with v2
# hence ensuring that the cache key is recycled properly
Rails.cache.read(product.cache_key, version: v1) # => nil
Rails.cache.read(product.cache_key).name # => "Product 2"

This is what the PR tried to do exactly: https://github.com/rails-api/active_model_serializers/pull/2288/files#diff-b762caa6dd51036307106006dd81600f. So the version info is passed while fetching the cache value.

Now we can discuss the actual issue. For collection serializer, the serialization happens in this sequence:

all the attributes' keys & values are read from the cache using ActiveSupport::Cache::Store#read_multi

# active_model_serializers/lib/active_model/serializer/collection_serializer.rb#25

options[:cached_attributes] ||= ActiveModel::Serializer.cache_read_multi(self, adapter_instance, options[:include_directive])

here it looks up a key in the hash derived above. If the key is found, it returns the value. Otherwise the new value is written to cache using the cache version introduced in this PR.

Here the thing to note is: once the cache is written for some key, it will be read using read_multi as described on point 1. But ActiveSupport::Cache::Store#read_multi unfortunately does not support versioning yet. (does not accept :version option unlike fetch/read). And so we will always get the oldest cache value stored using the recyclable cache key support introduced in this PR.

Summary (assuming a model has been updated multiple times):

Without this PR:
- ActiveRecord::Base.cache_versioning = true: cache key looks like products/1/attributes resulting in stale cache value
- ActiveRecord::Base.cache_versioning = false: cache key looks like products/1-20190110212435047688/attributes resulting in up-to-date cache value
With this PR:
- ActiveRecord::Base.cache_versioning = true: cache key looks like products/1/attributes resulting in stale cache value
- ActiveRecord::Base.cache_versioning = false: cache key looks like products/1-20190110212435047688/attributes resulting in up-to-date cache value
With the very first commit in this PR 7d498d2:
- ActiveRecord::Base.cache_versioning = true: cache key looks like products/1-20190110212435047688/attributes resulting in up-to-date cache value
- ActiveRecord::Base.cache_versioning = false: cache key looks like products/1-20190110212435047688/attributes resulting in up-to-date cache value

So my humble opinion is that we stick with the form of cache key that would embed the version info itself as before i.e products/1-20190110212435047688/attributes which is fixed with the very first commit in this PR 7d498d2 and wait for the rails community until they support recyclable cache key versioning in ActiveSupport::Cache::Store#read_multi

cintamani · 2019-01-25T13:52:27Z

Yes. The idea behind is that we start sending the key and versions as 2 separate value to the cache gem. But since not even dalli is actually ready to deal with recyclable keys, I believe we should merge the first version of this change that simply uses cache_key_with_version when available.
If you all agree, I'll update the code.

test/cache_test.rb

cintamani · 2019-02-01T17:47:17Z

If Trevis is happy, I'm happy for you to merge this. And yes I would like to be added as a collaborator if you are happy with it :)

cintamani · 2019-02-01T18:05:54Z

@bf4 looks like we have lost support to 4.2 (also is Appveyor suppose to run Rails 4.2 only 🤔?)

bf4 · 2019-02-01T20:29:20Z

@cintamani at the end of the day, appveyor runs whatever maintainers consider a valuable burden to maintain. I'm ok with adding more complexity to the appveyor config, but that should be balanced in consideration of what it might be testing that travis isn't. Having >0 Rails version on appveyor is valuable. Having >1 is up for grabs :)

wasifhossain · 2019-02-02T14:18:03Z

@bf4 i would also be very happy to be a collaborator. Would appreciate some directions from you to be effective enough with the role. Thanks

bf4 · 2019-02-03T19:43:17Z

I've sent you both invites

cintamani · 2019-02-04T10:55:29Z

@bf4 Rails 4.2 versions are in the list of "Allowed Failures" for travis so I suggest we update the rails version on AppVeyor too. Also the 4.2 rails version failures doesn't look related to the code in this PR, can you confirm it? Also, can you confirm that we are happy for the next released version of AMS to not support Rails 4.2?

test/cache_test.rb

bf4 · 2019-02-05T01:36:21Z

@cintamani

Rails 4.2 versions are in the list of "Allowed Failures" for travis

?

active_model_serializers/.travis.yml

Lines 47 to 62 in 209834d

    
           exclude: 
        
           - { rvm: 2.1.10,        env: RAILS_VERSION=master } 
        
           - { rvm: 2.2.8,        env: RAILS_VERSION=master } 
        
           - { rvm: 2.3.5,        env: RAILS_VERSION=master } 
        
           - { rvm: 2.4.2,        env: RAILS_VERSION=master } 
        
           - { rvm: 2.1.10,        env: RAILS_VERSION=5.0 } 
        
           - { rvm: 2.1.10,        env: RAILS_VERSION=5.1 } 
        
           - { rvm: 2.1.10,        env: RAILS_VERSION=5.2 } 
        
           - { rvm: 2.4.2,         env: RAILS_VERSION=4.1 } 
        
           - { rvm: 2.5.3,         env: RAILS_VERSION=4.1 } 
        
           - { rvm: ruby-head,     env: RAILS_VERSION=4.1 } 
        
           allow_failures: 
        
             - rvm: ruby-head 
        
             - rvm: jruby-head 
        
             # See JRuby currently failing on Rails 5+ https://github.com/jruby/activerecord-jdbc-adapter/issues/708 
        
             - { rvm: jruby-9.1.13.0, jdk: oraclejdk8, env: "RAILS_VERSION=5.1 JRUBY_OPTS='--dev -J-Xmx1024M --debug'" }

cintamani · 2019-02-05T15:31:42Z

@bf4 sorry my mistake. The way Travis show the allowed failures in their view make me thought it was allowed for the Rails version.

Co-Authored-By: cintamani <cintamani.puddu@gmail.com>

…ant tests

bf4 · 2019-02-05T17:09:41Z

wtf failures

/home/travis/build/rails-api/active_model_serializers/vendor/bundle/ruby/2.2.0/gems/activerecord-4.1.16/lib/active_record/connection_adapters/connection_specification.rb:190:in rescue in spec': Specified 'sqlite3' for database adapter, but the gem is not loaded. Add gem 'sqlite3' to your Gemfile (and ensure its version is at the minimum required by ActiveRecord). (Gem::LoadError)`

cintamani · 2019-02-05T18:37:20Z

Yep saw it on Appveyor first but I ignored it. I see Travis is having the same issue 😩investigating

bf4 · 2019-02-05T20:00:40Z

@cintamani the good news it, you now have the power to create a PR to fix it and merge when green :)

CHANGELOG.md

Run tests again

…ek is not compatible with the adapters currently in use

cintamani · 2019-02-08T11:28:03Z

We are very unlucky! Looks like the issue is the newly released version of sqlite3 we are using in our test. It was released on the 4th of February, just in time to mess up with this PR 😆
I have added a version rule in the Gemfile so we stick to 1.3.13. Appveyor and Travis looks happy again 🎊 🎊

cintamani · 2019-02-08T12:38:18Z

@bf4 will you deal with creating a new release? Otherwise I'll give it a go myself

bf4 · 2019-02-08T17:28:24Z

@cintamani you can't release until you get rubygems permissions. I'm going to wait a little bit before adding you as a gem owner.

I juste released 0.10.9

bb0f9d0

https://twitter.com/rubygems/status/1093924332659789827

https://twitter.com/hazula/status/1093925234074361857

cc @wasifhossain

cintamani · 2019-02-09T15:18:45Z

Thank you, @bf4! I have never dealt with maintaining a public gem before, so forgive my ignorance about the RubyGems permissions 🙂

bf4 · 2019-02-11T14:49:23Z

@cintamani it's a pretty common thing to forget or just not consider.

The gems we install are from rubygems.org. Only gem owners can 'push' a gem
GitHub is just where a lot of people collaborate on code. But permissions to write code on GitHub don't sync in any way with ownership on rubygems.org

shaojunda · 2019-06-19T14:18:14Z

@maxrosecollins this version works for sure 7d498d2 let me know if you get the time to make this PR's code work. I am very full those days and last time I have checked I found the error you are mentioning too. Still didn't have time to look at it properly.

I can confirm the fact that this commit 7d498d2 (1st commit of this PR) should work 🎉 . Working excellent 👌 on our production app with this cherry-pick gomalindo@823b558, i.e. cache key expires properly in both cases when ActiveRecord::Base.cache_versioning is set to true/false as the cache key is formed containing the dynamic part (updated_at).

Unfortunately, unlike that single commit, this PR does not work as expected :(

TL;DR collection_serializer cache keys don't expire at all using this PR, resulting in stale cache values

For those who are not concerned yet what this PR is trying to solve, I would humbly request to have a quick visit to these links

rails/rails#29092: PR which introduced recyclable cache keys in rails 5.2

https://blog.heroku.com/cache-invalidation-rails-5-2-dalli-store: a recent issue (does not relate to this PR directly, but will help in debugging similar issues)

In short, when ActiveRecord::Base.cache_versioning = true (which will become default from Rails 6), updated_at is stored inside cache value, while cache key remains static over time (which is why its called recyclable cache key), e.g. products/1. In AMS, a cache key would look like: products/1/attributes.

But when ActiveRecord::Base.cache_versioning = false, AMS cache keys would look like products/1-20190110212435047688/attributes

To read/write the latest cache value, we need to provide the :version option in the ActiveSupport::Cache::Store#fetch method, which can be easily retrieved by calling ActiveRecord::Base#cache_version

Here are some examples to highlight the behavior of a recyclable cache key
# returns up-to-date cached value
def cached_product(product)
  Rails.cache.fetch(product.cache_key, version: product.cache_version) do
    product
  end
end

product = Product.first
product.name # => "Product 1"
v1 = product.cache_version # => "20190125020615012258"

# here calling this method will store the current product value inside `product.cache_key` ('products/1')
cached_product(product).name # => "Product 1"

# now reading the cache value with/without the cache version will return v1 value
Rails.cache.read(product.cache_key, version: v1).name # => "Product 1"
Rails.cache.read(product.cache_key).name # => "Product 1"

# let's update the product
product.update(name: 'Product 2')
product.name # => "Product 2"
v2 = product.cache_version # => "20190125022443338534"

# here calling this method will overwrite the cache value with the v2 product value
cached_product(product).name # => "Product 2"

# now reading the cache value with/without the cache version will return v2 value
Rails.cache.read(product.cache_key, version: v2).name # => "Product 2"
Rails.cache.read(product.cache_key).name # => "Product 2"

# but trying to read the cache value with `version: v1` will return nil as it's been overwritten with v2
# hence ensuring that the cache key is recycled properly
Rails.cache.read(product.cache_key, version: v1) # => nil
Rails.cache.read(product.cache_key).name # => "Product 2"
This is what the PR tried to do exactly: https://github.com/rails-api/active_model_serializers/pull/2288/files#diff-b762caa6dd51036307106006dd81600f. So the version info is passed while fetching the cache value.

Now we can discuss the actual issue. For collection serializer, the serialization happens in this sequence:

all the attributes' keys & values are read from the cache using ActiveSupport::Cache::Store#read_multi
# active_model_serializers/lib/active_model/serializer/collection_serializer.rb#25

options[:cached_attributes] ||= ActiveModel::Serializer.cache_read_multi(self, adapter_instance, options[:include_directive])
here it looks up a key in the hash derived above. If the key is found, it returns the value. Otherwise the new value is written to cache using the cache version introduced in this PR.

Here the thing to note is: once the cache is written for some key, it will be read using read_multi as described on point 1. But ActiveSupport::Cache::Store#read_multi unfortunately does not support versioning yet. (does not accept :version option unlike fetch/read). And so we will always get the oldest cache value stored using the recyclable cache key support introduced in this PR.

Summary (assuming a model has been updated multiple times):

Without this PR:

ActiveRecord::Base.cache_versioning = true: cache key looks like products/1/attributes resulting in stale cache value

ActiveRecord::Base.cache_versioning = false: cache key looks like products/1-20190110212435047688/attributes resulting in up-to-date cache value

With this PR:

ActiveRecord::Base.cache_versioning = true: cache key looks like products/1/attributes resulting in stale cache value

ActiveRecord::Base.cache_versioning = false: cache key looks like products/1-20190110212435047688/attributes resulting in up-to-date cache value

With the very first commit in this PR 7d498d2:

ActiveRecord::Base.cache_versioning = true: cache key looks like products/1-20190110212435047688/attributes resulting in up-to-date cache value

ActiveRecord::Base.cache_versioning = false: cache key looks like products/1-20190110212435047688/attributes resulting in up-to-date cache value

So my humble opinion is that we stick with the form of cache key that would embed the version info itself as before i.e products/1-20190110212435047688/attributes which is fixed with the very first commit in this PR 7d498d2 and wait for the rails community until they support recyclable cache key versioning in ActiveSupport::Cache::Store#read_multi

def cached_product(product)
  Rails.cache.fetch(product.cache_key, version: product.cache_version) do
    product
  end
end

I have one question, if you already get product, why use cache? @wasifhossain

wasifhossain · 2019-06-19T18:12:28Z

@shaojunda the example was intended to demonstrate the behavior of a recyclable cache key.

shaojunda · 2019-06-24T06:00:53Z

I have always wanted to try the recyclable cache key, but I have never figured out how to use it. Do you have an example for use in a production environment? @wasifhossain

cintamani changed the title ~~Use #cache_key_with_key when available~~ [WIP] - Use #cache_key_with_key when available Oct 1, 2018

cintamani changed the title ~~[WIP] - Use #cache_key_with_key when available~~ Use #cache_key_with_key when available Oct 1, 2018

cintamani changed the title ~~Use #cache_key_with_key when available~~ Change the fetch method to deal with recyclable key cache strategy Oct 26, 2018

bf4 reviewed Nov 13, 2018

View reviewed changes