Optimized Cache::Entry should support old dalli_store values #42566
Conversation
|
|
||
| @cache.instance_variable_get(:@data).with { |c| c.set(@cache.send(:normalize_key, key, nil), false, 0, compress: false) } | ||
| @cache.with_local_cache do | ||
| assert_equal false, @cache.read(key) |
There was a problem hiding this comment.
without the fixes in this PR, this raises with
NoMethodError: undefined method `start_with?' for false:FalseClass
/Users/alex/code/rails/activesupport/lib/active_support/cache.rb:851:in `load'
| key = "test-with-value-the-way-the-dalli-store-did" | ||
|
|
||
| @cache.instance_variable_get(:@data).with { |c| c.set(@cache.send(:normalize_key, key, nil), "value", 0, compress: false) } | ||
| assert_equal "value", @cache.read(key) |
There was a problem hiding this comment.
without the fixes in this PR, this raises with
ActiveSupport::Cache::InvalidCacheItemError: Invalid cache prefix: "v", expected "\x00" or "\x01"
/Users/alex/code/rails/activesupport/lib/active_support/cache.rb:858:in `load'
ghiculescu
force-pushed
the
dalli-store-optimised-cache
branch
from
0bd90ea
to
922aea8
Compare
|
@byroot thanks for nudging me to add a test, turns out there was a bigger issue lurking here! Would love your input on this one, hopefully this is the last PR I tag you in today 😆 |
No worries, feel free to tag me as much as you want. |
ghiculescu
force-pushed
the
dalli-store-optimised-cache
branch
from
922aea8
to
6fb66f3
Compare
| # You should use the `raw:` option if you know this is not the case. But this may | ||
| # happen in cases you can't control, eg. if upgrading from an old version of | ||
| # the dalli_store. We return an Entry as a fallback. | ||
| return Entry.new(payload) unless Cache::Coders::Loader.valid_format?(payload) |
There was a problem hiding this comment.
Please just check is_a?(String) here.
In the future if someone were to upgrade incorrectly (e.g deploy new Rails version using the new format immediately), it's important that we fail rather than to return the serialized payload as a value.
There was a problem hiding this comment.
That won't work, this test will still fail if we just check for a string: https://github.com/rails/rails/pull/42566/files#diff-14cab604508ced2a334cad4623993cac14e1d4b55d6b4e1bc681b830924f492aR235-R240
There was a problem hiding this comment.
Your point is valid though 🤔
There was a problem hiding this comment.
I see. Then rather than raise an ArgumentError, we should return nil. It's actually better for people messing their upgrades, they'll just get a few misses. Maybe with a Rails.logger.warn()?
There was a problem hiding this comment.
Yeah, nils are better than cache reads crashing. I'll upgrade it to do that in all cases where it would currently raise.
ghiculescu
force-pushed
the
dalli-store-optimised-cache
branch
3 times, most recently
from
1828ae1
to
dca9200
Compare
| @@ -844,14 +844,18 @@ module Loader | |||
| extend self | |||
|
|
|||
| def load(payload) | |||
| return nil unless payload.is_a?(String) | |||
There was a problem hiding this comment.
I think this line could be surprising enough to warrant a comment.
ghiculescu
force-pushed
the
dalli-store-optimised-cache
branch
from
dca9200
to
b5fcd09
Compare
Same bug as rails#42559, but a very different fix due to rails#42025. To recap, the issue: - While using the `dalli_store`, you set any value in the Rails cache with no expiry. - You change to the `mem_cache_store`. - You upgrade to Rails 7. - You try to read the same cache key you set in step 1, and it crashes on read. rails#42025 was backward compatible with entries written using the `mem_cache_store`, but *not* the `dalli_store` which did not use `ActiveSupport::Cache::Entry`. This PR attempts to fix that.
ghiculescu
force-pushed
the
dalli-store-optimised-cache
branch
from
b5fcd09
to
25b47c2
Compare
|
thanks for all your help! |
Same bug as #42559, but a very different fix due to #42025. To recap, the issue:
dalli_store, you set any value in the Rails cache with no expiry.mem_cache_store.#42025 was backward compatible with entries written using the
mem_cache_store, but not thedalli_storewhich did not useActiveSupport::Cache::Entry. So this is actually worse than #42559 in that it would impact any key that was written by thedalli_storeand then was read by Rails 7.This PR fixes that by rescuing when a value can't be handled by the new coder system and falling back to just returning an
Entry.