Postgres sequence_name correction is not thread safe

[andrba]

While reviewing this gem I noticed that f8eefc4 added some changes to the Postgres Adapter. It appears that they are not thread safe.

Steps to reproduce

Given two tenants - test1 and test2, and a Jobseeker model, the following code eventually fails:

def check_sequence_name(tenant)
  Apartment::Tenant.switch(tenant) do
    sequence_name = Jobseeker.sequence_name
    unless sequence_name.to_s.starts_with?(tenant)
      raise "Invalid sequence name for #{tenant}: #{sequence_name}"
    end
  end
end

Thread.new do
  while true do
    check_sequence_name('test1')
  end
end

Thread.new do
  while true do
    check_sequence_name('test2')
  end
end

#<Thread:0x00005557dd7d22d0@(pry):14 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
	4: from (pry):16:in `block in <main>'
	3: from (pry):2:in `check_sequence_name'
	2: from /usr/local/lib/ruby/2.6.0/forwardable.rb:230:in `switch'
	1: from /usr/local/bundle/gems/ros-apartment-2.9.0/lib/apartment/adapters/abstract_adapter.rb:89:in `switch'
(pry):5:in `block in check_sequence_name': Invalid sequence name for test2: test1.jobseekers_id_seq (RuntimeError)

Expected behavior

No exceptions should be raised when running the above test example.

Actual behavior

The reset_sequence_names method modifies sequence names of ActiveRecord::Base.descendants without considering thread safety:

apartment/lib/apartment/adapters/postgresql_adapter.rb

Lines 133 to 149 in f79ec20

	def reset_sequence_names
	# sequence_name contains the schema, so it must be reset after switch
	# There is `reset_sequence_name`, but that method actually goes to the database
	# to find out the new name. Therefore, we do this hack to only unset the name,
	# and it will be dynamically found the next time it is needed
	descendants_to_unset = ActiveRecord::Base.descendants
	.select { |c| c.instance_variable_defined?(:@sequence_name) }
	.reject do |c|
	c.instance_variable_defined?(:@explicit_sequence_name) &&
	c.instance_variable_get(:@explicit_sequence_name)
	end
	descendants_to_unset.each do |c|
	# NOTE: due to this https://github.com/rails-on-services/apartment/issues/81
	# unreproduceable error we're checking before trying to remove it
	c.remove_instance_variable :@sequence_name if c.instance_variable_defined?(:@sequence_name)
	end
	end

System configuration

• Database: Postgres 12.2

• Apartment version: 2.9.0

• Apartment config (in config/initializers/apartment.rb or so):

Apartment.configure do |config|
  config.excluded_models = %w{Tenant}
  config.tenant_names = lambda{ Tenant.pluck(:database) }
  config.persistent_schemas = ['shared_extensions']
end

• Rails (or ActiveRecord) version: 5.2.4

• Ruby version: 2.6.0

P.S. Thank you for putting so much effort into maintaining this gem.

[rpbaltazar]

Thank you for reporting this.
Your description and error reproduction example were quite clear and helped me understanding how could i try to address the issue.
I've ran some tests yesterday with the linked PR and for the scenario you described it seems to work without issues.

[raysuelzer]

Has this been merged? It looks like this gem may not be receiving any updates?

[rpbaltazar]

It has not been merged as i was waiting for some feedback on #143 (comment)

I haven't had much time to dedicate to the gem and the updates I've been doing have been mostly on a need basis from problems i encounter on our use case. I'm always happy to have someone else helping with the maintenance of the gem