diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5d9e094005..76c221c11a 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -127,16 +127,6 @@ *Alex Ghiculescu* -* Add `ActiveRecord::Base.connection.with_advisory_lock`. - - This method allow applications to obtain an exclusive session level advisory lock, - if available, for the duration of the block. - - If another session already have the lock, the method will return `false` and the block will - not be executed. - - *Rafael Mendonça França* - * Removing trailing whitespace when matching columns in `ActiveRecord::Sanitization.disallow_raw_sql!`. diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index ca1cdb2275..75cf7c1c83 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -441,28 +441,11 @@ module ActiveRecord supports_advisory_locks? && @advisory_locks_enabled end - # Obtains an exclusive session level advisory lock, if available, for the duration of the block. - # - # Returns +false+ without executing the block if the lock could not be obtained. - def with_advisory_lock(lock_id, timeout = 0) - lock_acquired = get_advisory_lock(lock_id, timeout) - - if lock_acquired - yield - end - - lock_acquired - ensure - if lock_acquired && !release_advisory_lock(lock_id) - raise ReleaseAdvisoryLockError - end - end - # This is meant to be implemented by the adapters that support advisory # locks # # Return true if we got the lock, otherwise false - def get_advisory_lock(lock_id, timeout = 0) # :nodoc: + def get_advisory_lock(lock_id) # :nodoc: end # This is meant to be implemented by the adapters that support advisory diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index fd9571af1e..4bdb230a28 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -369,7 +369,7 @@ module ActiveRecord true end - def get_advisory_lock(lock_id, timeout = 0) # :nodoc: + def get_advisory_lock(lock_id) # :nodoc: unless lock_id.is_a?(Integer) && lock_id.bit_length <= 63 raise(ArgumentError, "PostgreSQL requires advisory lock ids to be a signed 64 bit integer") end diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb index 74f0cd245a..77171cb559 100644 --- a/activerecord/lib/active_record/errors.rb +++ b/activerecord/lib/active_record/errors.rb @@ -401,13 +401,6 @@ module ActiveRecord class LockWaitTimeout < StatementInvalid end - # ReleaseAdvisoryLockError will be raised when a advisory lock fail to be released. - class ReleaseAdvisoryLockError < StatementInvalid - def initialize(message = "Failed to release advisory lock") - super - end - end - # StatementTimeout will be raised when statement timeout exceeded. class StatementTimeout < QueryAborted end diff --git a/activerecord/lib/active_record/migration.rb b/activerecord/lib/active_record/migration.rb index 6c98ef8bf1..4ea1f07945 100644 --- a/activerecord/lib/active_record/migration.rb +++ b/activerecord/lib/active_record/migration.rb @@ -167,6 +167,7 @@ module ActiveRecord class ConcurrentMigrationError < MigrationError #:nodoc: DEFAULT_MESSAGE = "Cannot run migrations because another migration process is currently running." + RELEASE_LOCK_FAILED_MESSAGE = "Failed to release advisory lock" def initialize(message = DEFAULT_MESSAGE) super @@ -1412,18 +1413,16 @@ module ActiveRecord lock_id = generate_migrator_advisory_lock_id with_advisory_lock_connection do |connection| - result = nil - - got_lock = connection.with_advisory_lock(lock_id) do - load_migrated # reload schema_migrations to be sure it wasn't changed by another process before we got the lock - result = yield - end - + got_lock = connection.get_advisory_lock(lock_id) raise ConcurrentMigrationError unless got_lock - - result - rescue ReleaseAdvisoryLockError => e - raise ConcurrentMigrationError, e.message, e.backtrace + 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 diff --git a/activerecord/test/cases/adapters/mysql2/connection_test.rb b/activerecord/test/cases/adapters/mysql2/connection_test.rb index 2f2cffd3bf..f7e94c160d 100644 --- a/activerecord/test/cases/adapters/mysql2/connection_test.rb +++ b/activerecord/test/cases/adapters/mysql2/connection_test.rb @@ -220,59 +220,8 @@ class Mysql2ConnectionTest < ActiveRecord::Mysql2TestCase "expected release_advisory_lock to return false when there was no lock to release" end - def test_with_advisory_lock - lock_name = "test lock'n'name" - - got_lock = @connection.with_advisory_lock(lock_name) do - assert_equal test_lock_free(lock_name), false, - "expected the test advisory lock to be held but it wasn't" - end - - assert got_lock, "get_advisory_lock should have returned true but it didn't" - - assert test_lock_free(lock_name), "expected the test lock to be available after releasing" - end - - def test_with_advisory_lock_with_an_already_existing_lock - lock_name = "test lock'n'name" - - with_another_process_holding_lock(lock_name) do - assert_equal test_lock_free(lock_name), false, "expected the test advisory lock to be held but it wasn't" - - got_lock = @connection.with_advisory_lock(lock_name) do - flunk "lock should not be acquired" - end - - assert_equal test_lock_free(lock_name), false, "expected the test advisory lock to be held but it wasn't" - - assert_not got_lock, "get_advisory_lock should have returned false but it didn't" - end - end - private def test_lock_free(lock_name) @connection.select_value("SELECT IS_FREE_LOCK(#{@connection.quote(lock_name)})") == 1 end - - def with_another_process_holding_lock(lock_id) - thread_lock = Concurrent::CountDownLatch.new - test_terminated = Concurrent::CountDownLatch.new - - other_process = Thread.new do - conn = ActiveRecord::Base.connection_pool.checkout - conn.get_advisory_lock(lock_id) - thread_lock.count_down - test_terminated.wait # hold the lock open until we tested everything - ensure - conn.release_advisory_lock(lock_id) - ActiveRecord::Base.connection_pool.checkin(conn) - end - - thread_lock.wait # wait until the 'other process' has the lock - - yield - - test_terminated.count_down - other_process.join - end end diff --git a/activerecord/test/cases/adapters/postgresql/connection_test.rb b/activerecord/test/cases/adapters/postgresql/connection_test.rb index 6a5081c3a7..f863233e06 100644 --- a/activerecord/test/cases/adapters/postgresql/connection_test.rb +++ b/activerecord/test/cases/adapters/postgresql/connection_test.rb @@ -203,19 +203,25 @@ module ActiveRecord def test_get_and_release_advisory_lock lock_id = 5295901941911233559 + list_advisory_locks = <<~SQL + SELECT locktype, + (classid::bigint << 32) | objid::bigint AS lock_id + FROM pg_locks + WHERE locktype = 'advisory' + SQL got_lock = @connection.get_advisory_lock(lock_id) assert got_lock, "get_advisory_lock should have returned true but it didn't" - advisory_lock = test_lock_free(lock_id) - assert_equal advisory_lock, false, + advisory_lock = @connection.query(list_advisory_locks).find { |l| l[1] == lock_id } + assert advisory_lock, "expected to find an advisory lock with lock_id #{lock_id} but there wasn't one" released_lock = @connection.release_advisory_lock(lock_id) assert released_lock, "expected release_advisory_lock to return true but it didn't" - advisory_lock = test_lock_free(lock_id) - assert advisory_lock, + advisory_locks = @connection.query(list_advisory_locks).select { |l| l[1] == lock_id } + assert_empty advisory_locks, "expected to have released advisory lock with lock_id #{lock_id} but it was still held" end @@ -228,70 +234,7 @@ module ActiveRecord end end - def test_with_advisory_lock - lock_id = 5295901941911233559 - - got_lock = @connection.with_advisory_lock(lock_id) do - assert_equal test_lock_free(lock_id), false, - "expected to find an advisory lock with lock_id #{lock_id} but there wasn't one" - end - - assert got_lock, "get_advisory_lock should have returned true but it didn't" - - assert test_lock_free(lock_id), "expected to find an advisory lock with lock_id #{lock_id} but there wasn't one" - end - - def test_with_advisory_lock_with_an_already_existing_lock - lock_id = 5295901941911233559 - - with_another_process_holding_lock(lock_id) do - assert_equal test_lock_free(lock_id), false, - "expected to find an advisory lock with lock_id #{lock_id} but there wasn't one" - - got_lock = @connection.with_advisory_lock(lock_id) do - flunk "lock should not be acquired" - end - - assert_equal test_lock_free(lock_id), false, - "expected to find an advisory lock with lock_id #{lock_id} but there wasn't one" - - assert_not got_lock, "get_advisory_lock should have returned false but it didn't" - end - end - private - def test_lock_free(lock_id) - list_advisory_locks = <<~SQL - SELECT locktype, - (classid::bigint << 32) | objid::bigint AS lock_id - FROM pg_locks - WHERE locktype = 'advisory' - SQL - !@connection.query(list_advisory_locks).find { |l| l[1] == lock_id } - end - - def with_another_process_holding_lock(lock_id) - thread_lock = Concurrent::CountDownLatch.new - test_terminated = Concurrent::CountDownLatch.new - - other_process = Thread.new do - conn = ActiveRecord::Base.connection_pool.checkout - conn.get_advisory_lock(lock_id) - thread_lock.count_down - test_terminated.wait # hold the lock open until we tested everything - ensure - conn.release_advisory_lock(lock_id) - ActiveRecord::Base.connection_pool.checkin(conn) - end - - thread_lock.wait # wait until the 'other process' has the lock - - yield - - test_terminated.count_down - other_process.join - end - def with_warning_suppression log_level = @connection.client_min_messages @connection.client_min_messages = "error" diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 3b0e6164aa..1efe57fce6 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -1033,7 +1033,10 @@ class MigrationTest < ActiveRecord::TestCase end end - assert_match(/Failed to release advisory lock/, e.message) + assert_match( + /#{ActiveRecord::ConcurrentMigrationError::RELEASE_LOCK_FAILED_MESSAGE}/, + e.message + ) end end