Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make retrieval of cached entry scope to be public. Fixes #45311 #45313

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

poloka
Copy link

@poloka poloka commented Jun 9, 2022

Summary

Making the retrieval of ActiveSupport::Cache::Entry objects public to assist in consumers retrieving a value prior to its deletion if they so wish to perform a retry operation at some future period of time.

Fixes #45311

Other Information

n/a

@poloka
Copy link
Author

poloka commented Jun 9, 2022

There was also a read multi that I will move public as well to allow both reads to be publicly accessible.

@fatkodima
Copy link
Member

I am sure this is not the way to go by making internal methods public.

Have you tried race_condition_ttl setting? It will 1) save an expired entry for some amount of time before 2) getting and overwriting it with a new entry. And while step 2) will fail, step 1) will succeed and you get an entry from cache.

@poloka
Copy link
Author

poloka commented Jun 10, 2022

@fatkodima that doesn't work if the yield block raises an error. There is no opportunity to return the value. I simply modified my provided example and you can see that when the block yields the error there is no opportunity to retrieve and/or use the previous value.
image

@ghiculescu
Copy link
Member

ghiculescu commented Jun 10, 2022

@poloka which cache store are you using?

My questions are based on how the memcached store works, where if an entry is expired in the cache it's also expired in memcached (so it won't even get returned). Which is why I am confused by the idea that fetch wouldn't return an entry, but read_entry would, at the same time.

I'm guessing you're using the file store or memory store, because it seems like that situation is possible and sensible for you.

Using memcached store:

> key = 'foo'
"foo"
> args = { expires_in: 10 }
{
    :expires_in => 10
}
> Rails.cache.fetch(key, args) { 'bing' }
# Dalli::Server#connect localhost:11211
"bing"
> sleep(10)
10
> entry = Rails.cache.send(:read_entry, key, args)
nil # whereas I think you expect this to return an entry

@poloka
Copy link
Author

poloka commented Jun 10, 2022

@ghiculescu my example is using the memcache. You are correct that if you call fetch it deletes the entry if it is expired as I noted in my issue #45311. So it would be completely expected that if you try to perform an execution on read_entry afterwards that the entry will be gone. That is why I said you have to pull it via the read_entry prior to the fetch call in order to retain the value in an exception case in the yield block. I am not expecting the value to be there after the fact.

@ghiculescu
Copy link
Member

But in my example just above I don't call fetch after the sleep. I call read_entry and it returns nil because the value has expired in memcached.

That is why I said you have to pull it via the read_entry prior to the fetch call in order to retain the value in an exception case in the yield block.

Basically I have done this but you can't retain the value by doing this.

@poloka
Copy link
Author

poloka commented Jun 10, 2022

@ghiculescu, I don't believe the use case in your console is the same and I did not experience this behavior within the rails application. I initially thought I was a MemCacheStore but digging into what logic I drop into it looks as if I'm an ActiveSupport::Cache::NullStore object by default (Rails.application.config.cache_store == :null_store). So here in the rails application the logic works:
image

I took your example and I did the same code within the rails c and got your same result; however, I opted to test a boundary and tried retrieving the result immediately.
So with the following code

key = 'foo'
args = { expires_in: 10 }
Rails.cache.fetch(key, args) { puts 'retrieving value'; 'bing' }
Rails.cache.fetch(key, args) { puts 'retrieving value'; 'bing' }
entry = Rails.cache.send(:read_entry, key, args)
puts entry
Rails.application.config.cache_store

It yielded the following:

2.7.2 :001 > key = 'foo'
 => "foo"
2.7.2 :002 > args = { expires_in: 10 }
 => {:expires_in=>10}
2.7.2 :003 > Rails.cache.fetch(key, args) { puts 'retrieving value'; 'bing' }
retrieving value
 => "bing"
2.7.2 :004 > Rails.cache.fetch(key, args) { puts 'retrieving value'; 'bing' }
retrieving value
 => "bing"
2.7.2 :005 > entry = Rails.cache.send(:read_entry, key, args)
 => nil
2.7.2 :006 > puts entry

 => nil
2.7.2 :007 > Rails.application.config.cache_store
 => :null_store

As you can see, the following fetch still executed its yield block as noted by the puts statement and did not pull a cached value. I would suggest attempting this on your console as well to double-check.

@ghiculescu
Copy link
Member

As you can see, the following fetch still executed its yield block as noted by the puts statement and did not pull a cached value. I would suggest attempting this on your console as well to double-check.

Yeah, that's to be expected in the Null store, as it doesn't do any actual caching. So it would always need to retrieve a value. But you probably shouldn't use the Null store in production :)

I would suggest Rails.cache.class to be sure of which cache store you're using btw.

@poloka
Copy link
Author

poloka commented Jun 10, 2022

Thanks @ghiculescu, yeah I wouldn't use :null_store in production. My production.rb is setup for :memory_store.

@poloka
Copy link
Author

poloka commented Jun 10, 2022

And if I run the example with RAILS_ENV=production rails c I get the following as expected

 :001 >
 :002 > key = 'foo'
 => "foo"
 :003 > args = { expires_in: 10 }
 => {:expires_in=>10}
 :004 > Rails.cache.fetch(key, args) { puts 'retrieving value'; 'bing' }
retrieving value
 => "bing"
 :005 > Rails.cache.fetch(key, args) { puts 'retrieving value'; 'bing' }
 => "bing"
 :006 > entry = Rails.cache.send(:read_entry, key, args)
 => #<ActiveSupport::Cache::Entry:0x00007f8afc844818 @value="bing", @version=nil, @created_at=1654880053.508685, @expires_in=10.0>
 :007 > puts entry
#<ActiveSupport::Cache::Entry:0x00007f8afc844818>
 => nil
 :008 > Rails.application.config.cache_store
 => :memory_store
 :009 >

@ghiculescu
Copy link
Member

Thanks @ghiculescu, yeah I wouldn't use :null_store in production. My production.rb is setup for :memory_store.

The memory store isn't the same as the memcached store FYI. I wouldn't recommend it for production either; https://api.rubyonrails.org/classes/ActiveSupport/Cache/MemoryStore.html talks about why.

But yeah what you're seeing makes sense for the memory store as it doesn't have a separate backend, similar to null store.

@ghiculescu
Copy link
Member

I'm going to close this for now as the conversation's split between here and #45311 and that can get confusing.

We can re-open if it seems like there is a bug here or something that requires a PR.

@ghiculescu ghiculescu closed this Jun 13, 2022
@poloka
Copy link
Author

poloka commented Jun 13, 2022

@ghiculescu I do not agree that this PR should be closed. Please reopen this PR as the functionality being requested is still valid.

@poloka
Copy link
Author

poloka commented Jun 13, 2022

The conversations that have been going on the PR should have been on the issue in the first place but I continued the conversation wherever it was occurring.

@poloka
Copy link
Author

poloka commented Jun 13, 2022

Thanks @ghiculescu for reopening!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rails.cache.fetch unable to retrieve previous cached value if expired
3 participants