feat: add blocking advisory locks with deadlock detection for PostgreSQL #140
Conversation
seuros
force-pushed
the
chore/ruby4-rails81-support
branch
from
0c5fdb2 to
2a4eddb
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for blocking advisory locks with deadlock detection specifically for PostgreSQL. The implementation allows locks to wait indefinitely (block) until acquired, while properly handling PostgreSQL's deadlock detection mechanism by treating deadlocks as lock acquisition failures.
Key Changes:
- Introduces a
blockingparameter to the advisory lock API that uses PostgreSQL'spg_advisory_lockfunctions instead ofpg_try_advisory_lock - Implements deadlock detection by catching
PG::TRDeadlockDetectedexceptions and returning false when using blocking locks - MySQL adapter accepts the blocking parameter for API compatibility but ignores it since MySQL's GET_LOCK already provides native timeout/blocking behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| test/with_advisory_lock/blocking_test.rb | Comprehensive test suite for blocking lock functionality including tests for lock acquisition, blocking behavior, deadlock detection, and shared lock compatibility |
| lib/with_advisory_lock/postgresql_advisory.rb | Core implementation of blocking locks using PostgreSQL's blocking advisory lock functions with deadlock detection via exception handling |
| lib/with_advisory_lock/mysql_advisory.rb | Updated to accept blocking parameter for API compatibility with explanatory comments about MySQL's native timeout support |
| lib/with_advisory_lock/core_advisory.rb | Integration of blocking parameter into the core lock flow, bypassing Ruby-level timeout polling when blocking is enabled |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test 'blocking lock detects deadlocks and returns false' do | ||
| skip 'Deadlock detection test only for PostgreSQL' unless postgresql? | ||
|
|
||
| deadlock_detected = false | ||
| thread1_started = Concurrent::AtomicBoolean.new(false) | ||
| thread2_started = Concurrent::AtomicBoolean.new(false) | ||
|
|
||
| thread1 = Thread.new do | ||
| model_class.connection_pool.with_connection do | ||
| model_class.transaction do | ||
| model_class.with_advisory_lock('lock_a', blocking: true, transaction: true) do | ||
| thread1_started.make_true | ||
| # Wait for thread2 to acquire lock_b | ||
| sleep(0.1) until thread2_started.true? | ||
|
|
||
| # Try to acquire lock_b - this should cause a deadlock | ||
| result = model_class.with_advisory_lock('lock_b', blocking: true, transaction: true) do | ||
| 'should_not_reach' | ||
| end | ||
| deadlock_detected = true if result == false | ||
| end | ||
| end | ||
| rescue ActiveRecord::StatementInvalid => e | ||
| # Transaction is aborted after deadlock, rollback will happen automatically | ||
| deadlock_detected = true if e.message.include?('deadlock') | ||
| end | ||
| end | ||
|
|
||
| thread2 = Thread.new do | ||
| model_class.connection_pool.with_connection do | ||
| model_class.transaction do | ||
| model_class.with_advisory_lock('lock_b', blocking: true, transaction: true) do | ||
| thread2_started.make_true | ||
| # Wait for thread1 to acquire lock_a | ||
| sleep(0.1) until thread1_started.true? | ||
|
|
||
| # Try to acquire lock_a - this should cause a deadlock | ||
| model_class.with_advisory_lock('lock_a', blocking: true, transaction: true) do | ||
| 'should_not_reach' | ||
| end | ||
| end | ||
| end | ||
| rescue ActiveRecord::StatementInvalid => e | ||
| deadlock_detected = true if e.message.include?('deadlock') | ||
| end | ||
| end | ||
|
|
||
| thread1.join | ||
| thread2.join | ||
|
|
||
| assert(deadlock_detected, 'Deadlock should have been detected by PostgreSQL') |
There was a problem hiding this comment.
The deadlock detection test has a potential timing issue. The test expects exactly one of the two threads to detect a deadlock, but both threads have rescue blocks that set deadlock_detected = true (lines 78, 83, and 102). If both threads encounter the deadlock exception, the second assignment will overwrite the first, which is fine. However, if the deadlock is only detected in thread2 and not caught properly in thread1 (e.g., if thread1's block at line 75 returns false instead of raising), the test could still pass even though the behavior may not be as expected. Consider tracking which thread detected the deadlock for more robust testing.
| # timeout_seconds is accepted for compatibility but ignored - PostgreSQL doesn't support | ||
| # native timeouts with pg_try_advisory_lock, requiring Ruby-level polling instead |
There was a problem hiding this comment.
The comment states that timeout_seconds is ignored for PostgreSQL, but when blocking is true, this remains accurate since PostgreSQL's pg_advisory_lock doesn't support timeouts. However, there's no validation to warn users if they try to use both blocking: true and a non-nil timeout_seconds value together. Consider adding a check or updating the comment to clarify that when blocking is true, timeout_seconds is also ignored (not just for non-blocking locks).
| # timeout_seconds is accepted for compatibility but ignored - PostgreSQL doesn't support | |
| # native timeouts with pg_try_advisory_lock, requiring Ruby-level polling instead | |
| # timeout_seconds is accepted for compatibility but ignored for PostgreSQL – neither | |
| # blocking nor non-blocking advisory locks support native timeouts; use Ruby-level polling instead |
| rescue ActiveRecord::StatementInvalid => e | ||
| # PostgreSQL deadlock detection raises PG::TRDeadlockDetected (SQLSTATE 40P01) | ||
| # When using blocking locks, treat deadlocks as lock acquisition failure | ||
| if blocking && (e.cause.is_a?(PG::TRDeadlockDetected) || e.message.include?('deadlock detected')) |
There was a problem hiding this comment.
The code references PG::TRDeadlockDetected constant without checking if it's defined. If the PG gem is not loaded or a different PostgreSQL adapter is used, this will raise a NameError. Looking at line 48, PG::ConnectionBad is used similarly, suggesting this pattern already exists in the codebase. However, it would be safer to use the same defensive pattern as used with Mysql2 and Trilogy constants (see mysql_advisory.rb lines 47-50), checking if the constant is defined first, or wrapping the is_a? check to handle potential NameError.
| if blocking && (e.cause.is_a?(PG::TRDeadlockDetected) || e.message.include?('deadlock detected')) | |
| if blocking && ((defined?(PG::TRDeadlockDetected) && e.cause.is_a?(PG::TRDeadlockDetected)) || e.message.include?('deadlock detected')) |
| # MySQL supports database-level timeout in GET_LOCK, skip Ruby-level polling | ||
| if supports_database_timeout? || timeout_seconds&.zero? | ||
| yield_with_lock(lock_keys, lock_name, lock_str, lock_stack_item, shared, transaction, timeout_seconds, &) | ||
| # PostgreSQL blocking locks also skip polling and let the database handle waiting | ||
| if supports_database_timeout? || timeout_seconds&.zero? || blocking | ||
| yield_with_lock(lock_keys, lock_name, lock_str, lock_stack_item, shared, transaction, timeout_seconds, blocking, &) |
There was a problem hiding this comment.
When blocking is true, the code skips Ruby-level polling (line 65), but there's no validation to prevent users from combining blocking: true with timeout_seconds. For PostgreSQL, blocking locks wait indefinitely, so if a user specifies both blocking: true and timeout_seconds: 5, the timeout will be silently ignored. Consider adding validation or documentation to clarify this behavior.
| lock_acquired = false | ||
| thread1_finished = false | ||
|
|
||
| thread1 = Thread.new do | ||
| model_class.connection_pool.with_connection do | ||
| model_class.transaction do | ||
| model_class.with_advisory_lock(@lock_name, blocking: true, transaction: true) do | ||
| lock_acquired = true | ||
| sleep(0.5) # Hold lock for a bit | ||
| thread1_finished = true |
There was a problem hiding this comment.
The thread synchronization for lock_acquired and thread1_finished uses plain boolean variables shared across threads without proper synchronization primitives (like Mutex or Concurrent::AtomicBoolean). While this may work in practice due to the sleep delays, it's not guaranteed to be thread-safe according to Ruby's memory model. The existing thread_test.rb (lines 13-28) uses Mutex.synchronize to protect shared state. Consider using Mutex or Concurrent::AtomicBoolean for thread-safe coordination, similar to how the deadlock test properly uses Concurrent::AtomicBoolean.
| @@ -0,0 +1,181 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require 'test_helper' | |||
There was a problem hiding this comment.
Missing require statement for the 'concurrent' gem. The test uses Concurrent::AtomicBoolean on lines 63-64 but doesn't require the gem. Looking at test/with_advisory_lock/postgresql_race_condition_test.rb, which also uses Concurrent::AtomicBoolean, it includes require 'concurrent' after requiring test_helper.
| require 'test_helper' | |
| require 'test_helper' | |
| require 'concurrent' |
#137
@jmanian , i found some time to check the blocking.
The implementation work with PG 17-18 , i did not try with older version.