bug #32071 Fix expired lock not cleaned (jderusse)

This PR was merged into the 3.4 branch.

Discussion
----------

Fix expired lock not cleaned

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

When a lock is acquired BUT not as fast as expected, a LockExpiredException is thrown.
Issue is, that the lock is not removed which avoid other process to acquire that lock.

This PR clean state of store when a LockExpiredException is triggered.

note: same bug should be fixed in 4.3 in PDO and Zookeeper

Commits
-------

9f960f34e7 Fix expired lock not cleaned

src/Symfony/Component/Lock/Lock.php

@@ -83,6 +83,11 @@ final class Lock implements LockInterface, LoggerAwareInterface
             }
 
             if ($this->key->isExpired()) {
+                try {
+                    $this->release();
+                } catch (\Exception $e) {
+                    // swallow exception to not hide the original issue
+                }
                 throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $this->key));
             }
 
@@ -117,6 +122,11 @@ final class Lock implements LockInterface, LoggerAwareInterface
             $this->dirty = true;
 
             if ($this->key->isExpired()) {
+                try {
+                    $this->release();
+                } catch (\Exception $e) {
+                    // swallow exception to not hide the original issue
+                }
                 throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $this->key));
             }
 

src/Symfony/Component/Lock/Store/CombinedStore.php

@@ -16,7 +16,6 @@ use Psr\Log\LoggerAwareTrait;
 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\StoreInterface;
@@ -30,6 +29,7 @@ use Symfony\Component\Lock\Strategy\StrategyInterface;
 class CombinedStore implements StoreInterface, LoggerAwareInterface
 {
     use LoggerAwareTrait;
+    use ExpiringStoreTrait;
 
     /** @var StoreInterface[] */
     private $stores;
@@ -78,6 +78,8 @@ class CombinedStore implements StoreInterface, LoggerAwareInterface
             }
         }
 
+        $this->checkNotExpired($key);
+
         if ($this->strategy->isMet($successCount, $storesCount)) {
             return;
         }
@@ -125,9 +127,7 @@ class CombinedStore implements StoreInterface, LoggerAwareInterface
             }
         }
 
-        if ($key->isExpired()) {
-            throw new LockExpiredException(sprintf('Failed to put off the expiration of the "%s" lock within the specified time.', $key));
-        }
+        $this->checkNotExpired($key);
 
         if ($this->strategy->isMet($successCount, $storesCount)) {
             return;

src/Symfony/Component/Lock/Store/ExpiringStoreTrait.php

@@ -0,0 +1,30 @@
+<?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\Store;
+
+use Symfony\Component\Lock\Exception\LockExpiredException;
+use Symfony\Component\Lock\Key;
+
+trait ExpiringStoreTrait
+{
+    private function checkNotExpired(Key $key)
+    {
+        if ($key->isExpired()) {
+            try {
+                $this->delete($key);
+            } catch (\Exception $e) {
+                // swallow exception to not hide the original issue
+            }
+            throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $key));
+        }
+    }
+}

src/Symfony/Component/Lock/Store/MemcachedStore.php

@@ -13,7 +13,6 @@ namespace Symfony\Component\Lock\Store;
 
 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;
 
@@ -24,6 +23,8 @@ use Symfony\Component\Lock\StoreInterface;
  */
 class MemcachedStore implements StoreInterface
 {
+    use ExpiringStoreTrait;
+
     private $memcached;
     private $initialTtl;
     /** @var bool */
@@ -64,9 +65,7 @@ class MemcachedStore implements StoreInterface
             $this->putOffExpiration($key, $this->initialTtl);
         }
 
-        if ($key->isExpired()) {
-            throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $key));
-        }
+        $this->checkNotExpired($key);
     }
 
     public function waitAndSave(Key $key)
@@ -110,9 +109,7 @@ class MemcachedStore implements StoreInterface
             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));
-        }
+        $this->checkNotExpired($key);
     }
 
     /**

src/Symfony/Component/Lock/Store/RedisStore.php

@@ -14,7 +14,6 @@ namespace Symfony\Component\Lock\Store;
 use Symfony\Component\Cache\Traits\RedisProxy;
 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;
 
@@ -25,6 +24,8 @@ use Symfony\Component\Lock\StoreInterface;
  */
 class RedisStore implements StoreInterface
 {
+    use ExpiringStoreTrait;
+
     private $redis;
     private $initialTtl;
 
@@ -66,9 +67,7 @@ class RedisStore implements StoreInterface
             throw new LockConflictedException();
         }
 
-        if ($key->isExpired()) {
-            throw new LockExpiredException(sprintf('Failed to store the "%s" lock.', $key));
-        }
+        $this->checkNotExpired($key);
     }
 
     public function waitAndSave(Key $key)
@@ -94,9 +93,7 @@ class RedisStore implements StoreInterface
             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));
-        }
+        $this->checkNotExpired($key);
     }
 
     /**

src/Symfony/Component/Lock/Tests/Store/ExpiringStoreTestTrait.php

@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\Lock\Tests\Store;
 
+use Symfony\Component\Lock\Exception\LockExpiredException;
 use Symfony\Component\Lock\Key;
 use Symfony\Component\Lock\StoreInterface;
 
@@ -105,4 +106,28 @@ trait ExpiringStoreTestTrait
         $this->assertGreaterThanOrEqual(0, $key->getRemainingLifetime());
         $this->assertLessThanOrEqual(1, $key->getRemainingLifetime());
     }
+
+    public function testExpiredLockCleaned()
+    {
+        $resource = uniqid(__METHOD__, true);
+
+        $key1 = new Key($resource);
+        $key2 = new Key($resource);
+
+        /** @var StoreInterface $store */
+        $store = $this->getStore();
+        $key1->reduceLifetime(0);
+
+        $this->assertTrue($key1->isExpired());
+        try {
+            $store->save($key1);
+            $this->fail('The store shouldn\'t have save an expired key');
+        } catch (LockExpiredException $e) {
+        }
+
+        $this->assertFalse($store->exists($key1));
+
+        $store->save($key2);
+        $this->assertTrue($store->exists($key2));
+    }
 }