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

MemCacheStore: always convert underlying values into an Entry #42559

Merged
merged 1 commit into from Jun 22, 2021

Conversation

@ghiculescu
Copy link
Member

@ghiculescu ghiculescu commented Jun 22, 2021

There's a bug in the Mem Cache Store on 6.1, I haven't confirmed if it exists on main too (since #42025).

We came across the bug when upgrading an app from 6.0 to 6.1 that had recently switched from the dalli_store to the mem_cache_store. petergoldstein/dalli#771 seems similar, but in our case it's specifically to do with caching the value false.

To replicate:

  • Open a Rails console with the dalli_store enabled.
  • Rails.cache.write "test", false
  • Open another Rails console, with the mem_cache_store enabled.
  • Enable the local cache (starting a Rails server would also achieve this): ActiveSupport::Cache::Strategy::LocalCache::LocalCacheRegistry.set_cache_for(Rails.cache.send(:local_cache_key), ActiveSupport::Cache::Strategy::LocalCache::LocalStore.new)
  • Rails.cache.fetch("test") { false }

It will crash with

NoMethodError (undefined method `dup_value!' for false:FalseClass)

The fix is to convert any entry into an Entry, even if it's false.

@ghiculescu
Copy link
Member Author

@ghiculescu ghiculescu commented Jun 22, 2021

@byroot do you think #42025 would fix this on main?

@byroot
Copy link
Member

@byroot byroot commented Jun 22, 2021

do you think #42025 would fix this on main?

Hum, maybe? Best way to know is to check.

byroot
byroot approved these changes Jun 22, 2021
@byroot
Copy link
Member

@byroot byroot commented Jun 22, 2021

CI failures seem totally unrelated. Your fix look good to me. Maybe we should add a regression test though?

@ghiculescu
Copy link
Member Author

@ghiculescu ghiculescu commented Jun 22, 2021

Good idea - regression test added.

There's a bug in the Mem Cache Store on 6.1, I haven't confirmed if it exists on `main` too (since rails#42025).

We came across the bug when upgrading an app from 6.0 to 6.1 that had recently switched from the `dalli_store` to the `mem_cache_store`. petergoldstein/dalli#771 seems similar, but in our case it's specifically to do with caching the value `false`.

To replicate:

- Open a Rails console with the `dalli_store` enabled.
- `Rails.cache.write "test", false`
- Open another Rails console, with the `mem_cache_store` enabled.
- Enable the local cache (starting a Rails server would also achieve this): `ActiveSupport::Cache::Strategy::LocalCache::LocalCacheRegistry.set_cache_for(Rails.cache.send(:local_cache_key), ActiveSupport::Cache::Strategy::LocalCache::LocalStore.new)`
- ` Rails.cache.fetch("test") { false }`

It will crash with `NoMethodError (undefined method `dup_value!' for false:FalseClass)`.

The fix is to convert any entry into an `Entry`, even if it's `nil` or `false`.
@ghiculescu
Copy link
Member Author

@ghiculescu ghiculescu commented Jun 22, 2021

Regression test passes here. It fails on main (with any value, not just false) so I will look into that.

@byroot byroot merged commit 0d54271 into rails:6-1-stable Jun 22, 2021
1 of 2 checks passed
@ghiculescu ghiculescu deleted the patch-4 branch Jun 22, 2021
ghiculescu added a commit to ghiculescu/rails that referenced this issue Jun 22, 2021
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 added a commit to ghiculescu/rails that referenced this issue Jun 22, 2021
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 added a commit to ghiculescu/rails that referenced this issue Jun 22, 2021
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 added a commit to ghiculescu/rails that referenced this issue Jun 22, 2021
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 added a commit to ghiculescu/rails that referenced this issue Jun 22, 2021
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 added a commit to ghiculescu/rails that referenced this issue Jun 22, 2021
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 added a commit to ghiculescu/rails that referenced this issue Jun 22, 2021
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 added a commit to ghiculescu/rails that referenced this issue Jun 22, 2021
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.
@zzak
Copy link
Member

@zzak zzak commented Jun 22, 2021

@ghiculescu @byroot Could you add a changelog? 🙇

@ghiculescu
Copy link
Member Author

@ghiculescu ghiculescu commented Jun 22, 2021

ghiculescu added a commit to ghiculescu/rails that referenced this issue Jun 22, 2021
zzak added a commit that referenced this issue Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants