From 61f66d6e300d393a2f8cf7196d6ccb65590e12ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Deruss=C3=A9?= Date: Thu, 23 Mar 2017 21:40:19 +0100 Subject: [PATCH 1/3] Automaticaly release lock on destruction --- src/Symfony/Component/Lock/Factory.php | 2 +- src/Symfony/Component/Lock/Lock.php | 20 ++++++++++++--- .../Component/Lock/Store/SemaphoreStore.php | 2 +- src/Symfony/Component/Lock/Tests/LockTest.php | 25 ++++++++++++++++--- 4 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/Symfony/Component/Lock/Factory.php b/src/Symfony/Component/Lock/Factory.php index c050145f90978..a63c36e12dea7 100644 --- a/src/Symfony/Component/Lock/Factory.php +++ b/src/Symfony/Component/Lock/Factory.php @@ -37,7 +37,7 @@ public function __construct(StoreInterface $store) * Creates a lock for the given resource. * * @param string $resource The resource to lock - * @param float $ttl maximum expected lock duration + * @param float $ttl Maximum expected lock duration in seconds * * @return Lock */ diff --git a/src/Symfony/Component/Lock/Lock.php b/src/Symfony/Component/Lock/Lock.php index d8271569d8c05..1af8ed2484f04 100644 --- a/src/Symfony/Component/Lock/Lock.php +++ b/src/Symfony/Component/Lock/Lock.php @@ -34,9 +34,9 @@ final class Lock implements LockInterface, LoggerAwareInterface private $ttl; /** - * @param Key $key - * @param StoreInterface $store - * @param float|null $ttl + * @param Key $key Resource to lock + * @param StoreInterface $store Store used to handle lock persistence + * @param float|null $ttl Maximum expected lock duration in seconds */ public function __construct(Key $key, StoreInterface $store, $ttl = null) { @@ -47,6 +47,20 @@ public function __construct(Key $key, StoreInterface $store, $ttl = null) $this->logger = new NullLogger(); } + /** + * Automatically release the underlying lock when the object is destructed. + */ + public function __destruct() + { + try { + if ($this->isAcquired()) { + $this->release(); + } + } catch (\Throwable $e) { + } catch (\Exception $e) { + } + } + /** * {@inheritdoc} */ diff --git a/src/Symfony/Component/Lock/Store/SemaphoreStore.php b/src/Symfony/Component/Lock/Store/SemaphoreStore.php index fe9455cccea09..a6cc9ea8b9f1d 100644 --- a/src/Symfony/Component/Lock/Store/SemaphoreStore.php +++ b/src/Symfony/Component/Lock/Store/SemaphoreStore.php @@ -116,7 +116,7 @@ public function delete(Key $key) */ public function putOffExpiration(Key $key, $ttl) { - // do nothing, the flock locks forever. + // do nothing, the semaphore locks forever. } /** diff --git a/src/Symfony/Component/Lock/Tests/LockTest.php b/src/Symfony/Component/Lock/Tests/LockTest.php index 97b376621169a..3d32d46473d89 100644 --- a/src/Symfony/Component/Lock/Tests/LockTest.php +++ b/src/Symfony/Component/Lock/Tests/LockTest.php @@ -103,7 +103,7 @@ public function testIsAquired() $lock = new Lock($key, $store, 10); $store - ->expects($this->once()) + ->expects($this->atLeast(1)) ->method('exists') ->with($key) ->willReturn(true); @@ -123,7 +123,7 @@ public function testRelease() ->with($key); $store - ->expects($this->once()) + ->expects($this->atLeast(1)) ->method('exists') ->with($key) ->willReturn(false); @@ -131,6 +131,23 @@ public function testRelease() $lock->release(); } + public function testReleaseOnDestruction() + { + $key = new Key(uniqid(__METHOD__, true)); + $store = $this->getMockBuilder(StoreInterface::class)->getMock(); + $lock = new Lock($key, $store, 10); + + $store + ->method('exists') + ->willReturnOnConsecutiveCalls(array(true, false)) + ; + $store + ->expects($this->once()) + ->method('delete') + ; + unset($lock); + } + /** * @expectedException \Symfony\Component\Lock\Exception\LockReleasingException */ @@ -141,12 +158,12 @@ public function testReleaseThrowsExceptionIfNotWellDeleted() $lock = new Lock($key, $store, 10); $store - ->expects($this->once()) + ->expects($this->atLeast(1)) ->method('delete') ->with($key); $store - ->expects($this->once()) + ->expects($this->atLeast(1)) ->method('exists') ->with($key) ->willReturn(true); From 14ebf6a4da4f83260ed9f3cec95c2b485c4304c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Deruss=C3=A9?= Date: Fri, 31 Mar 2017 20:56:25 +0200 Subject: [PATCH 2/3] Add a private variable to store lock state --- src/Symfony/Component/Lock/Lock.php | 10 ++++++++-- src/Symfony/Component/Lock/Tests/LockTest.php | 10 ++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/Symfony/Component/Lock/Lock.php b/src/Symfony/Component/Lock/Lock.php index 1af8ed2484f04..c217299012a32 100644 --- a/src/Symfony/Component/Lock/Lock.php +++ b/src/Symfony/Component/Lock/Lock.php @@ -32,6 +32,7 @@ final class Lock implements LockInterface, LoggerAwareInterface private $store; private $key; private $ttl; + private $dirty = false; /** * @param Key $key Resource to lock @@ -53,7 +54,7 @@ public function __construct(Key $key, StoreInterface $store, $ttl = null) public function __destruct() { try { - if ($this->isAcquired()) { + if ($this->dirty && $this->isAcquired()) { $this->release(); } } catch (\Throwable $e) { @@ -73,6 +74,7 @@ public function acquire($blocking = false) $this->store->waitAndSave($this->key); } + $this->dirty = true; $this->logger->info('Successfully acquired the "{resource}" lock.', array('resource' => $this->key)); if ($this->ttl) { @@ -85,6 +87,7 @@ public function acquire($blocking = false) return true; } catch (LockConflictedException $e) { + $this->dirty = false; $this->logger->warning('Failed to acquire the "{resource}" lock. Someone else already acquired the lock.', array('resource' => $this->key)); if ($blocking) { @@ -110,6 +113,7 @@ public function refresh() try { $this->key->resetLifetime(); $this->store->putOffExpiration($this->key, $this->ttl); + $this->dirty = true; if ($this->key->isExpired()) { throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $this->key)); @@ -117,6 +121,7 @@ public function refresh() $this->logger->info('Expiration defined for "{resource}" lock for "{ttl}" seconds.', array('resource' => $this->key, 'ttl' => $this->ttl)); } catch (LockConflictedException $e) { + $this->dirty = false; $this->logger->warning('Failed to define an expiration for the "{resource}" lock, someone else acquired the lock.', array('resource' => $this->key)); throw $e; } catch (\Exception $e) { @@ -130,7 +135,7 @@ public function refresh() */ public function isAcquired() { - return $this->store->exists($this->key); + return $this->dirty = $this->store->exists($this->key); } /** @@ -139,6 +144,7 @@ public function isAcquired() public function release() { $this->store->delete($this->key); + $this->dirty = false; if ($this->store->exists($this->key)) { $this->logger->warning('Failed to release the "{resource}" lock.', array('resource' => $this->key)); diff --git a/src/Symfony/Component/Lock/Tests/LockTest.php b/src/Symfony/Component/Lock/Tests/LockTest.php index 3d32d46473d89..f688134b79421 100644 --- a/src/Symfony/Component/Lock/Tests/LockTest.php +++ b/src/Symfony/Component/Lock/Tests/LockTest.php @@ -103,7 +103,7 @@ public function testIsAquired() $lock = new Lock($key, $store, 10); $store - ->expects($this->atLeast(1)) + ->expects($this->any()) ->method('exists') ->with($key) ->willReturn(true); @@ -123,7 +123,7 @@ public function testRelease() ->with($key); $store - ->expects($this->atLeast(1)) + ->expects($this->once()) ->method('exists') ->with($key) ->willReturn(false); @@ -145,6 +145,8 @@ public function testReleaseOnDestruction() ->expects($this->once()) ->method('delete') ; + + $lock->acquire(false); unset($lock); } @@ -158,12 +160,12 @@ public function testReleaseThrowsExceptionIfNotWellDeleted() $lock = new Lock($key, $store, 10); $store - ->expects($this->atLeast(1)) + ->expects($this->once()) ->method('delete') ->with($key); $store - ->expects($this->atLeast(1)) + ->expects($this->once()) ->method('exists') ->with($key) ->willReturn(true); From 70a01ac2aa3ec21f3a1f6ee9b83756596c946f09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Deruss=C3=A9?= Date: Tue, 11 Jul 2017 23:22:02 +0200 Subject: [PATCH 3/3] Configurable autorelease --- src/Symfony/Component/Lock/Factory.php | 9 ++++---- src/Symfony/Component/Lock/Lock.php | 23 ++++++++++--------- src/Symfony/Component/Lock/Tests/LockTest.php | 21 ++++++++++++++++- 3 files changed, 37 insertions(+), 16 deletions(-) diff --git a/src/Symfony/Component/Lock/Factory.php b/src/Symfony/Component/Lock/Factory.php index a63c36e12dea7..e9fdde97e8b35 100644 --- a/src/Symfony/Component/Lock/Factory.php +++ b/src/Symfony/Component/Lock/Factory.php @@ -36,14 +36,15 @@ public function __construct(StoreInterface $store) /** * Creates a lock for the given resource. * - * @param string $resource The resource to lock - * @param float $ttl Maximum expected lock duration in seconds + * @param string $resource The resource to lock + * @param float $ttl Maximum expected lock duration in seconds + * @param bool $autoRelease Whether to automatically release the lock or not when the lock instance is destroyed * * @return Lock */ - public function createLock($resource, $ttl = 300.0) + public function createLock($resource, $ttl = 300.0, $autoRelease = true) { - $lock = new Lock(new Key($resource), $this->store, $ttl); + $lock = new Lock(new Key($resource), $this->store, $ttl, $autoRelease); $lock->setLogger($this->logger); return $lock; diff --git a/src/Symfony/Component/Lock/Lock.php b/src/Symfony/Component/Lock/Lock.php index c217299012a32..e81878a43725c 100644 --- a/src/Symfony/Component/Lock/Lock.php +++ b/src/Symfony/Component/Lock/Lock.php @@ -32,34 +32,35 @@ final class Lock implements LockInterface, LoggerAwareInterface private $store; private $key; private $ttl; + private $autoRelease; private $dirty = false; /** - * @param Key $key Resource to lock - * @param StoreInterface $store Store used to handle lock persistence - * @param float|null $ttl Maximum expected lock duration in seconds + * @param Key $key Resource to lock + * @param StoreInterface $store Store used to handle lock persistence + * @param float|null $ttl Maximum expected lock duration in seconds + * @param bool $autoRelease Whether to automatically release the lock or not when the lock instance is destroyed */ - public function __construct(Key $key, StoreInterface $store, $ttl = null) + public function __construct(Key $key, StoreInterface $store, $ttl = null, $autoRelease = true) { $this->store = $store; $this->key = $key; $this->ttl = $ttl; + $this->autoRelease = (bool) $autoRelease; $this->logger = new NullLogger(); } /** - * Automatically release the underlying lock when the object is destructed. + * Automatically releases the underlying lock when the object is destructed. */ public function __destruct() { - try { - if ($this->dirty && $this->isAcquired()) { - $this->release(); - } - } catch (\Throwable $e) { - } catch (\Exception $e) { + if (!$this->autoRelease || !$this->dirty || !$this->isAcquired()) { + return; } + + $this->release(); } /** diff --git a/src/Symfony/Component/Lock/Tests/LockTest.php b/src/Symfony/Component/Lock/Tests/LockTest.php index f688134b79421..ece5cf667963a 100644 --- a/src/Symfony/Component/Lock/Tests/LockTest.php +++ b/src/Symfony/Component/Lock/Tests/LockTest.php @@ -106,7 +106,7 @@ public function testIsAquired() ->expects($this->any()) ->method('exists') ->with($key) - ->willReturn(true); + ->will($this->onConsecutiveCalls(true, false)); $this->assertTrue($lock->isAcquired()); } @@ -150,6 +150,25 @@ public function testReleaseOnDestruction() unset($lock); } + public function testNoAutoReleaseWhenNotConfigured() + { + $key = new Key(uniqid(__METHOD__, true)); + $store = $this->getMockBuilder(StoreInterface::class)->getMock(); + $lock = new Lock($key, $store, 10, false); + + $store + ->method('exists') + ->willReturnOnConsecutiveCalls(array(true, false)) + ; + $store + ->expects($this->never()) + ->method('delete') + ; + + $lock->acquire(false); + unset($lock); + } + /** * @expectedException \Symfony\Component\Lock\Exception\LockReleasingException */