Skip to content

[Lock] Check TTL expiration in lock acquisition #22542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/Symfony/Component/Lock/Exception/LockExpiredException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Lock\Exception;

/**
* LockExpiredException is thrown when a lock may conflict due to a TTL expiration.
*
* @author Jérémy Derussé <jeremy@derusse.com>
*/
class LockExpiredException extends \RuntimeException implements ExceptionInterface
{
}
28 changes: 19 additions & 9 deletions src/Symfony/Component/Lock/Key.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
final class Key
{
private $resource;
private $expiringDate;
private $expiringTime;
private $state = array();

/**
Expand Down Expand Up @@ -72,28 +72,38 @@ public function getState($stateKey)
return $this->state[$stateKey];
}

public function resetLifetime()
{
$this->expiringTime = null;
}

/**
* @param float $ttl The expiration delay of locks in seconds.
*/
public function reduceLifetime($ttl)
{
$newExpiringDate = \DateTimeImmutable::createFromFormat('U.u', (string) (microtime(true) + $ttl));
$newTime = microtime(true) + $ttl;

if (null === $this->expiringDate || $newExpiringDate < $this->expiringDate) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to float comparison instead of datetime, due to bug in PHP => https://3v4l.org/bSt3d

$this->expiringDate = $newExpiringDate;
if (null === $this->expiringTime || $this->expiringTime > $newTime) {
$this->expiringTime = $newTime;
}
}

public function resetExpiringDate()
/**
* Returns the remaining lifetime.
*
* @return float|null Remaining lifetime in seconds. Null when the key won't expire.
*/
public function getRemainingLifetime()
{
$this->expiringDate = null;
return null === $this->expiringTime ? null : $this->expiringTime - microtime(true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be null or (float) PHP_MAX_INT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep null with a comment explaining its meaning in the return annotation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}

/**
* @return \DateTimeImmutable
* @return bool
*/
public function getExpiringDate()
public function isExpired()
{
return $this->expiringDate;
return null !== $this->expiringTime && $this->expiringTime <= microtime(true);
}
}
27 changes: 19 additions & 8 deletions src/Symfony/Component/Lock/Lock.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Symfony\Component\Lock\Exception\InvalidArgumentException;
use Symfony\Component\Lock\Exception\LockAcquiringException;
use Symfony\Component\Lock\Exception\LockConflictedException;
use Symfony\Component\Lock\Exception\LockExpiredException;
use Symfony\Component\Lock\Exception\LockReleasingException;

/**
Expand Down Expand Up @@ -64,6 +65,10 @@ public function acquire($blocking = false)
$this->refresh();
}

if ($this->key->isExpired()) {
throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $this->key));
}

return true;
} catch (LockConflictedException $e) {
$this->logger->warning('Failed to acquire the "{resource}" lock. Someone else already acquired the lock.', array('resource' => $this->key));
Expand All @@ -89,8 +94,13 @@ public function refresh()
}

try {
$this->key->resetExpiringDate();
$this->key->resetLifetime();
$this->store->putOffExpiration($this->key, $this->ttl);

if ($this->key->isExpired()) {
throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $this->key));
}

$this->logger->info('Expiration defined for "{resource}" lock for "{ttl}" seconds.', array('resource' => $this->key, 'ttl' => $this->ttl));
} catch (LockConflictedException $e) {
$this->logger->warning('Failed to define an expiration for the "{resource}" lock, someone else acquired the lock.', array('resource' => $this->key));
Expand Down Expand Up @@ -127,15 +137,16 @@ public function release()
*/
public function isExpired()
{
if (null === $expireDate = $this->key->getExpiringDate()) {
return false;
}

return $expireDate <= new \DateTime();
return $this->key->isExpired();
}

public function getExpiringDate()
/**
* Returns the remaining lifetime.
*
* @return float|null Remaining lifetime in seconds. Null when the lock won't expire.
*/
public function getRemainingLifetime()
{
return $this->key->getExpiringDate();
return $this->key->getRemainingLifetime();
}
}
14 changes: 13 additions & 1 deletion src/Symfony/Component/Lock/Store/CombinedStore.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Psr\Log\NullLogger;
use Symfony\Component\Lock\Exception\InvalidArgumentException;
use Symfony\Component\Lock\Exception\LockConflictedException;
use Symfony\Component\Lock\Exception\LockExpiredException;
use Symfony\Component\Lock\Exception\NotSupportedException;
use Symfony\Component\Lock\Key;
use Symfony\Component\Lock\Strategy\StrategyInterface;
Expand Down Expand Up @@ -102,10 +103,17 @@ public function putOffExpiration(Key $key, $ttl)
$successCount = 0;
$failureCount = 0;
$storesCount = count($this->stores);
$expireAt = microtime(true) + $ttl;

foreach ($this->stores as $store) {
try {
$store->putOffExpiration($key, $ttl);
if (0.0 >= $adjustedTtl = $expireAt - microtime(true)) {
$this->logger->warning('Stores took to long to put off the expiration of the "{resource}" lock.', array('resource' => $key, 'store' => $store, 'ttl' => $ttl));
$key->reduceLifetime(0);
break;
}

$store->putOffExpiration($key, $adjustedTtl);
++$successCount;
} catch (\Exception $e) {
$this->logger->warning('One store failed to put off the expiration of the "{resource}" lock.', array('resource' => $key, 'store' => $store, 'exception' => $e));
Expand All @@ -117,6 +125,10 @@ public function putOffExpiration(Key $key, $ttl)
}
}

if ($key->isExpired()) {
throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $key));
}

if ($this->strategy->isMet($successCount, $storesCount)) {
return;
}
Expand Down
16 changes: 11 additions & 5 deletions src/Symfony/Component/Lock/Store/MemcachedStore.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\Lock\Exception\InvalidArgumentException;
use Symfony\Component\Lock\Exception\LockConflictedException;
use Symfony\Component\Lock\Exception\LockExpiredException;
use Symfony\Component\Lock\Key;
use Symfony\Component\Lock\StoreInterface;

Expand Down Expand Up @@ -57,14 +58,15 @@ public function __construct(\Memcached $memcached, $initialTtl = 300)
public function save(Key $key)
{
$token = $this->getToken($key);

$key->reduceLifetime($this->initialTtl);
if ($this->memcached->add((string) $key, $token, (int) ceil($this->initialTtl))) {
return;
if (!$this->memcached->add((string) $key, $token, (int) ceil($this->initialTtl))) {
// the lock is already acquired. It could be us. Let's try to put off.
$this->putOffExpiration($key, $this->initialTtl);
}

// the lock is already acquire. It could be us. Let's try to put off.
$this->putOffExpiration($key, $this->initialTtl);
if ($key->isExpired()) {
throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $key));
}
}

public function waitAndSave(Key $key)
Expand Down Expand Up @@ -107,6 +109,10 @@ public function putOffExpiration(Key $key, $ttl)
if (!$this->memcached->cas($cas, (string) $key, $token, $ttl)) {
throw new LockConflictedException();
}

if ($key->isExpired()) {
throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $key));
}
}

/**
Expand Down
9 changes: 9 additions & 0 deletions src/Symfony/Component/Lock/Store/RedisStore.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\Lock\Exception\InvalidArgumentException;
use Symfony\Component\Lock\Exception\LockConflictedException;
use Symfony\Component\Lock\Exception\LockExpiredException;
use Symfony\Component\Lock\Key;
use Symfony\Component\Lock\StoreInterface;

Expand Down Expand Up @@ -61,6 +62,10 @@ public function save(Key $key)
if (!$this->evaluate($script, (string) $key, array($this->getToken($key), (int) ceil($this->initialTtl * 1000)))) {
throw new LockConflictedException();
}

if ($key->isExpired()) {
throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $key));
}
}

public function waitAndSave(Key $key)
Expand All @@ -85,6 +90,10 @@ public function putOffExpiration(Key $key, $ttl)
if (!$this->evaluate($script, (string) $key, array($this->getToken($key), (int) ceil($ttl * 1000)))) {
throw new LockConflictedException();
}

if ($key->isExpired()) {
throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $key));
}
}

/**
Expand Down
12 changes: 6 additions & 6 deletions src/Symfony/Component/Lock/Tests/LockTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ public function testExpiration($ttls, $expected)

foreach ($ttls as $ttl) {
if (null === $ttl) {
$key->resetExpiringDate();
$key->resetLifetime();
} else {
$key->reduceLifetime($ttl);
}
Expand All @@ -175,12 +175,12 @@ public function testExpiration($ttls, $expected)

public function provideExpiredDates()
{
yield array(array(-1.0), true);
yield array(array(1, -1.0), true);
yield array(array(-1.0, 1), true);
yield array(array(-0.1), true);
yield array(array(0.1, -0.1), true);
yield array(array(-0.1, 0.1), true);

yield array(array(), false);
yield array(array(1), false);
yield array(array(-1.0, null), false);
yield array(array(0.1), false);
yield array(array(-0.1, null), false);
}
}
10 changes: 5 additions & 5 deletions src/Symfony/Component/Lock/Tests/Store/CombinedStoreTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -175,12 +175,12 @@ public function testputOffExpirationThrowsExceptionOnFailure()
$this->store1
->expects($this->once())
->method('putOffExpiration')
->with($key, $ttl)
->with($key, $this->lessThanOrEqual($ttl))
->willThrowException(new LockConflictedException());
$this->store2
->expects($this->once())
->method('putOffExpiration')
->with($key, $ttl)
->with($key, $this->lessThanOrEqual($ttl))
->willThrowException(new LockConflictedException());

$this->strategy
Expand All @@ -203,12 +203,12 @@ public function testputOffExpirationCleanupOnFailure()
$this->store1
->expects($this->once())
->method('putOffExpiration')
->with($key, $ttl)
->with($key, $this->lessThanOrEqual($ttl))
->willThrowException(new LockConflictedException());
$this->store2
->expects($this->once())
->method('putOffExpiration')
->with($key, $ttl)
->with($key, $this->lessThanOrEqual($ttl))
->willThrowException(new LockConflictedException());

$this->store1
Expand Down Expand Up @@ -242,7 +242,7 @@ public function testputOffExpirationAbortWhenStrategyCantBeMet()
$this->store1
->expects($this->once())
->method('putOffExpiration')
->with($key, $ttl)
->with($key, $this->lessThanOrEqual($ttl))
->willThrowException(new LockConflictedException());
$this->store2
->expects($this->never())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,22 @@ public function testExpiration()
$this->assertFalse($store->exists($key));
}

/**
* Tests the store thrown exception when TTL expires.
*
* @expectedException \Symfony\Component\Lock\Exception\LockExpiredException
*/
public function testAbortAfterExpiration()
{
$key = new Key(uniqid(__METHOD__, true));

/** @var StoreInterface $store */
$store = $this->getStore();

$store->save($key);
$store->putOffExpiration($key, 1 / 1000000);
}

/**
* Tests the refresh can push the limits to the expiration.
*
Expand All @@ -69,10 +85,10 @@ public function testRefreshLock()
$store = $this->getStore();

$store->save($key);
$store->putOffExpiration($key, 1.0 * $clockDelay / 1000000);
$store->putOffExpiration($key, $clockDelay / 1000000);
$this->assertTrue($store->exists($key));

usleep(2.1 * $clockDelay);
usleep(2 * $clockDelay);
$this->assertFalse($store->exists($key));
}

Expand All @@ -85,6 +101,7 @@ public function testSetExpiration()

$store->save($key);
$store->putOffExpiration($key, 1);
$this->assertNotNull($key->getExpiringDate());
$this->assertGreaterThanOrEqual(0, $key->getRemainingLifetime());
$this->assertLessThanOrEqual(1, $key->getRemainingLifetime());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,9 @@ public function getStore()

return new MemcachedStore($memcached);
}

public function testAbortAfterExpiration()
{
$this->markTestSkipped('Memcached expects a TTL greater than 1 sec. Simulating a slow network is too hard');
}
}