Skip to content

Commit

Permalink
Move advisory locks to own connection handler.
Browse files Browse the repository at this point in the history
Removes the use of `ActiveRecord::AdvisoryLockBase` since it inherits
from `ActiveRecord::Base` and hence share module attributes that are defined in `ActiveRecord::Base`.
This is problematic because establishing connections through
`ActiveRecord::AdvisoryLockBase` can end up changing state of the default
connection handler of `ActiveRecord::Base` leading to unexpected
behaviors in a Rails application.

In the case of #39157,

Running migrations with `rails db:migrate:primary_shard_one` was not working as
the application itself defined the following

```
class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  connects_to shards: {
    default: { writing: :primary },
    shard_one: { writing: :primary_shard_one }
  }
end
```

In the database migrate rake task, the default connection was
established with the database config of `primary_shard_one`. However,
the default connection was altered to that of `primary` because
`ActiveRecord::AdvisoryLockBase.establish_connection` ended up loading
`ApplicationRecord` which calls `connects_to shards:`. Since all we
really need here is just a normal database connection, we can avoid
accidentally altering the default connection handler state during the migration
by creating a custom connection handler used for retrieving a connection.
  • Loading branch information
tgxworld committed Aug 4, 2020
1 parent b39ea7c commit 45add34
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 32 deletions.
1 change: 0 additions & 1 deletion activerecord/lib/active_record.rb
Expand Up @@ -36,7 +36,6 @@
module ActiveRecord
extend ActiveSupport::Autoload

autoload :AdvisoryLockBase
autoload :Base
autoload :Callbacks
autoload :Core
Expand Down
18 changes: 0 additions & 18 deletions activerecord/lib/active_record/advisory_lock_base.rb

This file was deleted.

31 changes: 20 additions & 11 deletions activerecord/lib/active_record/migration.rb
Expand Up @@ -1376,20 +1376,29 @@ def use_advisory_lock?

def with_advisory_lock
lock_id = generate_migrator_advisory_lock_id
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
yield
ensure
if got_lock && !connection.release_advisory_lock(lock_id)
raise ConcurrentMigrationError.new(
ConcurrentMigrationError::RELEASE_LOCK_FAILED_MESSAGE
)

with_advisory_lock_connection do |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
yield
ensure
if got_lock && !connection.release_advisory_lock(lock_id)
raise ConcurrentMigrationError.new(
ConcurrentMigrationError::RELEASE_LOCK_FAILED_MESSAGE
)
end
end
end

def with_advisory_lock_connection
pool = ActiveRecord::ConnectionAdapters::ConnectionHandler.new.establish_connection(
ActiveRecord::Base.connection_db_config
)

pool.with_connection { |connection| yield(connection) }
end

MIGRATOR_SALT = 2053462845
def generate_migrator_advisory_lock_id
db_name_hash = Zlib.crc32(Base.connection.current_database)
Expand Down
6 changes: 4 additions & 2 deletions activerecord/test/cases/migration_test.rb
Expand Up @@ -940,8 +940,10 @@ def test_with_advisory_lock_raises_the_right_error_when_it_fails_to_release_lock

e = assert_raises(ActiveRecord::ConcurrentMigrationError) do
silence_stream($stderr) do
migrator.send(:with_advisory_lock) do
ActiveRecord::AdvisoryLockBase.connection.release_advisory_lock(lock_id)
migrator.stub(:with_advisory_lock_connection, ->(&block) { block.call(ActiveRecord::Base.connection) }) do
migrator.send(:with_advisory_lock) do
ActiveRecord::Base.connection.release_advisory_lock(lock_id)
end
end
end
end
Expand Down

0 comments on commit 45add34

Please sign in to comment.