From 9f55355bb1461f48b4594610c2845bbe4c5afe61 Mon Sep 17 00:00:00 2001 From: Torben Dannhauer Date: Mon, 26 Oct 2026 23:06:55 +0100 Subject: [PATCH] fix(activesync): hold no DB transaction across backend calls in State_Sql State_Sql kept the collection-lock transaction (and the SELECT ... FOR UPDATE state row lock inside it) open from loadState() until save(). All backend work of a Sync request - importing client changes into Turba/Kronolith/Nag/IMAP, getSyncStamp(), getChanges() - therefore ran inside the ActiveSync state transaction on the shared Horde_Db connection. On PostgreSQL a single failed backend statement (e.g. a Turba UPDATE rejected with SQLSTATE 22001) aborts that transaction; every following state query fails with SQLSTATE 25P02, the request ends with HTTP 500, and the client retries the same change forever. The token-clearing UPDATE in _releaseCollectionLock() failed as well, leaving the lock row to the stale timeout. Horde_Db nests transactions by counter only (no savepoints), so a backend rollback would also silently drop the lock. - _acquireCollectionLock(): commit the check-and-set transaction before returning; the committed token row is the lock. - _releaseCollectionLock(): clear the token in autocommit mode. If the UPDATE fails while a (foreign, aborted) transaction is open, roll it back and retry once. - Read the state row FOR UPDATE in a short, immediately committed transaction (_readStateRow()); _saveSyncStateRow() always runs in its own short transaction. Drop the _stateRowLockHeld/_stateRowLockTxnOwner and _collectionLockTxnOwner machinery that only carried the long transaction. - Unit tests for the new transaction boundaries, including an end-to-end SQLite test where a backend statement fails between loadState() and save(). - Document the locking model in doc/architecture.md. Fixes #109 --- doc/architecture.md | 13 ++ doc/todo.md | 3 + lib/Horde/ActiveSync/State/Sql.php | 185 +++++++----------- .../StateTest/Sql/CollectionLockTest.php | 162 ++++++++++++++- .../StateTest/Sql/ReadonlyLoadStateTest.php | 2 +- .../ActiveSync/StateTest/Sql/RowLockTest.php | 141 ++++++++++--- 6 files changed, 365 insertions(+), 141 deletions(-) diff --git a/doc/architecture.md b/doc/architecture.md index 45287ebc..1d81f359 100644 --- a/doc/architecture.md +++ b/doc/architecture.md @@ -136,6 +136,19 @@ replayed; garbage collection trims older generations. When state and client diverge irrecoverably, the handler answers with a sync-key error status and the client restarts from sync key 0 (full resync of that collection). +**Locking without long transactions.** Mutating loads of a collection are +serialized by a token row in `horde_activesync_collection_lock` (acquire = +check-and-set in a transaction that is committed immediately; release = +clear the token; a stale timeout covers crashed workers). The state row is +read `FOR UPDATE` in a short, immediately committed transaction and written +back by `save()` in another one. Between the two the request hands control +to the backend (imports, `getSyncStamp()`, `getChanges()`), and **no +`Horde_Db` transaction may be open at that point**: the backend shares the +connection, `Horde_Db` nests transactions by counter only (no savepoints), +and on PostgreSQL a single failed backend statement would abort the shared +transaction (SQLSTATE `25P02`) and take every following state query of the +request down with it ([#109](https://github.com/horde/ActiveSync/issues/109)). + ## WBXML layer `Wbxml_Decoder` and `Wbxml_Encoder` implement EAS WBXML directly (no diff --git a/doc/todo.md b/doc/todo.md index 935daea3..71f8bbdb 100644 --- a/doc/todo.md +++ b/doc/todo.md @@ -392,6 +392,9 @@ active backlog; kept here so this file does not resurrect settled work. returning status `12` (SYNC) / `7` (PING) after five ignored responses; escalates to KEYMISMATCH / server error so broken clients can recover. - SQL/Mongo row locks for parallel state access. +- `State_Sql` holds no DB transaction across backend calls: lock + transactions are committed on acquire, so a failed backend statement no + longer aborts the whole Sync on PostgreSQL (#109). - Reject and repair corrupt `sync_data` on load/save. - Separate PING watermark from SYNC modseq (iOS mail loop fix). - Hardened initial-sync ACK and KEYMISMATCH on corrupt state. diff --git a/lib/Horde/ActiveSync/State/Sql.php b/lib/Horde/ActiveSync/State/Sql.php index 2263cc9e..dcd4427f 100644 --- a/lib/Horde/ActiveSync/State/Sql.php +++ b/lib/Horde/ActiveSync/State/Sql.php @@ -119,25 +119,17 @@ class Horde_ActiveSync_State_Sql extends Horde_ActiveSync_State_Base */ protected const COLLECTION_LOCK_TABLE = 'horde_activesync_collection_lock'; - /** - * True while a SELECT ... FOR UPDATE row lock is held for the loaded - * sync_key (released on save() or updateSyncStamp()). - * - * @var boolean - */ - protected $_stateRowLockHeld = false; - - /** - * True if this instance started the transaction that holds the row lock. - * - * @var boolean - */ - protected $_stateRowLockTxnOwner = false; - /** * True while a collection lock is held (released on save(), * updateSyncStamp(), or updateServerIdInState()). * + * The lock is the committed token row in COLLECTION_LOCK_TABLE, not an + * open database transaction: no Horde_Db transaction stays open while + * control is handed to the backend (imports, getSyncStamp(), ...). A + * failed backend statement must only fail that item - on PostgreSQL it + * would otherwise abort the shared transaction (SQLSTATE 25P02) and take + * every following state query of the request down with it. + * * @var boolean */ protected $_collectionLockHeld = false; @@ -149,14 +141,6 @@ class Horde_ActiveSync_State_Sql extends Horde_ActiveSync_State_Base */ protected $_collectionLockToken = null; - /** - * True if this instance started the transaction that holds the collection - * lock. - * - * @var boolean - */ - protected $_collectionLockTxnOwner = false; - /** * Folder id used for the active collection lock, if any. * @@ -209,52 +193,31 @@ public function __construct(array $params = []) } /** - * Roll back any row lock left open when the request ends without save(). + * Release a collection lock left behind when the request ends without + * save(). */ public function __destruct() { - if ($this->_stateRowLockHeld) { - $this->_releaseStateRowLock(false); - } if ($this->_collectionLockHeld) { $this->_releaseCollectionLock(false); } } /** - * Release a row lock opened by _loadState(). + * Read the current sync state row. * - * @param boolean $commit Commit (true) or roll back (false) the lock - * transaction when this instance owns it. - */ - protected function _releaseStateRowLock($commit = false) - { - if (!$this->_stateRowLockHeld) { - return; - } - - if ($this->_stateRowLockTxnOwner) { - try { - if ($commit) { - $this->_db->commitDbTransaction(); - } else { - $this->_db->rollbackDbTransaction(); - } - } catch (Horde_Db_Exception $e) { - $this->_logger->err($e->getMessage()); - } - } - - $this->_stateRowLockHeld = false; - $this->_stateRowLockTxnOwner = false; - } - - /** - * Begin a transaction and lock the current sync state row for update. + * The row is selected FOR UPDATE inside a short transaction that is + * committed as soon as the row has been read: a parallel save() of the + * same sync_key is waited for and its committed row is returned, but no + * transaction is kept open across the request. Serializing mutating + * loads against each other is the job of the collection lock. * + * @return array The state row (sync_data, sync_devid, sync_mod, + * sync_pending). * @throws Horde_ActiveSync_Exception + * @throws Horde_ActiveSync_Exception_StateGone */ - protected function _acquireStateRowLock() + protected function _readStateRow() { $started = false; if (!$this->_db->transactionStarted()) { @@ -287,13 +250,16 @@ protected function _acquireStateRowLock() throw new Horde_ActiveSync_Exception($e); } - if (empty($results)) { - if ($started) { - try { - $this->_db->rollbackDbTransaction(); - } catch (Horde_Db_Exception $e2) { - } + if ($started) { + try { + $this->_db->commitDbTransaction(); + } catch (Horde_Db_Exception $e) { + $this->_logger->err($e->getMessage()); + throw new Horde_ActiveSync_Exception($e); } + } + + if (empty($results)) { $this->_logger->warn( sprintf( 'STATE: Could not find state for synckey %s.', @@ -303,9 +269,6 @@ protected function _acquireStateRowLock() throw new Horde_ActiveSync_Exception_StateGone(); } - $this->_stateRowLockHeld = true; - $this->_stateRowLockTxnOwner = $started; - return $results; } @@ -403,6 +366,11 @@ protected function _ensureCollectionLockRow($user, $devid, $folderid) /** * Acquire an exclusive collection lock using portable SQL only. * + * The check-and-set of the lock token runs in a transaction that is + * committed before this method returns; the committed token row is the + * lock (bounded by STATE_ROW_LOCK_STALE_SECONDS). No transaction stays + * open while the request hands control to the backend. + * * @param string|null $folderId Optional folder id override. * * @throws Horde_ActiveSync_Exception @@ -463,9 +431,15 @@ protected function _acquireCollectionLock($folderId = null) [$token, $now, $user, $devid, $lockFolderid] ); + // Publish the token: the lock must not depend on this + // connection's transaction state once the backend runs. + if ($started) { + $this->_db->commitDbTransaction(); + $started = false; + } + $this->_collectionLockHeld = true; $this->_collectionLockToken = $token; - $this->_collectionLockTxnOwner = $started; $this->_collectionLockFolderId = $lockFolderid; } catch (Throwable $e) { if ($started) { @@ -473,7 +447,6 @@ protected function _acquireCollectionLock($folderId = null) $this->_db->rollbackDbTransaction(); } catch (Horde_Db_Exception $e2) { } - $this->_collectionLockTxnOwner = false; } if ($this->_collectionLockIsBestEffort()) { @@ -495,8 +468,15 @@ protected function _acquireCollectionLock($folderId = null) /** * Release a collection lock acquired by _acquireCollectionLock(). * - * @param boolean $commit Commit (true) or roll back (false) the lock - * transaction when this instance owns it. + * The token is cleared in autocommit mode. If the clearing UPDATE fails + * while a transaction is open on the connection - a backend left it + * behind, or PostgreSQL aborted it after a failed backend statement - + * that transaction is rolled back and the UPDATE is retried once, so the + * lock row does not have to wait for the stale timeout. + * + * @param boolean $commit Kept for the Horde_ActiveSync_State_Base + * contract; the token is cleared either way as + * the lock no longer lives in a transaction. */ protected function _releaseCollectionLock($commit = false) { @@ -505,35 +485,32 @@ protected function _releaseCollectionLock($commit = false) } [$user, $devid, $lockFolderid] = $this->_collectionLockIdentity(); + $sql = 'UPDATE ' . self::COLLECTION_LOCK_TABLE + . ' SET lock_token = NULL, lock_time = NULL' + . ' WHERE sync_user = ? AND sync_devid = ? AND sync_folderid = ?' + . ' AND lock_token = ?'; + $values = [$user, $devid, $lockFolderid, $this->_collectionLockToken]; try { - $this->_db->update( - 'UPDATE ' . self::COLLECTION_LOCK_TABLE - . ' SET lock_token = NULL, lock_time = NULL' - . ' WHERE sync_user = ? AND sync_devid = ? AND sync_folderid = ?' - . ' AND lock_token = ?', - [$user, $devid, $lockFolderid, $this->_collectionLockToken] - ); + $this->_db->update($sql, $values); } catch (Horde_Db_Exception $e) { $this->_logger->err($e->getMessage()); + if ($this->_db->transactionStarted()) { + $this->_logger->warn( + 'STATE: Rolling back a foreign transaction left open on the state connection to release the collection lock.' + ); + $this->_rollbackDbOnFailure(); + try { + $this->_db->update($sql, $values); + } catch (Horde_Db_Exception $e2) { + $this->_logger->err($e2->getMessage()); + } + } } $this->_collectionLockHeld = false; $this->_collectionLockToken = null; $this->_collectionLockFolderId = null; - - if ($this->_collectionLockTxnOwner) { - try { - if ($commit) { - $this->_db->commitDbTransaction(); - } else { - $this->_db->rollbackDbTransaction(); - } - } catch (Horde_Db_Exception $e) { - $this->_logger->err($e->getMessage()); - } - $this->_collectionLockTxnOwner = false; - } } /** @@ -655,9 +632,7 @@ protected function _loadState() // A mutating load may change the state; drop any read-only memo. unset($this->_readonlyStateMemo[$this->_stateFolderId()]); - $this->_releaseStateRowLock(false); - - $results = $this->_acquireStateRowLock(); + $results = $this->_readStateRow(); $this->_loadStateFromResults($results); } @@ -867,7 +842,6 @@ public function updateSyncStamp() ] ); } catch (Horde_Db_Exception $e) { - $this->_releaseStateRowLock(false); $this->_releaseCollectionLock(false); throw new Horde_ActiveSync_Exception($e); } @@ -877,9 +851,6 @@ public function updateSyncStamp() } } - if ($this->_stateRowLockHeld) { - $this->_releaseStateRowLock($updated); - } if ($this->_collectionLockHeld) { $this->_releaseCollectionLock($updated); } @@ -978,9 +949,10 @@ protected function _saveState(array $options = []) /** * Persist sync state using Horde_Db (update, else insert). * - * When _loadState() has acquired a row lock, the save runs in that same - * transaction so concurrent PING/SYNC workers cannot interleave writes to - * sync_data or sync_pending for the same sync_key. + * The write runs in its own short transaction (unless the caller already + * opened one). Mutating SYNC workers for the same folder are serialized + * by the collection lock, so no row lock has to be carried from + * _loadState() to this point. * * @param array $params Column data for horde_activesync_state. * @@ -1001,9 +973,8 @@ protected function _saveSyncStateRow(array $params) ], ]; $started = false; - $lockHeld = $this->_stateRowLockHeld; - if (!$lockHeld && !$this->_db->transactionStarted()) { + if (!$this->_db->transactionStarted()) { $this->_db->beginDbTransaction(); $started = true; } @@ -1011,9 +982,7 @@ protected function _saveSyncStateRow(array $params) try { $updated = $this->_db->updateBlob($this->_syncStateTable, $params, $where); if ($updated) { - if ($lockHeld) { - $this->_releaseStateRowLock(true); - } elseif ($started) { + if ($started) { $this->_db->commitDbTransaction(); } return; @@ -1051,15 +1020,11 @@ protected function _saveSyncStateRow(array $params) } } - if ($lockHeld) { - $this->_releaseStateRowLock(true); - } elseif ($started) { + if ($started) { $this->_db->commitDbTransaction(); } } catch (Horde_Db_Exception $e) { - if ($lockHeld) { - $this->_releaseStateRowLock(false); - } elseif ($started) { + if ($started) { try { $this->_db->rollbackDbTransaction(); } catch (Horde_Db_Exception $e2) { diff --git a/test/unit/Horde/ActiveSync/StateTest/Sql/CollectionLockTest.php b/test/unit/Horde/ActiveSync/StateTest/Sql/CollectionLockTest.php index 54275f65..dcdee4cc 100644 --- a/test/unit/Horde/ActiveSync/StateTest/Sql/CollectionLockTest.php +++ b/test/unit/Horde/ActiveSync/StateTest/Sql/CollectionLockTest.php @@ -21,6 +21,8 @@ use Horde_ActiveSync_Folder_Imap; use Horde_ActiveSync_Log_Logger; use Horde_Db_Adapter_Pdo_Sqlite; +use Horde_Db_Exception; +use Horde_Db_Value_Binary; use Horde_Log_Handler_Null; use ReflectionClass; @@ -57,11 +59,21 @@ public function testAcquireAndReleaseCollectionLockOnSqlite() $acquire->invoke($state); $this->assertTrue($this->_getProperty($state, '_collectionLockHeld')); + // The token is published; no transaction is left open for the + // backend to run in. + $this->assertFalse($db->transactionStarted()); + $row = $db->selectOne( + 'SELECT lock_token, lock_time FROM horde_activesync_collection_lock' + . ' WHERE sync_user = ? AND sync_devid = ? AND sync_folderid = ?', + ['user@example.com', 'device', 'Ftest'] + ); + $this->assertNotNull($row['lock_token']); $release = $this->_method($state, '_releaseCollectionLock'); $release->invoke($state, true); $this->assertFalse($this->_getProperty($state, '_collectionLockHeld')); + $this->assertFalse($db->transactionStarted()); $row = $db->selectOne( 'SELECT lock_token, lock_time FROM horde_activesync_collection_lock' . ' WHERE sync_user = ? AND sync_devid = ? AND sync_folderid = ?', @@ -71,28 +83,165 @@ public function testAcquireAndReleaseCollectionLockOnSqlite() $this->assertNull($row['lock_time']); } - public function testSaveCommitsCollectionLockTransaction() + public function testAcquireCollectionLockCommitsBeforeReturning() + { + $db = $this->createMock(DbAdapterWithTables::class); + $db->method('tables')->willReturn(['horde_activesync_collection_lock']); + $db->method('transactionStarted')->willReturn(false); + $db->method('selectValue')->willReturn(1); + $db->method('selectOne')->willReturn([ + 'lock_token' => null, + 'lock_time' => null, + ]); + // Exactly one transaction, closed by the acquire itself; the token + // set (acquire) and token clear (release) are the two UPDATEs. + $db->expects($this->once())->method('beginDbTransaction'); + $db->expects($this->exactly(2))->method('update')->willReturn(1); + $db->expects($this->once())->method('commitDbTransaction'); + $db->expects($this->never())->method('rollbackDbTransaction'); + + $state = $this->_newState($db); + $this->_setProperty($state, '_type', Horde_ActiveSync::REQUEST_TYPE_SYNC); + $this->_setProperty($state, '_collection', ['id' => 'Ftest']); + + $this->_method($state, '_acquireCollectionLock')->invoke($state); + + $this->assertTrue($this->_getProperty($state, '_collectionLockHeld')); + $this->assertNotNull($this->_getProperty($state, '_collectionLockToken')); + + $this->_method($state, '_releaseCollectionLock')->invoke($state, true); + + $this->assertFalse($this->_getProperty($state, '_collectionLockHeld')); + } + + public function testSaveReleasesCollectionLockInAutocommit() { $db = $this->getMockBuilder('Horde_Db_Adapter') ->disableOriginalConstructor() ->getMock(); - $db->method('transactionStarted')->willReturn(true); + $db->method('transactionStarted')->willReturn(false); + // One short transaction for the state row write only. + $db->expects($this->once())->method('beginDbTransaction'); $db->expects($this->once())->method('updateBlob')->willReturn(true); $db->expects($this->once())->method('commitDbTransaction'); $db->expects($this->never())->method('rollbackDbTransaction'); + // Token clear. + $db->expects($this->once())->method('update')->willReturn(1); $state = $this->_newState($db); $this->_primeForSave($state); - $this->_setProperty($state, '_stateRowLockTxnOwner', false); $this->_setProperty($state, '_collectionLockHeld', true); - $this->_setProperty($state, '_collectionLockTxnOwner', true); $this->_setProperty($state, '_collectionLockToken', 42); $this->_setProperty($state, '_collectionLockFolderId', 'Ftest'); - $db->expects($this->once())->method('update')->willReturn(1); $state->save(); $this->assertFalse($this->_getProperty($state, '_collectionLockHeld')); - $this->assertFalse($this->_getProperty($state, '_stateRowLockHeld')); + } + + /** + * A backend statement that failed inside a transaction leaves a + * PostgreSQL connection in SQLSTATE 25P02: the token-clearing UPDATE + * fails too. The release must roll that transaction back and clear the + * token anyway instead of leaving the row to the stale timeout. + */ + public function testReleaseRollsBackAbortedTransactionAndRetries() + { + $db = $this->getMockBuilder('Horde_Db_Adapter') + ->disableOriginalConstructor() + ->getMock(); + $db->method('transactionStarted')->willReturn(true); + $db->expects($this->exactly(2)) + ->method('update') + ->willReturnOnConsecutiveCalls( + $this->throwException(new Horde_Db_Exception( + 'SQLSTATE[25P02]: In failed sql transaction' + )), + 1 + ); + $db->expects($this->once())->method('rollbackDbTransaction'); + $db->expects($this->never())->method('commitDbTransaction'); + + $state = $this->_newState($db); + $this->_setProperty($state, '_type', Horde_ActiveSync::REQUEST_TYPE_SYNC); + $this->_setProperty($state, '_collection', ['id' => 'Ftest']); + $this->_setProperty($state, '_collectionLockHeld', true); + $this->_setProperty($state, '_collectionLockToken', 42); + $this->_setProperty($state, '_collectionLockFolderId', 'Ftest'); + + $this->_method($state, '_releaseCollectionLock')->invoke($state, false); + + $this->assertFalse($this->_getProperty($state, '_collectionLockHeld')); + $this->assertNull($this->_getProperty($state, '_collectionLockToken')); + } + + /** + * End-to-end on a real adapter: a backend statement failing between + * loadState() and save() must not break the state save or the lock + * release, and the connection must be usable afterwards. + */ + public function testBackendFailureBetweenLoadAndSaveDoesNotPoisonState() + { + if (!extension_loaded('pdo_sqlite')) { + $this->markTestSkipped('PDO SQLite extension is not loaded'); + } + + $migrationDir = dirname(__DIR__, 6) . '/migration/Horde/ActiveSync'; + $db = DbHelper::createSqliteDb([ + 'migrations' => [[ + 'migrationsPath' => $migrationDir, + 'schemaTableName' => 'horde_activesync_schema_info', + ]], + ]); + + $synckey = '{a1b2c3d4-0000-0000-0000-000000000000}2'; + $folder = new Horde_ActiveSync_Folder_Collection('Ftest', Horde_ActiveSync::CLASS_CONTACTS); + $db->insertBlob('horde_activesync_state', [ + 'sync_key' => $synckey, + 'sync_data' => new Horde_Db_Value_Binary(serialize($folder)), + 'sync_devid' => 'device', + 'sync_mod' => 1, + 'sync_folderid' => 'Ftest', + 'sync_user' => 'user@example.com', + 'sync_pending' => '', + 'sync_timestamp' => time(), + ]); + + $state = $this->_newState($db); + $state->loadState( + ['id' => 'Ftest', 'class' => Horde_ActiveSync::CLASS_CONTACTS, 'serverid' => 'Ftest'], + $synckey, + Horde_ActiveSync::REQUEST_TYPE_SYNC, + 'Ftest' + ); + $this->assertTrue($this->_getProperty($state, '_collectionLockHeld')); + $this->assertFalse($db->transactionStarted()); + + // The "backend" fails a statement on the shared connection. + try { + $db->execute('UPDATE no_such_backend_table SET x = 1'); + $this->fail('Expected the backend statement to fail.'); + } catch (Horde_Db_Exception $e) { + // expected + } + + $this->_setProperty($state, '_thisSyncStamp', 2); + $state->save(); + + $this->assertFalse($this->_getProperty($state, '_collectionLockHeld')); + $this->assertFalse($db->transactionStarted()); + $row = $db->selectOne( + 'SELECT lock_token FROM horde_activesync_collection_lock' + . ' WHERE sync_user = ? AND sync_devid = ? AND sync_folderid = ?', + ['user@example.com', 'device', 'Ftest'] + ); + $this->assertNull($row['lock_token']); + $this->assertSame( + '2', + (string) $db->selectValue( + 'SELECT sync_mod FROM horde_activesync_state WHERE sync_key = ?', + [$synckey] + ) + ); } public function testHeldLockThrowsTemporaryFailure() @@ -200,7 +349,6 @@ protected function _primeForSave(Horde_ActiveSync_State_Sql $state) ]); $this->_setProperty($state, '_thisSyncStamp', 100); $this->_setProperty($state, '_changes', null); - $this->_setProperty($state, '_stateRowLockHeld', true); } protected function _setProperty($object, $name, $value) diff --git a/test/unit/Horde/ActiveSync/StateTest/Sql/ReadonlyLoadStateTest.php b/test/unit/Horde/ActiveSync/StateTest/Sql/ReadonlyLoadStateTest.php index 42514901..6cf3b47d 100644 --- a/test/unit/Horde/ActiveSync/StateTest/Sql/ReadonlyLoadStateTest.php +++ b/test/unit/Horde/ActiveSync/StateTest/Sql/ReadonlyLoadStateTest.php @@ -51,7 +51,7 @@ public function testReadonlyLoadIsLockFreeAndMemoized() ); $this->assertFalse($this->_getProperty($state, '_collectionLockHeld')); - $this->assertFalse($this->_getProperty($state, '_stateRowLockHeld')); + $this->assertFalse($db->transactionStarted()); $folderFirst = $this->_getProperty($state, '_folder'); $this->assertInstanceOf(Horde_ActiveSync_Folder_Imap::class, $folderFirst); $this->assertEquals('INBOX', $folderFirst->serverid()); diff --git a/test/unit/Horde/ActiveSync/StateTest/Sql/RowLockTest.php b/test/unit/Horde/ActiveSync/StateTest/Sql/RowLockTest.php index 60fa7348..b51ca6e1 100644 --- a/test/unit/Horde/ActiveSync/StateTest/Sql/RowLockTest.php +++ b/test/unit/Horde/ActiveSync/StateTest/Sql/RowLockTest.php @@ -1,7 +1,12 @@ * @license http://www.horde.org/licenses/gpl GPLv2 @@ -14,15 +19,18 @@ use PHPUnit\Framework\TestCase; use Horde_ActiveSync; +use Horde_ActiveSync_Exception; +use Horde_ActiveSync_Exception_StateGone; use Horde_ActiveSync_State_Sql; use Horde_ActiveSync_Folder_Imap; use Horde_ActiveSync_Log_Logger; +use Horde_Db_Exception; use Horde_Log_Handler_Null; use ReflectionClass; class RowLockTest extends TestCase { - public function testAcquireStateRowLockUsesForUpdate() + public function testReadStateRowSelectsForUpdateAndCommitsImmediately() { $db = $this->getMockBuilder('Horde_Db_Adapter') ->disableOriginalConstructor() @@ -38,24 +46,87 @@ public function testAcquireStateRowLockUsesForUpdate() 'sync_mod' => 1, 'sync_pending' => '', ]); + $db->expects($this->once())->method('commitDbTransaction'); + $db->expects($this->never())->method('rollbackDbTransaction'); $state = $this->_newState($db); $this->_setProperty($state, '_syncKey', '{test-lock}1'); $this->_setProperty($state, '_collection', ['id' => 'Ftest']); - $acquire = $this->_method($state, '_acquireStateRowLock'); - $acquire->invoke($state); + $row = $this->_method($state, '_readStateRow')->invoke($state); - $this->assertTrue($this->_getProperty($state, '_stateRowLockHeld')); - $this->assertTrue($this->_getProperty($state, '_stateRowLockTxnOwner')); + $this->assertSame('device', $row['sync_devid']); } - public function testSaveCommitsRowLockTransaction() + public function testReadStateRowJoinsOpenTransactionWithoutClosingIt() { $db = $this->getMockBuilder('Horde_Db_Adapter') ->disableOriginalConstructor() ->getMock(); $db->method('transactionStarted')->willReturn(true); + $db->expects($this->never())->method('beginDbTransaction'); + $db->expects($this->never())->method('commitDbTransaction'); + $db->expects($this->never())->method('rollbackDbTransaction'); + $db->method('selectOne')->willReturn([ + 'sync_data' => '', + 'sync_devid' => 'device', + 'sync_mod' => 1, + 'sync_pending' => '', + ]); + + $state = $this->_newState($db); + $this->_setProperty($state, '_syncKey', '{test-lock}1'); + $this->_setProperty($state, '_collection', ['id' => 'Ftest']); + + $this->_method($state, '_readStateRow')->invoke($state); + } + + public function testReadStateRowRollsBackOnQueryFailure() + { + $db = $this->getMockBuilder('Horde_Db_Adapter') + ->disableOriginalConstructor() + ->getMock(); + $db->method('transactionStarted')->willReturn(false); + $db->expects($this->once())->method('beginDbTransaction'); + $db->method('selectOne') + ->willThrowException(new Horde_Db_Exception('SQLSTATE[25P02]')); + $db->expects($this->once())->method('rollbackDbTransaction'); + $db->expects($this->never())->method('commitDbTransaction'); + + $state = $this->_newState($db); + $this->_setProperty($state, '_syncKey', '{test-lock}1'); + $this->_setProperty($state, '_collection', ['id' => 'Ftest']); + + $this->expectException(Horde_ActiveSync_Exception::class); + $this->_method($state, '_readStateRow')->invoke($state); + } + + public function testReadStateRowCommitsBeforeReportingMissingState() + { + $db = $this->getMockBuilder('Horde_Db_Adapter') + ->disableOriginalConstructor() + ->getMock(); + $db->method('transactionStarted')->willReturn(false); + $db->expects($this->once())->method('beginDbTransaction'); + $db->method('selectOne')->willReturn(null); + $db->expects($this->once())->method('commitDbTransaction'); + $db->expects($this->never())->method('rollbackDbTransaction'); + + $state = $this->_newState($db); + $this->_setProperty($state, '_syncKey', '{test-lock}1'); + $this->_setProperty($state, '_collection', ['id' => 'Ftest']); + + $this->expectException(Horde_ActiveSync_Exception_StateGone::class); + $this->_method($state, '_readStateRow')->invoke($state); + } + + public function testSaveRunsInItsOwnShortTransaction() + { + $db = $this->getMockBuilder('Horde_Db_Adapter') + ->disableOriginalConstructor() + ->getMock(); + $db->method('transactionStarted')->willReturn(false); + $db->expects($this->once())->method('beginDbTransaction'); $db->expects($this->once())->method('updateBlob')->willReturn(true); $db->expects($this->once())->method('commitDbTransaction'); $db->expects($this->never())->method('rollbackDbTransaction'); @@ -64,27 +135,61 @@ public function testSaveCommitsRowLockTransaction() $this->_primeForSave($state); $state->save(); - $this->assertFalse($this->_getProperty($state, '_stateRowLockHeld')); } - public function testUpdateSyncStampRollsBackUnusedLock() + public function testSaveJoinsCallerTransaction() { $db = $this->getMockBuilder('Horde_Db_Adapter') ->disableOriginalConstructor() ->getMock(); - $db->expects($this->never())->method('update'); + $db->method('transactionStarted')->willReturn(true); + $db->expects($this->never())->method('beginDbTransaction'); + $db->expects($this->once())->method('updateBlob')->willReturn(true); + $db->expects($this->never())->method('commitDbTransaction'); + $db->expects($this->never())->method('rollbackDbTransaction'); + + $state = $this->_newState($db); + $this->_primeForSave($state); + + $state->save(); + } + + public function testSaveRollsBackItsOwnTransactionOnFailure() + { + $db = $this->getMockBuilder('Horde_Db_Adapter') + ->disableOriginalConstructor() + ->getMock(); + $db->method('transactionStarted')->willReturn(false); + $db->expects($this->once())->method('beginDbTransaction'); + $db->method('updateBlob') + ->willThrowException(new Horde_Db_Exception('SQLSTATE[25P02]')); $db->expects($this->once())->method('rollbackDbTransaction'); + $db->expects($this->never())->method('commitDbTransaction'); + + $state = $this->_newState($db); + $this->_primeForSave($state); + + $this->expectException(Horde_ActiveSync_Exception::class); + $state->save(); + } + + public function testUpdateSyncStampBelowThresholdTouchesNoTransaction() + { + $db = $this->getMockBuilder('Horde_Db_Adapter') + ->disableOriginalConstructor() + ->getMock(); + $db->expects($this->never())->method('update'); + $db->expects($this->never())->method('beginDbTransaction'); + $db->expects($this->never())->method('commitDbTransaction'); + $db->expects($this->never())->method('rollbackDbTransaction'); $state = $this->_newState($db); $this->_setProperty($state, '_syncKey', '{test-lock}3'); $this->_setProperty($state, '_collection', ['id' => 'Ftest']); $this->_setProperty($state, '_lastSyncStamp', 10); $this->_setProperty($state, '_thisSyncStamp', 11); - $this->_setProperty($state, '_stateRowLockHeld', true); - $this->_setProperty($state, '_stateRowLockTxnOwner', true); $state->updateSyncStamp(); - $this->assertFalse($this->_getProperty($state, '_stateRowLockHeld')); } protected function _newState($db) @@ -119,8 +224,6 @@ protected function _primeForSave(Horde_ActiveSync_State_Sql $state) ]); $this->_setProperty($state, '_thisSyncStamp', 100); $this->_setProperty($state, '_changes', null); - $this->_setProperty($state, '_stateRowLockHeld', true); - $this->_setProperty($state, '_stateRowLockTxnOwner', true); } protected function _setProperty($object, $name, $value) @@ -131,14 +234,6 @@ protected function _setProperty($object, $name, $value) $prop->setValue($object, $value); } - protected function _getProperty($object, $name) - { - $ref = new ReflectionClass($object); - $prop = $ref->getProperty($name); - $prop->setAccessible(true); - return $prop->getValue($object); - } - protected function _method($object, $name) { $ref = new ReflectionClass($object);