diff --git a/adr/adr-003-within-session-lock-exception-safety.md b/adr/adr-003-within-session-lock-exception-safety.md index 7ab6646..8f7b23c 100644 --- a/adr/adr-003-within-session-lock-exception-safety.md +++ b/adr/adr-003-within-session-lock-exception-safety.md @@ -56,8 +56,17 @@ PHP sets `previous` only in the exception constructor — there is no `addPrevio The library has no logger dependency and adding one for a single edge case is not justified. Users who need visibility into release failures can wrap `withinSessionLevelLock` in their own try/catch or use `acquireSessionLevelLock` / `releaseSessionLevelLock` directly. +## Known limitation: callback result is lost on release failure + +When the callback succeeds but `releaseSessionLevelLock()` throws, `LockReleaseException` is thrown and the callback's return value is discarded. The caller receives no indication of the callback result. + +This is a consequence of PHP's `finally` semantics — a `throw` in `finally` prevents the `try` block's `return` from completing. The `LockReleaseException` itself serves as a reliable signal that the callback completed successfully (it is only thrown when `$exception === null`), but the return value is not preserved. + +A future improvement could introduce a dedicated exception subclass (e.g. `WithinSessionLockReleaseException`) that carries the callback result. For now, users who need the callback result in this scenario should use `acquireSessionLevelLock()` / `releaseSessionLevelLock()` directly. + ## Consequences - Original exceptions from user callbacks are never masked by release failures. - `PG_ADVISORY_UNLOCK` is not called when the lock was never acquired, preventing reentrant counter corruption. - Release failures are silently suppressed when a callback exception is already in flight — acceptable trade-off given no logger. +- Callback return value is lost when release fails — acceptable trade-off; low-level API is available for callers who need it. diff --git a/src/Postgres/LockHandle/WithinSessionLevelLockHandle.php b/src/Postgres/LockHandle/WithinSessionLevelLockHandle.php new file mode 100644 index 0000000..c668648 --- /dev/null +++ b/src/Postgres/LockHandle/WithinSessionLevelLockHandle.php @@ -0,0 +1,28 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +declare(strict_types=1); + +namespace Cog\DbLocker\Postgres\LockHandle; + +/** + * @template TReturn + */ +final class WithinSessionLevelLockHandle +{ + /** + * @param TReturn $result + */ + public function __construct( + public readonly bool $wasAcquired, + public readonly mixed $result, + ) {} +} diff --git a/src/Postgres/PostgresAdvisoryLocker.php b/src/Postgres/PostgresAdvisoryLocker.php index 54bb017..395c474 100644 --- a/src/Postgres/PostgresAdvisoryLocker.php +++ b/src/Postgres/PostgresAdvisoryLocker.php @@ -19,6 +19,7 @@ use Cog\DbLocker\Postgres\Enum\PostgresLockAccessModeEnum; use Cog\DbLocker\Postgres\Enum\PostgresLockLevelEnum; use Cog\DbLocker\Postgres\LockHandle\SessionLevelLockHandle; +use Cog\DbLocker\Postgres\LockHandle\WithinSessionLevelLockHandle; use Cog\DbLocker\Postgres\LockHandle\TransactionLevelLockHandle; use Cog\DbLocker\TimeoutDuration; @@ -53,24 +54,28 @@ public function acquireTransactionLevelLock( } /** - * Acquires a session-level advisory lock and ensures its release after executing the callback. + * Acquires a session-level advisory lock, executes the callback only if acquired, and ensures release. + * + * The callback is invoked only when the lock is successfully acquired. If the lock + * is not acquired (held by another process), the callback is not called and + * a handle with `wasAcquired = false` is returned. * * This method guarantees that the lock is released even if an exception is thrown during execution. * Useful for safely wrapping critical sections that require locking. * - * If the lock was not acquired (i.e., `wasAcquired` is `false`), it is up to the callback - * to decide how to handle the situation (e.g., retry, throw, log, or silently skip). - * * ⚠️ Transaction-level advisory locks are strongly preferred whenever possible, * as they are automatically released at the end of a transaction and are less error-prone. * Use session-level locks only when transactional context is not available. * - * @param callable(SessionLevelLockHandle): TReturn $callback A callback that receives the lock handle. + * @param callable(): TReturn $callback A callback to execute while the lock is held. * @param TimeoutDuration $timeoutDuration Maximum wait time. Use TimeoutDuration::zero() for an immediate (non-blocking) attempt. - * @return TReturn The return value of the callback. + * @return WithinSessionLevelLockHandle Handle with wasAcquired and result of the callback (null if not acquired). * * @throws LockAcquireException If a database error occurs during lock acquisition. NOT thrown for normal lock contention. * @throws LockReleaseException If a database error occurs during lock release (only thrown if no other exception occurred during callback execution). + * ⚠️ A LockReleaseException guarantees that the callback has completed successfully, + * but the callback's return value is lost. If you need access to the callback result + * in this scenario, use acquireSessionLevelLock() / releaseSessionLevelLock() directly. * * @see acquireTransactionLevelLock() for preferred locking strategy. * @@ -82,7 +87,7 @@ public function withinSessionLevelLock( callable $callback, TimeoutDuration $timeoutDuration, PostgresLockAccessModeEnum $accessMode = PostgresLockAccessModeEnum::Exclusive, - ): mixed { + ): WithinSessionLevelLockHandle { $lockHandle = $this->acquireSessionLevelLock( dbConnection: $dbConnection, key: $key, @@ -90,24 +95,34 @@ public function withinSessionLevelLock( accessMode: $accessMode, ); + if (!$lockHandle->wasAcquired) { + return new WithinSessionLevelLockHandle( + wasAcquired: false, + result: null, + ); + } + $exception = null; try { - return $callback($lockHandle); + $result = $callback(); + + return new WithinSessionLevelLockHandle( + wasAcquired: true, + result: $result, + ); } catch (\Throwable $e) { $exception = $e; throw $e; } finally { - if ($lockHandle->wasAcquired) { - try { - $this->releaseSessionLevelLock( - dbConnection: $dbConnection, - key: $key, - accessMode: $accessMode, - ); - } catch (\Throwable $releaseException) { - if ($exception === null) { - throw $releaseException; - } + try { + $this->releaseSessionLevelLock( + dbConnection: $dbConnection, + key: $key, + accessMode: $accessMode, + ); + } catch (\Throwable $releaseException) { + if ($exception === null) { + throw $releaseException; } } } diff --git a/test/Integration/Postgres/PostgresAdvisoryLockerTest.php b/test/Integration/Postgres/PostgresAdvisoryLockerTest.php index 8a3557c..e4511b8 100644 --- a/test/Integration/Postgres/PostgresAdvisoryLockerTest.php +++ b/test/Integration/Postgres/PostgresAdvisoryLockerTest.php @@ -843,7 +843,7 @@ public function testItCanExecuteCodeWithinSessionLock( $y = 3; // WHEN: Executing code within a session-level lock using callback - $result = $locker->withinSessionLevelLock( + $handle = $locker->withinSessionLevelLock( new PdoConnectionAdapter($dbConnection), $lockKey, function () use ($dbConnection, $lockKey, $accessMode, $x, $y): int { @@ -857,7 +857,8 @@ function () use ($dbConnection, $lockKey, $accessMode, $x, $y): int { ); // THEN: Callback should execute successfully and lock should be auto-released - $this->assertSame(5, $result); + $this->assertTrue($handle->wasAcquired); + $this->assertSame(5, $handle->result); $this->assertPgAdvisoryLocksCount(0); $this->assertPgAdvisoryLockMissingInConnection($dbConnection, $lockKey); } @@ -902,11 +903,13 @@ function () use ($dbConnection): never { // THEN: The original exception from callback should be preserved, not masked by release failure $this->assertSame('Original error from callback', $e->getMessage()); } catch (\PDOException $e) { - $this->fail('PDOException from release should not mask the original RuntimeException: ' . $e->getMessage()); + $this->fail( + 'PDOException from release should not mask the original RuntimeException: ' . $e->getMessage(), + ); } } - public function testWithinSessionLevelLockDoesNotReleaseWhenLockWasNotAcquired(): void + public function testWithinSessionLevelLockDoesNotCallCallbackWhenLockWasNotAcquired(): void { // GIVEN: A lock already held by another connection so the second cannot acquire it $locker = $this->initLocker(); @@ -918,22 +921,60 @@ public function testWithinSessionLevelLockDoesNotReleaseWhenLockWasNotAcquired() $lockKey, TimeoutDuration::zero(), ); + $callbackExecuted = false; // WHEN: withinSessionLevelLock is called on second connection (lock not acquired) - $locker->withinSessionLevelLock( + $handle = $locker->withinSessionLevelLock( new PdoConnectionAdapter($dbConnection2), $lockKey, - function ($lockHandle): void { - $this->assertFalse($lockHandle->wasAcquired); + function () use (&$callbackExecuted): void { + $callbackExecuted = true; }, TimeoutDuration::zero(), ); + // THEN: Callback should not be called, handle should indicate lock was not acquired + $this->assertFalse($handle->wasAcquired); + $this->assertNull($handle->result); + $this->assertFalse($callbackExecuted); // THEN: The first connection's lock should still be intact (not decremented by a spurious release) $this->assertPgAdvisoryLocksCount(1); $this->assertPgAdvisoryLockExistsInConnection($dbConnection1, $lockKey); } + public function testWithinSessionLevelLockLosesCallbackResultWhenReleaseFails(): void + { + // GIVEN: A session-level lock acquired via withinSessionLevelLock callback + $locker = $this->initLocker(); + $dbConnection = $this->initPostgresPdoConnection(); + $dbConnection->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION); + $lockKey = PostgresLockKey::create('test'); + + // WHEN: Callback succeeds with a result but connection is killed before release + try { + $locker->withinSessionLevelLock( + new PdoConnectionAdapter($dbConnection), + $lockKey, + function () use ($dbConnection): string { + // Kill own connection's backend to make release fail + $pid = $dbConnection->pgsqlGetPid(); + $killerConnection = $this->initPostgresPdoConnection(); + $killerConnection->exec("SELECT PG_TERMINATE_BACKEND($pid)"); + + return 'important-result'; + }, + TimeoutDuration::zero(), + ); + $this->fail('Expected LockReleaseException was not thrown'); + } catch (\Cog\DbLocker\Exception\LockReleaseException $e) { + // THEN: LockReleaseException is thrown (guaranteeing callback succeeded) + $this->assertStringContainsString('Failed to release lock', $e->getMessage()); + // THEN: But the callback result is lost - this is a known limitation documented in ADR-003 + } catch (\Throwable $e) { + $this->fail('Expected LockReleaseException but got: ' . get_class($e) . ' - ' . $e->getMessage()); + } + } + public function testItSanitizesCommentToPreventSqlInjection(): void { // GIVEN: A lock key with newline that could break SQL comment and inject code