Bug fixes

CHANGELOG.md

@@ -4,6 +4,23 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## 1.1.6
+
+### Fixed
+
+- Queries on relations with conditions that cannot be represented in the cache key (SQL string conditions, ranges, `not`, `or`, joins, `group`, `having`, `from`, `offset`, locking, or non-hash `find_by` arguments) now bypass the cache instead of silently ignoring those conditions and returning or caching the wrong record.
+- `create_with` values are no longer included in cache keys, so queries using `create_with` can now be cached correctly.
+- Queries inside an open transaction now bypass the cache so that uncommitted data can never be cached. Previously a rolled back transaction could permanently poison the cache with data that was never committed. Transactions created with `joinable: false` (such as Rails transactional test fixtures) still use the cache.
+- Cache invalidation now runs even when caching is disabled. Previously records changed inside a `disable` block (or while caching was disabled globally) left stale entries behind for when caching was re-enabled.
+- Cache key values are now cast through the attribute type, so equivalent values (e.g. `:one` and `"one"`, or `"5"` and `5`) produce the same cache key. Previously such entries could be written under keys that the invalidation callbacks could never delete. This also fixes cache keys for `false` attribute values, which were previously indistinguishable from `nil`.
+- `load_cache` no longer raises an error when caching is disabled and now honors `where` conditions on `cache_by` configurations instead of caching records that do not match them. It also refreshes existing cache entries instead of skipping them.
+- A `cache_by` configuration whose `where` clause does not match a query no longer prevents later configurations from matching, and no longer mutates the query attributes while matching.
+- Calling `cache_by` in a subclass no longer mutates the superclass's cache configuration.
+- Models that include `SupportTableCache` without calling `cache_by` no longer raise an error on `find_by`.
+- Calling `cache_belongs_to` more than once for the same association no longer causes infinite recursion when reading the association.
+- `SupportTableCache::MemoryCache` now synchronizes all access to the underlying hash (previously reads, deletes, and clears were unsynchronized), purges expired entries, and no longer serializes values twice on a cache miss.
+- `SupportTableCache::FiberLocals` now stores state in the fiber's native local storage so that state cannot leak from fibers that are garbage collected while suspended inside a block.
+
 ## 1.1.5
 
 ### Fixed

README.md

@@ -89,6 +89,12 @@ You can also set a cache per class. For instance, you can set an in-memory cache
 
 Note that in-memory caches exist separately within each process and will not be cleared when records are changed in the database. The only way to refresh elements in an in-memory cache is to restart the process or set the `support_table_cache_ttl` value so that the entries will expire.
 
+It is a good idea to always set `support_table_cache_ttl`, even when using a shared cache store. Cache invalidation happens when a record's changes are committed, but a process reading the old row at the same moment can still write the stale value back to the cache just after it was invalidated. A TTL puts an upper bound on how long such a stale entry can survive.
+
+The global cache and disabled settings (`SupportTableCache.cache=` and `SupportTableCache.disable` without a block) are intended to be set during application initialization and are not synchronized for concurrent modification at runtime.
+
+Queries made inside an open database transaction always bypass the cache. This prevents uncommitted data from being cached (which would never be cleared if the transaction were rolled back). Transactions created with `joinable: false`, such as the ones wrapping Rails transactional test fixtures, do not bypass the cache.
+
 ### Disabling Caching
 
 You can disable the cache within a block either globally or only for a specific class. If the cache is disabled, then all queries will pass through to the database.
@@ -108,6 +114,10 @@ SupportTableCache.enable do
 end
 ```
 
+Cache entries are still cleared when records are changed while caching is disabled, so re-enabling the cache will not expose stale data.
+
+Note that the class-level `disable_cache` and `enable_cache` settings apply only to the class they are called on; they do not apply to subclasses in a single table inheritance hierarchy.
+
 ### Caching Belongs to Associations
 
 You can also cache belongs to associations to cacheable models.

VERSION

@@ -1 +1 @@
-1.1.5
+1.1.6

lib/support_table_cache.rb

@@ -28,6 +28,8 @@ module SupportTableCache
     # @api private
     class_attribute :support_table_cache_impl, instance_accessor: false
 
+    self.support_table_cache_by_attributes = []
+
     unless ActiveRecord::Relation.include?(RelationOverride)
       ActiveRecord::Relation.prepend(RelationOverride)
     end
@@ -68,13 +70,15 @@ def enable_cache(&block)
     # @return [void]
     def load_cache
       cache = current_support_table_cache
-      return super if cache.nil?
+      return if cache.nil?
 
       find_each do |record|
-        support_table_cache_by_attributes.each do |attribute_names, case_sensitive|
+        support_table_cache_by_attributes.each do |attribute_names, case_sensitive, where|
+          next unless where.nil? || where.all? { |name, value| record[name] == value }
+
           attributes = record.attributes.slice(*attribute_names)
           cache_key = SupportTableCache.cache_key(self, attributes, attribute_names, case_sensitive)
-          cache.fetch(cache_key, expires_in: support_table_cache_ttl) { record }
+          cache.write(cache_key, record, expires_in: support_table_cache_ttl)
         end
       end
     end
@@ -120,9 +124,10 @@ def cache_by(attributes, case_sensitive: true, where: nil)
         where = where.stringify_keys
       end
 
-      self.support_table_cache_by_attributes ||= []
-      support_table_cache_by_attributes.delete_if { |data| data.first == attributes }
-      self.support_table_cache_by_attributes += [[attributes, case_sensitive, where]]
+      # Build a new array rather than mutating the existing one since the class attribute
+      # value can be shared with the superclass.
+      existing = (support_table_cache_by_attributes || []).reject { |data| data.first == attributes }
+      self.support_table_cache_by_attributes = existing + [[attributes, case_sensitive, where]]
     end
 
     private
@@ -138,6 +143,13 @@ def support_table_cache_disabled?
 
     def current_support_table_cache
       return nil if support_table_cache_disabled?
+      support_table_cache_for_invalidation
+    end
+
+    # The cache used when removing entries. Invalidation must ignore the disabled flag;
+    # otherwise records changed while caching is disabled would leave stale entries behind
+    # for when caching is re-enabled.
+    def support_table_cache_for_invalidation
       SupportTableCache.testing_cache || support_table_cache_impl || SupportTableCache.cache
     end
   end
@@ -241,7 +253,11 @@ def cache_key(klass, attributes, key_attribute_names, case_sensitive)
 
       sorted_attributes = {}
       sorted_names.each do |attribute_name|
-        value = (attributes[attribute_name] || attributes[attribute_name.to_sym])
+        value = (attributes.key?(attribute_name) ? attributes[attribute_name] : attributes[attribute_name.to_sym])
+        # Cast the value through the attribute type so that equivalent values (e.g. a symbol
+        # and a string, or "5" and 5) always produce the same cache key. Otherwise entries
+        # could be written under keys that the invalidation callbacks can never delete.
+        value = klass.type_for_attribute(attribute_name).cast(value)
         if !case_sensitive && (value.is_a?(String) || value.is_a?(Symbol))
           value = value.to_s.downcase
         end
@@ -251,6 +267,44 @@ def cache_key(klass, attributes, key_attribute_names, case_sensitive)
       [klass.name, sorted_attributes]
     end
 
+    # Find the cache key for a query on a set of attributes by matching the attributes
+    # against the cacheable attribute configuration for a class. Returns nil if the
+    # query cannot be cached.
+    #
+    # @param klass [Class] The class that is being queried.
+    # @param attributes [Hash] The query attributes with stringified keys.
+    # @return [Array(String, Hash), nil] The cache key or nil if the query is not cacheable.
+    # @api private
+    def cache_key_for_query(klass, attributes)
+      return nil if attributes.blank?
+
+      Array(klass.support_table_cache_by_attributes).each do |attribute_names, case_sensitive, where|
+        where_matched = where.nil? || where.all? { |name, value| attributes.include?(name) && attributes[name] == value }
+        next unless where_matched
+
+        key_attributes = (where ? attributes.except(*where.keys) : attributes)
+        key = cache_key(klass, key_attributes, attribute_names, case_sensitive)
+        return key if key
+      end
+
+      nil
+    end
+
+    # Return true if there is an open transaction on the class' connection. Queries should
+    # not be cached inside a transaction since they could return uncommitted data that would
+    # be invalid if the transaction is rolled back. Transactions opened with joinable: false
+    # (i.e. Rails transactional test fixtures) are ignored.
+    #
+    # @param klass [Class] The model class being queried.
+    # @return [Boolean]
+    # @api private
+    def open_transaction?(klass)
+      return false unless klass.connection_pool.active_connection?
+
+      connection = klass.connection
+      connection.transaction_open? && connection.current_transaction.joinable?
+    end
+
     def fiber_local_value(varname)
       @fiber_locals[varname]
     end
@@ -267,7 +321,7 @@ def uncache
     cache_by_attributes = self.class.support_table_cache_by_attributes
     return if cache_by_attributes.blank?
 
-    cache = self.class.send(:current_support_table_cache)
+    cache = self.class.send(:support_table_cache_for_invalidation)
     return if cache.nil?
 
     cache_by_attributes.each do |attribute_names, case_sensitive|
@@ -289,7 +343,7 @@ def support_table_clear_cache_entries
     cache_by_attributes = self.class.support_table_cache_by_attributes
     return if cache_by_attributes.blank?
 
-    cache = self.class.send(:current_support_table_cache)
+    cache = self.class.send(:support_table_cache_for_invalidation)
     return if cache.nil?
 
     cache_by_attributes.each do |attribute_names, case_sensitive|

lib/support_table_cache/associations.rb

@@ -50,7 +50,9 @@ def #{association_name}_with_cache
             end
           end
 
-          alias_method :#{association_name}_without_cache, :#{association_name}
+          unless method_defined?(:#{association_name}_without_cache) || private_method_defined?(:#{association_name}_without_cache)
+            alias_method :#{association_name}_without_cache, :#{association_name}
+          end
           alias_method :#{association_name}, :#{association_name}_with_cache
         RUBY
       end

lib/support_table_cache/fiber_locals.rb

@@ -1,51 +1,40 @@
 # frozen_string_literal: true
 
 module SupportTableCache
-  # Utility class for managing fiber-local variables. This implementation
-  # does not pollute the global namespace.
+  # Utility class for managing fiber-local variables. All values are stored in a single
+  # hash inside the fiber's native local storage (Thread.current[], which is fiber-local
+  # in Ruby) so the fiber-local namespace is not polluted with individual keys. Because
+  # the state lives on the fiber itself, it is garbage collected along with the fiber
+  # and cannot leak or be picked up by another fiber.
   class FiberLocals
     def initialize
-      @mutex = Mutex.new
-      @locals = {}
+      @locals_key = :"support_table_cache_locals_#{object_id}"
     end
 
     def [](key)
-      fiber_locals = nil
-      @mutex.synchronize do
-        fiber_locals = @locals[Fiber.current.object_id]
-      end
-      return nil if fiber_locals.nil?
-
-      fiber_locals[key]
+      locals = Thread.current[@locals_key]
+      locals[key] if locals
     end
 
     def with(key, value)
-      fiber_id = Fiber.current.object_id
-      fiber_locals = nil
-      previous_value = nil
-      inited_vars = false
-
-      begin
-        @mutex.synchronize do
-          fiber_locals = @locals[fiber_id]
-          if fiber_locals.nil?
-            fiber_locals = {}
-            @locals[fiber_id] = fiber_locals
-            inited_vars = true
-          end
-        end
+      locals = Thread.current[@locals_key]
+      if locals.nil?
+        locals = {}
+        Thread.current[@locals_key] = locals
+      end
 
-        previous_value = fiber_locals[key]
-        fiber_locals[key] = value
+      exists = locals.key?(key)
+      previous_value = locals[key]
+      locals[key] = value
 
+      begin
         yield
       ensure
-        if inited_vars
-          @mutex.synchronize do
-            @locals.delete(fiber_id)
-          end
+        if exists
+          locals[key] = previous_value
         else
-          fiber_locals[key] = previous_value
+          locals.delete(key)
+          Thread.current[@locals_key] = nil if locals.empty?
         end
       end
     end

lib/support_table_cache/find_by_override.rb

@@ -8,26 +8,18 @@ def find_by(*args)
       cache = current_support_table_cache
       return super unless cache
 
-      cache_key = nil
-      attributes = ((args.size == 1 && args.first.is_a?(Hash)) ? args.first.stringify_keys : {})
+      # Only queries by simple attribute equality can be matched against cache keys.
+      return super unless args.size == 1 && args.first.is_a?(Hash)
 
-      if respond_to?(:scope_attributes) && scope_attributes.present?
-        attributes = scope_attributes.stringify_keys.merge(attributes)
-      end
+      # If the class has any scope applied (a default scope or a scoping block), defer to
+      # the relation override, which checks whether the scoped query can be cached.
+      return super if all.values.present?
 
-      if attributes.present?
-        support_table_cache_by_attributes.each do |attribute_names, case_sensitive, where|
-          where&.each do |name, value|
-            if attributes.include?(name) && attributes[name] == value
-              attributes.delete(name)
-            else
-              return super
-            end
-          end
-          cache_key = SupportTableCache.cache_key(self, attributes, attribute_names, case_sensitive)
-          break if cache_key
-        end
-      end
+      # Queries inside a transaction could see uncommitted data that would be invalid
+      # if the transaction is rolled back, so they cannot be cached.
+      return super if SupportTableCache.open_transaction?(self)
+
+      cache_key = SupportTableCache.cache_key_for_query(self, args.first.stringify_keys)
 
       if cache_key
         cache.fetch(cache_key, expires_in: support_table_cache_ttl) { super }

lib/support_table_cache/memory_cache.rb

@@ -23,13 +23,21 @@ def initialize
     # @yield Block to execute to get a new value if the key is not cached.
     # @return [Object, nil] The cached value or the result of the block, or nil if no value is found.
     def fetch(key, expires_in: nil)
-      serialized_value, expire_at = @cache[key]
-      if serialized_value.nil? || (expire_at && expire_at < Process.clock_gettime(Process::CLOCK_MONOTONIC))
+      serialized_value = nil
+      @mutex.synchronize do
+        serialized_value, expire_at = @cache[key]
+        if expire_at && expire_at < Process.clock_gettime(Process::CLOCK_MONOTONIC)
+          @cache.delete(key)
+          serialized_value = nil
+        end
+      end
+
+      if serialized_value.nil?
         value = yield if block_given?
         return nil if value.nil?
-        write(key, value, expires_in: expires_in)
-        serialized_value = Marshal.dump(value)
+        serialized_value = write(key, value, expires_in: expires_in)
       end
+
       Marshal.load(serialized_value)
     end
 
@@ -59,21 +67,29 @@ def write(key, value, expires_in: nil)
       @mutex.synchronize do
         @cache[key] = [serialized_value, expire_at]
       end
+
+      serialized_value
     end
 
     # Delete a value from the cache.
     #
     # @param key [Object] The cache key.
     # @return [void]
     def delete(key)
-      @cache.delete(key)
+      @mutex.synchronize do
+        @cache.delete(key)
+      end
+      nil
     end
 
     # Clear all values from the cache.
     #
     # @return [void]
     def clear
-      @cache.clear
+      @mutex.synchronize do
+        @cache.clear
+      end
+      nil
     end
   end
 end