From 777bb85758f472401f20739914729a40fb7c8b11 Mon Sep 17 00:00:00 2001 From: Kyle McGrogan Date: Mon, 4 Dec 2017 13:06:59 -0500 Subject: [PATCH 1/5] Use the actual key value for locks Using just the key column names caused the lock to exist on the entire table, instead of just the row for the supplied Doctrine_Record. Modify the locking/finding/unlocking methods to use the actual key values instead. --- lib/Doctrine/Locking/Manager/Pessimistic.php | 34 +++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/lib/Doctrine/Locking/Manager/Pessimistic.php b/lib/Doctrine/Locking/Manager/Pessimistic.php index 97c772b2e..57d2ef33a 100644 --- a/lib/Doctrine/Locking/Manager/Pessimistic.php +++ b/lib/Doctrine/Locking/Manager/Pessimistic.php @@ -106,9 +106,16 @@ public function getLock(Doctrine_Record $record, $userIdent) $gotLock = false; $time = time(); + $objectKey = []; if (is_array($key)) { // Composite key + foreach ($key as $keyName) { + $objectKey[] = $record->get($keyName); + } $key = implode('|', $key); + $objectKey = implode('|', $objectKey); + } else { + $objectKey = $record->get($key); } try { @@ -120,7 +127,7 @@ public function getLock(Doctrine_Record $record, $userIdent) . ' VALUES (:object_type, :object_key, :user_ident, :ts_obtained)'); $stmt->bindParam(':object_type', $objectType); - $stmt->bindParam(':object_key', $key); + $stmt->bindParam(':object_key', $objectKey); $stmt->bindParam(':user_ident', $userIdent); $stmt->bindParam(':ts_obtained', $time); @@ -145,7 +152,7 @@ public function getLock(Doctrine_Record $record, $userIdent) . ' user_ident = :user_ident'); $stmt->bindParam(':ts', $time); $stmt->bindParam(':object_type', $objectType); - $stmt->bindParam(':object_key', $key); + $stmt->bindParam(':object_key', $objectKey); $stmt->bindParam(':user_ident', $lockingUserIdent); $stmt->execute(); } @@ -172,9 +179,16 @@ public function releaseLock(Doctrine_Record $record, $userIdent) $objectType = $record->getTable()->getComponentName(); $key = $record->getTable()->getIdentifier(); + $objectKey = []; if (is_array($key)) { // Composite key + foreach ($key as $keyName) { + $objectKey[] = $record->get($keyName); + } $key = implode('|', $key); + $objectKey = implode('|', $objectKey); + } else { + $objectKey = $record->get($key); } try { @@ -184,7 +198,7 @@ public function releaseLock(Doctrine_Record $record, $userIdent) object_key = :object_key AND user_ident = :user_ident"); $stmt->bindParam(':object_type', $objectType); - $stmt->bindParam(':object_key', $key); + $stmt->bindParam(':object_key', $objectKey); $stmt->bindParam(':user_ident', $userIdent); $stmt->execute(); @@ -242,7 +256,19 @@ public function getLockOwner($lockedRecord) { $objectType = $lockedRecord->getTable()->getComponentName(); $key = $lockedRecord->getTable()->getIdentifier(); - return $this->_getLockingUserIdent($objectType, $key); + + $objectKey = []; + if (is_array($key)) { + // Composite key + foreach ($key as $keyName) { + $objectKey[] = $record->get($keyName); + } + $objectKey = implode('|', $objectKey); + } else { + $objectKey = $record->get($key); + } + + return $this->_getLockingUserIdent($objectType, $objectKey); } /** From fcdd02e23cb56e4f09e8e39e11f3bbb77d39541e Mon Sep 17 00:00:00 2001 From: Kyle McGrogan Date: Mon, 4 Dec 2017 13:11:39 -0500 Subject: [PATCH 2/5] Fix record reference --- lib/Doctrine/Locking/Manager/Pessimistic.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/Locking/Manager/Pessimistic.php b/lib/Doctrine/Locking/Manager/Pessimistic.php index 57d2ef33a..ff8697900 100644 --- a/lib/Doctrine/Locking/Manager/Pessimistic.php +++ b/lib/Doctrine/Locking/Manager/Pessimistic.php @@ -261,11 +261,11 @@ public function getLockOwner($lockedRecord) if (is_array($key)) { // Composite key foreach ($key as $keyName) { - $objectKey[] = $record->get($keyName); + $objectKey[] = $lockedRecord->get($keyName); } $objectKey = implode('|', $objectKey); } else { - $objectKey = $record->get($key); + $objectKey = $lockedRecord->get($key); } return $this->_getLockingUserIdent($objectType, $objectKey); From 32d9989257320241fb6a74125c22c19f631fd63d Mon Sep 17 00:00:00 2001 From: Kyle McGrogan Date: Thu, 14 Dec 2017 09:23:48 -0500 Subject: [PATCH 3/5] Update naming scheme for userIdent We don't want a single user to have lock access, we want to limit it per request. It makes more sense to just pass it a key which _could_ be a user ID, but more generally is just the ID of whatever we want to own the lock, be it a user or a request. --- lib/Doctrine/Locking/Manager/Pessimistic.php | 54 ++++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/lib/Doctrine/Locking/Manager/Pessimistic.php b/lib/Doctrine/Locking/Manager/Pessimistic.php index ff8697900..89e760feb 100644 --- a/lib/Doctrine/Locking/Manager/Pessimistic.php +++ b/lib/Doctrine/Locking/Manager/Pessimistic.php @@ -72,7 +72,7 @@ public function __construct(Doctrine_Connection $conn) 'notnull' => true, 'primary' => true); - $columns['user_ident'] = array('type' => 'string', + $columns['lock_key'] = array('type' => 'string', 'length' => 50, 'notnull' => true); @@ -93,12 +93,12 @@ public function __construct(Doctrine_Connection $conn) * Obtains a lock on a {@link Doctrine_Record} * * @param Doctrine_Record $record The record that has to be locked - * @param mixed $userIdent A unique identifier of the locking user + * @param mixed $lockKey A unique identifier for the lock. * @return boolean TRUE if the locking was successful, FALSE if another user * holds a lock on this record * @throws Doctrine_Locking_Exception If the locking failed due to database errors */ - public function getLock(Doctrine_Record $record, $userIdent) + public function getLock(Doctrine_Record $record, $lockKey) { $objectType = $record->getTable()->getComponentName(); $key = $record->getTable()->getIdentifier(); @@ -123,12 +123,12 @@ public function getLock(Doctrine_Record $record, $userIdent) $this->conn->beginTransaction(); $stmt = $dbh->prepare('INSERT INTO ' . $this->_lockTable - . ' (object_type, object_key, user_ident, timestamp_obtained)' - . ' VALUES (:object_type, :object_key, :user_ident, :ts_obtained)'); + . ' (object_type, object_key, lock_key, timestamp_obtained)' + . ' VALUES (:object_type, :object_key, :lock_key, :ts_obtained)'); $stmt->bindParam(':object_type', $objectType); $stmt->bindParam(':object_key', $objectKey); - $stmt->bindParam(':user_ident', $userIdent); + $stmt->bindParam(':lock_key', $lockKey); $stmt->bindParam(':ts_obtained', $time); try { @@ -141,19 +141,19 @@ public function getLock(Doctrine_Record $record, $userIdent) } if ( ! $gotLock) { - $lockingUserIdent = $this->_getLockingUserIdent($objectType, $key); - if ($lockingUserIdent !== null && $lockingUserIdent == $userIdent) { + $lockingKey = $this->_getLockingKey($objectType, $key); + if ($lockingKey !== null && $lockingKey == $lockKey) { $gotLock = true; // The requesting user already has a lock // Update timestamp $stmt = $dbh->prepare('UPDATE ' . $this->_lockTable . ' SET timestamp_obtained = :ts' . ' WHERE object_type = :object_type AND' . ' object_key = :object_key AND' - . ' user_ident = :user_ident'); + . ' lock_key = :lock_key'); $stmt->bindParam(':ts', $time); $stmt->bindParam(':object_type', $objectType); $stmt->bindParam(':object_key', $objectKey); - $stmt->bindParam(':user_ident', $lockingUserIdent); + $stmt->bindParam(':lock_key', $lockingKey); $stmt->execute(); } } @@ -170,11 +170,11 @@ public function getLock(Doctrine_Record $record, $userIdent) * Releases a lock on a {@link Doctrine_Record} * * @param Doctrine_Record $record The record for which the lock has to be released - * @param mixed $userIdent The unique identifier of the locking user + * @param mixed $lockKey. The unique identifier for the lock * @return boolean TRUE if a lock was released, FALSE if no lock was released * @throws Doctrine_Locking_Exception If the release procedure failed due to database errors */ - public function releaseLock(Doctrine_Record $record, $userIdent) + public function releaseLock(Doctrine_Record $record, $lockKey) { $objectType = $record->getTable()->getComponentName(); $key = $record->getTable()->getIdentifier(); @@ -196,10 +196,10 @@ public function releaseLock(Doctrine_Record $record, $userIdent) $stmt = $dbh->prepare("DELETE FROM $this->_lockTable WHERE object_type = :object_type AND object_key = :object_key AND - user_ident = :user_ident"); + lock_key = :lock_key"); $stmt->bindParam(':object_type', $objectType); $stmt->bindParam(':object_key', $objectKey); - $stmt->bindParam(':user_ident', $userIdent); + $stmt->bindParam(':lock_key', $lockKey); $stmt->execute(); $count = $stmt->rowCount(); @@ -211,14 +211,14 @@ public function releaseLock(Doctrine_Record $record, $userIdent) } /** - * Gets the unique user identifier of a lock + * Gets the unique identifier of a lock * * @param string $objectType The type of the object (component name) * @param mixed $key The unique key of the object. Can be string or array - * @return string The unique user identifier for the specified lock + * @return string The unique identifier for the specified lock * @throws Doctrine_Locking_Exception If the query failed due to database errors */ - private function _getLockingUserIdent($objectType, $key) + private function _getLockingKey($objectType, $key) { if (is_array($key)) { // Composite key @@ -227,7 +227,7 @@ private function _getLockingUserIdent($objectType, $key) try { $dbh = $this->conn->getDbh(); - $stmt = $dbh->prepare('SELECT user_ident FROM ' . $this->_lockTable + $stmt = $dbh->prepare('SELECT lock_key FROM ' . $this->_lockTable . ' WHERE object_type = :object_type AND object_key = :object_key'); $stmt->bindParam(':object_type', $objectType); $stmt->bindParam(':object_key', $key); @@ -237,12 +237,12 @@ private function _getLockingUserIdent($objectType, $key) throw new Doctrine_Locking_Exception("Failed to determine locking user"); } - $userIdent = $stmt->fetchColumn(); + $lockKey = $stmt->fetchColumn(); } catch (PDOException $pdoe) { throw new Doctrine_Locking_Exception($pdoe->getMessage()); } - return $userIdent; + return $lockKey; } /** @@ -268,7 +268,7 @@ public function getLockOwner($lockedRecord) $objectKey = $lockedRecord->get($key); } - return $this->_getLockingUserIdent($objectType, $objectKey); + return $this->_getLockingKey($objectType, $objectKey); } /** @@ -278,11 +278,11 @@ public function getLockOwner($lockedRecord) * * @param integer $age The maximum valid age of locks in seconds * @param string $objectType The type of the object (component name) - * @param mixed $userIdent The unique identifier of the locking user + * @param mixed $lockKey The unique identifier of the lock * @return integer The number of locks that have been released * @throws Doctrine_Locking_Exception If the release process failed due to database errors */ - public function releaseAgedLocks($age = 900, $objectType = null, $userIdent = null) + public function releaseAgedLocks($age = 900, $objectType = null, $lockKey = null) { $age = time() - $age; @@ -294,16 +294,16 @@ public function releaseAgedLocks($age = 900, $objectType = null, $userIdent = nu if ($objectType) { $query .= ' AND object_type = :object_type'; } - if ($userIdent) { - $query .= ' AND user_ident = :user_ident'; + if ($lockKey) { + $query .= ' AND lock_key = :lock_key'; } $stmt = $dbh->prepare($query); $stmt->bindParam(':age', $age); if ($objectType) { $stmt->bindParam(':object_type', $objectType); } - if ($userIdent) { - $stmt->bindParam(':user_ident', $userIdent); + if ($lockKey) { + $stmt->bindParam(':lock_key', $lockKey); } $stmt->execute(); From c7426d86b5d860c6c16c7b298cd41d657e2c9afa Mon Sep 17 00:00:00 2001 From: Michael Kopinsky Date: Mon, 5 Mar 2018 13:31:06 -0500 Subject: [PATCH 4/5] Add unit test locking individual records with Doctrine_Locking_Manager_Pessimistic --- tests/PessimisticLockingTestCase.php | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/PessimisticLockingTestCase.php b/tests/PessimisticLockingTestCase.php index 306067b78..cfeb6e8a1 100644 --- a/tests/PessimisticLockingTestCase.php +++ b/tests/PessimisticLockingTestCase.php @@ -39,15 +39,29 @@ class Doctrine_PessimisticLocking_TestCase extends Doctrine_UnitTestCase * * Creates a locking manager and a test record to work with. */ - public function testInitData() + public function setup() { + parent::setup(); + $this->lockingManager = new Doctrine_Locking_Manager_Pessimistic($this->connection); - + // Create sample data to test on $entry1 = new Forum_Entry(); $entry1->author = 'Bart Simpson'; $entry1->topic = 'I love donuts!'; $entry1->save(); + + $entry2 = new Forum_Entry(); + $entry2->author = 'Bart Simpson'; + $entry2->topic = 'I play saxophone.'; + $entry2->save(); + } + + public function tearDown() + { + parent::tearDown(); + $this->lockingManager->releaseAgedLocks(-1); // age -1 seconds => release all in prep for next test + $this->connection->query("DELETE FROM Forum_Entry WHERE Forum_Entry.author = 'Bart Simpson'"); } public function prepareTables() @@ -73,6 +87,10 @@ public function testLock() $gotLock = $this->lockingManager->getLock($entries[0], 'konstav'); $this->assertFalse($gotLock); + // Test successfully getting a lock on the second forum entry + $gotLock = $this->lockingManager->getLock($entries[1], 'michael'); + $this->assertTrue($gotLock); + // Test release lock $released = $this->lockingManager->releaseLock($entries[0], 'romanb'); $this->assertTrue($released); From 6353e9995e18209900fdd540ee0f659d50c61c4d Mon Sep 17 00:00:00 2001 From: Kyle McGrogan Date: Wed, 4 Apr 2018 12:26:14 -0400 Subject: [PATCH 5/5] Reverting lockKey to userIdent to prevent bc-breakage --- lib/Doctrine/Locking/Manager/Pessimistic.php | 50 ++++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/lib/Doctrine/Locking/Manager/Pessimistic.php b/lib/Doctrine/Locking/Manager/Pessimistic.php index 89e760feb..455a0c426 100644 --- a/lib/Doctrine/Locking/Manager/Pessimistic.php +++ b/lib/Doctrine/Locking/Manager/Pessimistic.php @@ -72,7 +72,7 @@ public function __construct(Doctrine_Connection $conn) 'notnull' => true, 'primary' => true); - $columns['lock_key'] = array('type' => 'string', + $columns['user_ident'] = array('type' => 'string', 'length' => 50, 'notnull' => true); @@ -93,12 +93,12 @@ public function __construct(Doctrine_Connection $conn) * Obtains a lock on a {@link Doctrine_Record} * * @param Doctrine_Record $record The record that has to be locked - * @param mixed $lockKey A unique identifier for the lock. + * @param mixed $userIdent A unique identifier for the lock. * @return boolean TRUE if the locking was successful, FALSE if another user * holds a lock on this record * @throws Doctrine_Locking_Exception If the locking failed due to database errors */ - public function getLock(Doctrine_Record $record, $lockKey) + public function getLock(Doctrine_Record $record, $userIdent) { $objectType = $record->getTable()->getComponentName(); $key = $record->getTable()->getIdentifier(); @@ -123,12 +123,12 @@ public function getLock(Doctrine_Record $record, $lockKey) $this->conn->beginTransaction(); $stmt = $dbh->prepare('INSERT INTO ' . $this->_lockTable - . ' (object_type, object_key, lock_key, timestamp_obtained)' - . ' VALUES (:object_type, :object_key, :lock_key, :ts_obtained)'); + . ' (object_type, object_key, user_ident, timestamp_obtained)' + . ' VALUES (:object_type, :object_key, :user_ident, :ts_obtained)'); $stmt->bindParam(':object_type', $objectType); $stmt->bindParam(':object_key', $objectKey); - $stmt->bindParam(':lock_key', $lockKey); + $stmt->bindParam(':user_ident', $userIdent); $stmt->bindParam(':ts_obtained', $time); try { @@ -141,19 +141,19 @@ public function getLock(Doctrine_Record $record, $lockKey) } if ( ! $gotLock) { - $lockingKey = $this->_getLockingKey($objectType, $key); - if ($lockingKey !== null && $lockingKey == $lockKey) { + $lockingKey = $this->_getUserIdent($objectType, $key); + if ($lockingKey !== null && $lockingKey == $userIdent) { $gotLock = true; // The requesting user already has a lock // Update timestamp $stmt = $dbh->prepare('UPDATE ' . $this->_lockTable . ' SET timestamp_obtained = :ts' . ' WHERE object_type = :object_type AND' . ' object_key = :object_key AND' - . ' lock_key = :lock_key'); + . ' user_ident = :user_ident'); $stmt->bindParam(':ts', $time); $stmt->bindParam(':object_type', $objectType); $stmt->bindParam(':object_key', $objectKey); - $stmt->bindParam(':lock_key', $lockingKey); + $stmt->bindParam(':user_ident', $lockingKey); $stmt->execute(); } } @@ -170,11 +170,11 @@ public function getLock(Doctrine_Record $record, $lockKey) * Releases a lock on a {@link Doctrine_Record} * * @param Doctrine_Record $record The record for which the lock has to be released - * @param mixed $lockKey. The unique identifier for the lock + * @param mixed $userIdent. The unique identifier for the lock * @return boolean TRUE if a lock was released, FALSE if no lock was released * @throws Doctrine_Locking_Exception If the release procedure failed due to database errors */ - public function releaseLock(Doctrine_Record $record, $lockKey) + public function releaseLock(Doctrine_Record $record, $userIdent) { $objectType = $record->getTable()->getComponentName(); $key = $record->getTable()->getIdentifier(); @@ -196,10 +196,10 @@ public function releaseLock(Doctrine_Record $record, $lockKey) $stmt = $dbh->prepare("DELETE FROM $this->_lockTable WHERE object_type = :object_type AND object_key = :object_key AND - lock_key = :lock_key"); + user_ident = :user_ident"); $stmt->bindParam(':object_type', $objectType); $stmt->bindParam(':object_key', $objectKey); - $stmt->bindParam(':lock_key', $lockKey); + $stmt->bindParam(':user_ident', $userIdent); $stmt->execute(); $count = $stmt->rowCount(); @@ -218,7 +218,7 @@ public function releaseLock(Doctrine_Record $record, $lockKey) * @return string The unique identifier for the specified lock * @throws Doctrine_Locking_Exception If the query failed due to database errors */ - private function _getLockingKey($objectType, $key) + private function _getUserIdent($objectType, $key) { if (is_array($key)) { // Composite key @@ -227,7 +227,7 @@ private function _getLockingKey($objectType, $key) try { $dbh = $this->conn->getDbh(); - $stmt = $dbh->prepare('SELECT lock_key FROM ' . $this->_lockTable + $stmt = $dbh->prepare('SELECT user_ident FROM ' . $this->_lockTable . ' WHERE object_type = :object_type AND object_key = :object_key'); $stmt->bindParam(':object_type', $objectType); $stmt->bindParam(':object_key', $key); @@ -237,12 +237,12 @@ private function _getLockingKey($objectType, $key) throw new Doctrine_Locking_Exception("Failed to determine locking user"); } - $lockKey = $stmt->fetchColumn(); + $userIdent = $stmt->fetchColumn(); } catch (PDOException $pdoe) { throw new Doctrine_Locking_Exception($pdoe->getMessage()); } - return $lockKey; + return $userIdent; } /** @@ -268,7 +268,7 @@ public function getLockOwner($lockedRecord) $objectKey = $lockedRecord->get($key); } - return $this->_getLockingKey($objectType, $objectKey); + return $this->_getUserIdent($objectType, $objectKey); } /** @@ -278,11 +278,11 @@ public function getLockOwner($lockedRecord) * * @param integer $age The maximum valid age of locks in seconds * @param string $objectType The type of the object (component name) - * @param mixed $lockKey The unique identifier of the lock + * @param mixed $userIdent The unique identifier of the lock * @return integer The number of locks that have been released * @throws Doctrine_Locking_Exception If the release process failed due to database errors */ - public function releaseAgedLocks($age = 900, $objectType = null, $lockKey = null) + public function releaseAgedLocks($age = 900, $objectType = null, $userIdent = null) { $age = time() - $age; @@ -294,16 +294,16 @@ public function releaseAgedLocks($age = 900, $objectType = null, $lockKey = null if ($objectType) { $query .= ' AND object_type = :object_type'; } - if ($lockKey) { - $query .= ' AND lock_key = :lock_key'; + if ($userIdent) { + $query .= ' AND user_ident = :user_ident'; } $stmt = $dbh->prepare($query); $stmt->bindParam(':age', $age); if ($objectType) { $stmt->bindParam(':object_type', $objectType); } - if ($lockKey) { - $stmt->bindParam(':lock_key', $lockKey); + if ($userIdent) { + $stmt->bindParam(':user_ident', $userIdent); } $stmt->execute();