Merge pull request #38235 from eileencodes/fix-advisory-lock

Move advisory lock to it's own connection

activerecord/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Store advisory locks on their own named connection.
+
+    Previously advisory locks were taken out against a connection when a migration started. This works fine in single database applications but doesn't work well when migrations need to open new connections which results in the lock getting dropped.
+
+    In order to fix this we are storing the advisory lock on a new connection with the connection specification name `AdisoryLockBase`. The caveat is that we need to maintain at least 2 connections to a database while migrations are running in order to do this.
+
+    *Eileen M. Uchitelle*, *John Crepezzi*
+
 *   Allow schema cache path to be defined in the database configuration file.
 
     For example:
@@ -21,8 +29,7 @@
 *   Deprecate `#default_hash` and it's alias `#[]` on database configurations
 
     Applications should use `configs_for`. `#default_hash` and `#[]` will be removed in 6.2.
-
-    *Eileen M. Uchitelle*, *John Crepezzi*
+=======
 
 *   Add scale support to `ActiveRecord::Validations::NumericalityValidator`.
 

activerecord/lib/active_record.rb

@@ -36,6 +36,7 @@ require "active_record/errors"
 module ActiveRecord
   extend ActiveSupport::Autoload
 
+  autoload :AdvisoryLockBase
   autoload :Base
   autoload :Callbacks
   autoload :Core

activerecord/lib/active_record/advisory_lock_base.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module ActiveRecord
+  # This class is used to create a connection that we can use for advisory
+  # locks. This will take out a "global" lock that can't be accidentally
+  # removed if a new connection is established during a migration.
+  class AdvisoryLockBase < ActiveRecord::Base # :nodoc:
+    self.abstract_class = true
+
+    self.connection_specification_name = "AdvisoryLockBase"
+
+    class << self
+      def _internal?
+        true
+      end
+    end
+  end
+end

activerecord/lib/active_record/migration.rb

@@ -1373,7 +1373,8 @@ module ActiveRecord
 
       def with_advisory_lock
         lock_id = generate_migrator_advisory_lock_id
-        connection = Base.connection
+        AdvisoryLockBase.establish_connection(ActiveRecord::Base.connection_db_config) unless AdvisoryLockBase.connected?
+        connection = AdvisoryLockBase.connection
         got_lock = connection.get_advisory_lock(lock_id)
         raise ConcurrentMigrationError unless got_lock
         load_migrated # reload schema_migrations to be sure it wasn't changed by another process before we got the lock

activerecord/test/cases/migration_test.rb

@@ -708,6 +708,17 @@ class MigrationTest < ActiveRecord::TestCase
         "without an advisory lock, the Migrator should not make any changes, but it did."
     end
 
+    def test_with_advisory_lock_doesnt_release_closed_connections
+      migration = Class.new(ActiveRecord::Migration::Current).new
+      migrator = ActiveRecord::Migrator.new(:up, [migration], @schema_migration, 100)
+
+      silence_stream($stderr) do
+        migrator.send(:with_advisory_lock) do
+          ActiveRecord::Base.establish_connection :arunit
+        end
+      end
+    end
+
     def test_with_advisory_lock_raises_the_right_error_when_it_fails_to_release_lock
       migration = Class.new(ActiveRecord::Migration::Current).new
       migrator = ActiveRecord::Migrator.new(:up, [migration], @schema_migration, 100)
@@ -716,7 +727,7 @@ class MigrationTest < ActiveRecord::TestCase
       e = assert_raises(ActiveRecord::ConcurrentMigrationError) do
         silence_stream($stderr) do
           migrator.send(:with_advisory_lock) do
-            ActiveRecord::Base.connection.release_advisory_lock(lock_id)
+            ActiveRecord::AdvisoryLockBase.connection.release_advisory_lock(lock_id)
           end
         end
       end