feature #22542 [Lock] Check TTL expiration in lock acquisition (jderusse)

This PR was squashed before being merged into the 3.4 branch (closes #22542).

Discussion
----------

[Lock] Check TTL expiration in lock acquisition

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22452
| License       | MIT
| Doc PR        | NA

This PR improve lock acquisition by throwing exception when the TTL is expired during the lock process.

Commits
-------

f74ef35778 [Lock] Check TTL expiration in lock acquisition

Author: fabpot

Exception/LockExpiredException.php

@@ -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
+{
+}

Key.php

@@ -19,7 +19,7 @@
 final class Key
 {
     private $resource;
-    private $expiringDate;
+    private $expiringTime;
     private $state = array();
 
     /**
@@ -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) {
-            $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);
     }
 
     /**
-     * @return \DateTimeImmutable
+     * @return bool
      */
-    public function getExpiringDate()
+    public function isExpired()
     {
-        return $this->expiringDate;
+        return null !== $this->expiringTime && $this->expiringTime <= microtime(true);
     }
 }

Lock.php

@@ -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;
 
 /**
@@ -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));
@@ -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));
@@ -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();
     }
 }

Store/CombinedStore.php

@@ -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;
@@ -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));
@@ -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;
         }

Store/MemcachedStore.php

@@ -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;
 
@@ -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)
@@ -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));
+        }
     }
 
     /**

Store/RedisStore.php

@@ -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;
 
@@ -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)
@@ -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));
+        }
     }
 
     /**

Tests/LockTest.php

@@ -165,7 +165,7 @@ public function testExpiration($ttls, $expected)
 
         foreach ($ttls as $ttl) {
             if (null === $ttl) {
-                $key->resetExpiringDate();
+                $key->resetLifetime();
             } else {
                 $key->reduceLifetime($ttl);
             }
@@ -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);
     }
 }

Tests/Store/CombinedStoreTest.php

@@ -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
@@ -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
@@ -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())

Tests/Store/ExpiringStoreTestTrait.php

@@ -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.
      *
@@ -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));
     }
 
@@ -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());
     }
 }

Tests/Store/MemcachedStoreTest.php

@@ -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');
+    }
 }