1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

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 <john.crepezzi@gmail.com>
This commit is contained in:
eileencodes 2020-01-14 12:41:38 -05:00
parent b0d4413ae7
commit 1ee4a8812f
No known key found for this signature in database
GPG key ID: BA5C575120BBE8DF
5 changed files with 42 additions and 4 deletions

View file

@ -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`.

View file

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

View file

@ -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

View file

@ -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

View file

@ -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