Fix expired lock not cleaned

symfony · Jun 17, 2019 · 2c6fca7 · 2c6fca7
1 parent a8520ae
commit 2c6fca7
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 16 deletions.
diff --git a/src/Symfony/Component/Lock/Lock.php b/src/Symfony/Component/Lock/Lock.php
@@ -83,6 +83,11 @@ public function acquire($blocking = false)
             }
 
             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 @@ public function refresh()
             $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));
             }
 

diff --git a/src/Symfony/Component/Lock/Store/CombinedStore.php b/src/Symfony/Component/Lock/Store/CombinedStore.php
@@ -30,6 +30,7 @@
 class CombinedStore implements StoreInterface, LoggerAwareInterface
 {
     use LoggerAwareTrait;
+    use ExpiringStoreTrait;
 
     /** @var StoreInterface[] */
     private $stores;
@@ -78,6 +79,8 @@ public function save(Key $key)
             }
         }
 
+        $this->checkNotExpired($key);
+
         if ($this->strategy->isMet($successCount, $storesCount)) {
             return;
         }
@@ -125,9 +128,7 @@ 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));
-        }
+        $this->checkNotExpired($key);
 
         if ($this->strategy->isMet($successCount, $storesCount)) {
             return;

diff --git a/src/Symfony/Component/Lock/Store/ExpiringStoreTrait.php b/src/Symfony/Component/Lock/Store/ExpiringStoreTrait.php
@@ -0,0 +1,21 @@
+<?php
+
+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));
+        }
+    }
+}
diff --git a/src/Symfony/Component/Lock/Store/MemcachedStore.php b/src/Symfony/Component/Lock/Store/MemcachedStore.php
@@ -24,6 +24,8 @@
  */
 class MemcachedStore implements StoreInterface
 {
+    use ExpiringStoreTrait;
+
     private $memcached;
     private $initialTtl;
     /** @var bool */
@@ -64,9 +66,7 @@ public function save(Key $key)
             $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 +110,7 @@ public function putOffExpiration(Key $key, $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));
-        }
+        $this->checkNotExpired($key);
     }
 
     /**

diff --git a/src/Symfony/Component/Lock/Store/RedisStore.php b/src/Symfony/Component/Lock/Store/RedisStore.php
@@ -14,7 +14,6 @@
 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 @@
  */
 class RedisStore implements StoreInterface
 {
+    use ExpiringStoreTrait;
+
     private $redis;
     private $initialTtl;
 
@@ -66,9 +67,7 @@ public function save(Key $key)
             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 @@ public function putOffExpiration(Key $key, $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));
-        }
+        $this->checkNotExpired($key);
     }
 
     /**

diff --git a/src/Symfony/Component/Lock/Tests/Store/ExpiringStoreTestTrait.php b/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 @@ public function testSetExpiration()
         $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));
+    }
 }