Thread safety when calling fetch_relation

[madejejej]

Hi!

We utilize a global, in-memory cache with TTL for one of our models per process. We also wanted to use identity_cache so that the whole object tree is fetched with one network call.

However, we receive rare errors when we call fetch_association with the following stack trace:

NameError instance variable @dehydrated_relation not defined

/usr/local/bundle/ruby/3.3.0/gems/identity_cache-1.5.6/lib/identity_cache/cached/recursive/association.rb:34
/usr/local/bundle/ruby/3.3.0/gems/identity_cache-1.5.6/lib/identity_cache/cached/recursive/association.rb:19

which points to this piece of code:

identity_cache/lib/identity_cache/cached/recursive/association.rb

Lines 25 to 42 in 313dfbc

	def read(record)
	assoc = record.association(name)
	if !assoc.loaded? && assoc.target.blank? && (record.send(:loaded_by_idc?) || assoc.klass.should_use_cache?)
	if record.instance_variable_defined?(records_variable_name)
	record.instance_variable_get(records_variable_name)
	elsif record.instance_variable_defined?(dehydrated_variable_name)
	dehydrated_target = record.instance_variable_get(dehydrated_variable_name)
	association_target = hydrate_association_target(assoc.klass, dehydrated_target)
	record.remove_instance_variable(dehydrated_variable_name)
	set_with_inverse(record, association_target)
	else
	assoc.load_target
	end
	else
	assoc.load_target
	end
	end

Since the code has a guard clause: record.instance_variable_defined?(dehydrated_variable_name), the only way this can happen is that another thread is concurrently executing the same code and has already removed this instance variable.

Are you open to contributions to fix concurrency issues? So far we've only seen this error popping up dozens of times on ~50M requests, however, there might be more that we haven't seen yet or are failing silently 👀

[madejejej]

FYI we use the following monkey-patch to fix this behavior:

# frozen_string_literal: true

# Open access to the IdentityCache::Cached module
IdentityCache.public_constant(:Cached)

module IdentityCache
  module Cached
    module Recursive
      # Monkey-patch to allow for thread-safe loading of associations
      class Association
        def initialize(name, reflection:)
          super
          @dehydrated_variable_name = :"@dehydrated_#{name}"
          @hydration_mutex = Mutex.new
        end

        def read(record)
          assoc = record.association(name)

          if !assoc.loaded? && assoc.target.blank? && (record.send(:loaded_by_idc?) || assoc.klass.should_use_cache?)
            if record.instance_variable_defined?(records_variable_name)
              record.instance_variable_get(records_variable_name)
            elsif record.instance_variable_defined?(dehydrated_variable_name)
              hydrate_safely(record, assoc)
            else
              assoc.load_target
            end
          else
            assoc.load_target
          end
        end

        private

        # Synchronizes access to the association with a mutex to prevent removing an ivar twice, which raises an error
        def hydrate_safely(record, assoc)
          @hydration_mutex.synchronize do
            if record.instance_variable_defined?(records_variable_name)
              # Another thread may have already hydrated the association - do an early return in this case
              record.instance_variable_get(records_variable_name)
            else
              dehydrated_target = record.instance_variable_get(dehydrated_variable_name)
              association_target = hydrate_association_target(assoc.klass, dehydrated_target)
              record.remove_instance_variable(dehydrated_variable_name)
              set_with_inverse(record, association_target)
            end
          end
        end
      end
    end
  end
end

But perhaps it would be easier to keep the dehydration status on the Association itself and get rid of using remove_instance_variable