[lock] Provides default implementation when store does not supports the behavior

Author: jderusse

BlockingSharedLockStoreInterface.php

@@ -0,0 +1,27 @@
+<?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;
+
+use Symfony\Component\Lock\Exception\LockConflictedException;
+
+/**
+ * @author Jérémy Derussé <jeremy@derusse.com>
+ */
+interface BlockingSharedLockStoreInterface extends SharedLockStoreInterface
+{
+    /**
+     * Waits until a key becomes free for reading, then stores the resource.
+     *
+     * @throws LockConflictedException
+     */
+    public function waitAndSaveRead(Key $key);
+}

CHANGELOG.md

@@ -7,6 +7,8 @@ CHANGELOG
  * `MongoDbStore` does not implement `BlockingStoreInterface` anymore, typehint against `PersistingStoreInterface` instead.
  * added support for shared locks
  * added `NoLock`
+ * deprecated `NotSupportedException`, it shouldn't be thrown anymore.
+ * deprecated `RetryTillSaveStore`, logic has been moved in `Lock` and is not needed anymore.
 
 5.1.0
 -----

Exception/NotSupportedException.php

@@ -11,10 +11,14 @@
 
 namespace Symfony\Component\Lock\Exception;
 
+trigger_deprecation('symfony/lock', '5.2', '%s is deprecated, You should stop using it, as it will be removed in 6.0.', NotSupportedException::class);
+
 /**
  * NotSupportedException is thrown when an unsupported method is called.
  *
  * @author Jérémy Derussé <jeremy@derusse.com>
+ *
+ * @deprecated since Symfony 5.2
  */
 class NotSupportedException extends \LogicException implements ExceptionInterface
 {

Lock.php

@@ -19,7 +19,6 @@
 use Symfony\Component\Lock\Exception\LockConflictedException;
 use Symfony\Component\Lock\Exception\LockExpiredException;
 use Symfony\Component\Lock\Exception\LockReleasingException;
-use Symfony\Component\Lock\Exception\NotSupportedException;
 
 /**
  * Lock is the default implementation of the LockInterface.
@@ -70,9 +69,16 @@ public function acquire(bool $blocking = false): bool
         try {
             if ($blocking) {
                 if (!$this->store instanceof BlockingStoreInterface) {
-                    throw new NotSupportedException(sprintf('The store "%s" does not support blocking locks.', get_debug_type($this->store)));
+                    while (true) {
+                        try {
+                            $this->store->wait($this->key);
+                        } catch (LockConflictedException $e) {
+                            usleep((100 + random_int(-10, 10)) * 1000);
+                        }
+                    }
+                } else {
+                    $this->store->waitAndSave($this->key);
                 }
-                $this->store->waitAndSave($this->key);
             } else {
                 $this->store->save($this->key);
             }
@@ -116,7 +122,9 @@ public function acquireRead(bool $blocking = false): bool
     {
         try {
             if (!$this->store instanceof SharedLockStoreInterface) {
-                throw new NotSupportedException(sprintf('The store "%s" does not support shared locks.', get_debug_type($this->store)));
+                $this->logger->debug('Store does not support ReadLocks, fallback to WriteLock.', ['resource' => $this->key]);
+
+                return $this->acquire($blocking);
             }
             if ($blocking) {
                 $this->store->waitAndSaveRead($this->key);
@@ -125,7 +133,7 @@ public function acquireRead(bool $blocking = false): bool
             }
 
             $this->dirty = true;
-            $this->logger->debug('Successfully acquired the "{resource}" lock.', ['resource' => $this->key]);
+            $this->logger->debug('Successfully acquired the "{resource}" lock for reading.', ['resource' => $this->key]);
 
             if ($this->ttl) {
                 $this->refresh();

SharedLockStoreInterface.php

@@ -12,7 +12,6 @@
 namespace Symfony\Component\Lock;
 
 use Symfony\Component\Lock\Exception\LockConflictedException;
-use Symfony\Component\Lock\Exception\NotSupportedException;
 
 /**
  * @author Jérémy Derussé <jeremy@derusse.com>
@@ -22,15 +21,7 @@ interface SharedLockStoreInterface extends PersistingStoreInterface
     /**
      * Stores the resource if it's not locked for reading by someone else.
      *
-     * @throws NotSupportedException
      * @throws LockConflictedException
      */
     public function saveRead(Key $key);
-
-    /**
-     * Waits until a key becomes free for reading, then stores the resource.
-     *
-     * @throws LockConflictedException
-     */
-    public function waitAndSaveRead(Key $key);
 }

Store/BlockingSharedLockStoreTrait.php

@@ -16,7 +16,6 @@
 use Psr\Log\NullLogger;
 use Symfony\Component\Lock\Exception\InvalidArgumentException;
 use Symfony\Component\Lock\Exception\LockConflictedException;
-use Symfony\Component\Lock\Exception\NotSupportedException;
 use Symfony\Component\Lock\Key;
 use Symfony\Component\Lock\PersistingStoreInterface;
 use Symfony\Component\Lock\SharedLockStoreInterface;
@@ -29,7 +28,6 @@
  */
 class CombinedStore implements SharedLockStoreInterface, LoggerAwareInterface
 {
-    use BlockingSharedLockStoreTrait;
     use ExpiringStoreTrait;
     use LoggerAwareTrait;
 
@@ -97,26 +95,17 @@ public function save(Key $key)
 
     public function saveRead(Key $key)
     {
-        if (null === $this->sharedLockStores) {
-            $this->sharedLockStores = [];
-            foreach ($this->stores as $store) {
-                if ($store instanceof SharedLockStoreInterface) {
-                    $this->sharedLockStores[] = $store;
-                }
-            }
-        }
-
         $successCount = 0;
+        $failureCount = 0;
         $storesCount = \count($this->stores);
-        $failureCount = $storesCount - \count($this->sharedLockStores);
 
-        if (!$this->strategy->canBeMet($failureCount, $storesCount)) {
-            throw new NotSupportedException(sprintf('The store "%s" does not contains enough compatible store to met the requirements.', get_debug_type($this)));
-        }
-
-        foreach ($this->sharedLockStores as $store) {
+        foreach ($this->stores as $store) {
             try {
-                $store->saveRead($key);
+                if ($store instanceof SharedLockStoreInterface) {
+                    $store->saveRead($key);
+                } else {
+                    $store->save($key);
+                }
                 ++$successCount;
             } catch (\Exception $e) {
                 $this->logger->debug('One store failed to save the "{resource}" lock.', ['resource' => $key, 'store' => $store, 'exception' => $e]);

Store/CombinedStore.php

@@ -31,7 +31,6 @@
 class RedisStore implements SharedLockStoreInterface
 {
     use ExpiringStoreTrait;
-    use BlockingSharedLockStoreTrait;
 
     private $redis;
     private $initialTtl;

Store/RedisStore.php

@@ -16,18 +16,21 @@
 use Psr\Log\NullLogger;
 use Symfony\Component\Lock\BlockingStoreInterface;
 use Symfony\Component\Lock\Exception\LockConflictedException;
-use Symfony\Component\Lock\Exception\NotSupportedException;
 use Symfony\Component\Lock\Key;
+use Symfony\Component\Lock\Lock;
 use Symfony\Component\Lock\PersistingStoreInterface;
-use Symfony\Component\Lock\SharedLockStoreInterface;
+
+trigger_deprecation('symfony/lock', '5.2', '%s is deprecated, the "%s" class provides the logic when store is not blocking.', RetryTillSaveStore::class, Lock::class);
 
 /**
  * RetryTillSaveStore is a PersistingStoreInterface implementation which decorate a non blocking PersistingStoreInterface to provide a
  * blocking storage.
  *
  * @author Jérémy Derussé <jeremy@derusse.com>
+ *
+ * @deprecated since Symfony 5.2
  */
-class RetryTillSaveStore implements BlockingStoreInterface, SharedLockStoreInterface, LoggerAwareInterface
+class RetryTillSaveStore implements BlockingStoreInterface, LoggerAwareInterface
 {
     use LoggerAwareTrait;
 
@@ -78,24 +81,6 @@ public function waitAndSave(Key $key)
         throw new LockConflictedException();
     }
 
-    public function saveRead(Key $key)
-    {
-        if (!$this->decorated instanceof SharedLockStoreInterface) {
-            throw new NotSupportedException(sprintf('The "%s" store must decorate a "%s" store.', get_debug_type($this), ShareLockStoreInterface::class));
-        }
-
-        $this->decorated->saveRead($key);
-    }
-
-    public function waitAndSaveRead(Key $key)
-    {
-        if (!$this->decorated instanceof SharedLockStoreInterface) {
-            throw new NotSupportedException(sprintf('The "%s" store must decorate a "%s" store.', get_debug_type($this), ShareLockStoreInterface::class));
-        }
-
-        $this->decorated->waitAndSaveRead($key);
-    }
-
     /**
      * {@inheritdoc}
      */

Store/RetryTillSaveStore.php

@@ -12,7 +12,6 @@
 namespace Symfony\Component\Lock\Tests\Store;
 
 use Symfony\Component\Lock\Exception\LockConflictedException;
-use Symfony\Component\Lock\Exception\NotSupportedException;
 use Symfony\Component\Lock\Key;
 use Symfony\Component\Lock\PersistingStoreInterface;
 
@@ -61,25 +60,20 @@ public function testBlockingLocks()
                 // This call should failed given the lock should already by acquired by the child
                 $store->save($key);
                 $this->fail('The store saves a locked key.');
-            } catch (NotSupportedException $e) {
             } catch (LockConflictedException $e) {
             }
 
             // send the ready signal to the child
             posix_kill($childPID, \SIGHUP);
 
             // This call should be blocked by the child #1
-            try {
-                $store->waitAndSave($key);
-                $this->assertTrue($store->exists($key));
-                $store->delete($key);
+            $store->waitAndSave($key);
+            $this->assertTrue($store->exists($key));
+            $store->delete($key);
 
-                // Now, assert the child process worked well
-                pcntl_waitpid($childPID, $status1);
-                $this->assertSame(0, pcntl_wexitstatus($status1), 'The child process couldn\'t lock the resource');
-            } catch (NotSupportedException $e) {
-                $this->markTestSkipped(sprintf('The store %s does not support waitAndSave.', \get_class($store)));
-            }
+            // Now, assert the child process worked well
+            pcntl_waitpid($childPID, $status1);
+            $this->assertSame(0, pcntl_wexitstatus($status1), 'The child process couldn\'t lock the resource');
         } else {
             // Block SIGHUP signal
             pcntl_sigprocmask(\SIG_BLOCK, [\SIGHUP]);

Tests/Store/BlockingStoreTestTrait.php

@@ -14,7 +14,6 @@
 use PHPUnit\Framework\MockObject\MockObject;
 use Symfony\Component\Lock\BlockingStoreInterface;
 use Symfony\Component\Lock\Exception\LockConflictedException;
-use Symfony\Component\Lock\Exception\NotSupportedException;
 use Symfony\Component\Lock\Key;
 use Symfony\Component\Lock\PersistingStoreInterface;
 use Symfony\Component\Lock\SharedLockStoreInterface;
@@ -355,18 +354,6 @@ public function testDeleteDontStopOnFailure()
         $this->store->delete($key);
     }
 
-    public function testSaveReadWithIncompatibleStores()
-    {
-        $key = new Key(uniqid(__METHOD__, true));
-
-        $badStore = $this->createMock(PersistingStoreInterface::class);
-
-        $store = new CombinedStore([$badStore], new UnanimousStrategy());
-        $this->expectException(NotSupportedException::class);
-
-        $store->saveRead($key);
-    }
-
     public function testSaveReadWithCompatibleStore()
     {
         $key = new Key(uniqid(__METHOD__, true));