From 1ee4a8812fcaf2de48e5ab65d7f707c391fce31d Mon Sep 17 00:00:00 2001 From: eileencodes Date: Tue, 14 Jan 2020 12:41:38 -0500 Subject: [PATCH] Move advisory lock to it's own connection This PR moves advisory lock to it's own connection instead of `ActiveRecord::Base` to fix #37748. As a note the issue is present on both mysql and postgres. We don't see it on sqlite3 because sqlite3 doesn't support advisory locks. The underlying problem only appears if: 1) the app is using multiple databases, and therefore establishing a new connetion in the abstract models 2) the app has a migration that loads a model (ex `Post.update_all`) which causes that new connection to get established. This is because when Rails runs migrations the default connections are established, the lock is taken out on the `ActiveRecord::Base` connection. When the migration that calls a model is loaded, a new connection will be established and the lock will automatically be released. When Rails goes to release the lock in the ensure block it will find that the connection has been closed. Even if the connection wasn't closed the lock would no longer exist on that connection. We originally considered checking if the connection was active, but ultimately that would hide that the advisory locks weren't working correctly because there'd be no lock to release. We also considered making the lock more granular - that it only blocked on each migration individually instead of all the migrations for that connection. This might be the right move going forward, but right now multi-db migrations that load models are very broken in Rails 6.0 and master. John and I don't love this fix, it requires a bit too much knowledge of internals and how Rails picks up connections. However, it does fix the issue, makes the lock more global, and makes the lock more resilient to changing connections. Co-authored-by: John Crepezzi --- activerecord/CHANGELOG.md | 11 +++++++++-- activerecord/lib/active_record.rb | 1 + .../lib/active_record/advisory_lock_base.rb | 18 ++++++++++++++++++ activerecord/lib/active_record/migration.rb | 3 ++- activerecord/test/cases/migration_test.rb | 13 ++++++++++++- 5 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 activerecord/lib/active_record/advisory_lock_base.rb diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 08d59008cb..4258fa09e9 100644 --- a/activerecord/CHANGELOG.md +++ b/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`. diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index ca6209e874..34d1eb344d 100644 --- a/activerecord/lib/active_record.rb +++ b/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 diff --git a/activerecord/lib/active_record/advisory_lock_base.rb b/activerecord/lib/active_record/advisory_lock_base.rb new file mode 100644 index 0000000000..7110a1f132 --- /dev/null +++ b/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 diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index dbea4d0676..bb59bec49f 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/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 diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 7868b88b19..227f2dcd2b 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/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