Skip to content

Conversation

@casperisfine
Copy link
Contributor

Following rails/rails#42649 we need to make some small changes to fix with_local_store.

Now ActiveSupport use the local store as a "payload" store (e.g. it contain serialized payloads instead of Entry instance). Both upstream MemCacheStore and MemcachedStore don't play so well with this new architecture, because they directly pass Entry instance to the underlying client.

So we basically need to backport the old local store behavior that was duping the cache entry to avoid leaks.

Longer term we should migrate to the new architecture where we no longer rely on memcached serializing objects for us (See: #63).

@casperisfine casperisfine force-pushed the fix-rails-7-local-cache branch from d3da046 to b1e0db9 Compare July 21, 2021 15:28
@casperisfine casperisfine merged commit d74ed76 into master Jul 21, 2021
@casperisfine casperisfine deleted the fix-rails-7-local-cache branch July 21, 2021 16:07
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems July 21, 2021 16:09 Inactive
true
module LocalCacheDup
def with_local_cache
use_temporary_local_cache(LocalStore.new) { yield }
Copy link
Member

@shioyama shioyama Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are not applied when Rails assigns LocalStore in middleware here:

https://github.com/rails/rails/blob/c903dfe618b8ed2b2c6d73b162a49fc4d478a855/activesupport/lib/active_support/cache/strategy/local_cache_middleware.rb#L28

            LocalCacheRegistry.set_cache_for(local_cache_key, LocalStore.new)

This means that duping is broken for middleware-applied LocalCache when memcached_store is used, since the duping fixes above are not present in ActiveSupport::Cache::Strategy::LocalCache::LocalStore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. I'll need to use a decorator or something like that instead.

@shopify-shipit shopify-shipit bot temporarily deployed to rubygems July 26, 2023 14:24 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants