[10.x] Use atomic locks for command mutex (#47624)

* [10.x] use atomic locks for command mutex * fix carbon usage and use correct lock store * fix styleci * fix tests * formatting --------- Co-authored-by: Taylor Otwell <[email protected]>
laravel · Jul 13, 2023 · 5a34797 · 5a34797
1 parent 3cd9bcb
commit 5a34797
Show file tree

Hide file tree

Showing 2 changed files with 136 additions and 14 deletions.
diff --git a/src/Illuminate/Console/CacheCommandMutex.php b/src/Illuminate/Console/CacheCommandMutex.php
@@ -3,10 +3,15 @@
 namespace Illuminate\Console;
 
 use Carbon\CarbonInterval;
+use Illuminate\Cache\DynamoDbStore;
 use Illuminate\Contracts\Cache\Factory as Cache;
+use Illuminate\Contracts\Cache\LockProvider;
+use Illuminate\Support\InteractsWithTime;
 
 class CacheCommandMutex implements CommandMutex
 {
+    use InteractsWithTime;
+
     /**
      * The cache factory implementation.
      *
@@ -39,13 +44,20 @@ public function __construct(Cache $cache)
      */
     public function create($command)
     {
-        return $this->cache->store($this->store)->add(
-            $this->commandMutexName($command),
-            true,
-            method_exists($command, 'isolationLockExpiresAt')
-                    ? $command->isolationLockExpiresAt()
-                    : CarbonInterval::hour(),
-        );
+        $store = $this->cache->store($this->store);
+
+        $expiresAt = method_exists($command, 'isolationLockExpiresAt')
+            ? $command->isolationLockExpiresAt()
+            : CarbonInterval::hour();
+
+        if ($this->shouldUseLocks($store->getStore())) {
+            return $store->getStore()->lock(
+                $this->commandMutexName($command),
+                $this->secondsUntil($expiresAt)
+            )->get();
+        }
+
+        return $store->add($this->commandMutexName($command), true, $expiresAt);
     }
 
     /**
@@ -56,9 +68,19 @@ public function create($command)
      */
     public function exists($command)
     {
-        return $this->cache->store($this->store)->has(
-            $this->commandMutexName($command)
-        );
+        $store = $this->cache->store($this->store);
+
+        if ($this->shouldUseLocks($store->getStore())) {
+            $lock = $store->getStore()->lock($this->commandMutexName($command));
+
+            return tap(! $lock->get(), function ($exists) {
+                if ($exists) {
+                    $lock->release();
+                }
+            });
+        }
+
+        return $this->cache->store($this->store)->has($this->commandMutexName($command));
     }
 
     /**
@@ -69,9 +91,13 @@ public function exists($command)
      */
     public function forget($command)
     {
-        return $this->cache->store($this->store)->forget(
-            $this->commandMutexName($command)
-        );
+        $store = $this->cache->store($this->store);
+
+        if ($this->shouldUseLocks($store->getStore())) {
+            return $store->getStore()->lock($this->commandMutexName($command))->forceRelease();
+        }
+
+        return $this->cache->store($this->store)->forget($this->commandMutexName($command));
     }
 
     /**
@@ -95,4 +121,15 @@ public function useStore($store)
 
         return $this;
     }
+
+    /**
+     * Determine if the given store should use locks for command mutexes.
+     *
+     * @param  \Illuminate\Contracts\Cache\Store  $store
+     * @return bool
+     */
+    protected function shouldUseLocks($store)
+    {
+        return $store instanceof LockProvider && ! $store instanceof DynamoDbStore;
+    }
 }
diff --git a/tests/Console/CacheCommandMutexTest.php b/tests/Console/CacheCommandMutexTest.php
@@ -5,8 +5,10 @@
 use Illuminate\Console\CacheCommandMutex;
 use Illuminate\Console\Command;
 use Illuminate\Contracts\Cache\Factory;
+use Illuminate\Contracts\Cache\LockProvider;
 use Illuminate\Contracts\Cache\Repository;
 use Mockery as m;
+use Mockery\MockInterface;
 use PHPUnit\Framework\TestCase;
 
 class CacheCommandMutexTest extends TestCase
@@ -35,16 +37,22 @@ protected function setUp(): void
     {
         $this->cacheFactory = m::mock(Factory::class);
         $this->cacheRepository = m::mock(Repository::class);
-        $this->cacheFactory->shouldReceive('store')->andReturn($this->cacheRepository);
         $this->mutex = new CacheCommandMutex($this->cacheFactory);
         $this->command = new class extends Command
         {
             protected $name = 'command-name';
         };
     }
 
+    protected function tearDown(): void
+    {
+        m::close();
+        parent::tearDown();
+    }
+
     public function testCanCreateMutex()
     {
+        $this->mockUsingCacheStore();
         $this->cacheRepository->shouldReceive('add')
             ->andReturn(true)
             ->once();
@@ -55,6 +63,7 @@ public function testCanCreateMutex()
 
     public function testCannotCreateMutexIfAlreadyExist()
     {
+        $this->mockUsingCacheStore();
         $this->cacheRepository->shouldReceive('add')
             ->andReturn(false)
             ->once();
@@ -65,6 +74,31 @@ public function testCannotCreateMutexIfAlreadyExist()
 
     public function testCanCreateMutexWithCustomConnection()
     {
+        $this->mockUsingCacheStore();
+        $this->cacheRepository->shouldReceive('getStore')
+            ->with('test')
+            ->andReturn($this->cacheRepository);
+        $this->cacheRepository->shouldReceive('add')
+            ->andReturn(false)
+            ->once();
+        $this->mutex->useStore('test');
+
+        $this->mutex->create($this->command);
+    }
+
+    public function testCanCreateMutexWithLockProvider()
+    {
+        $lock = $this->mockUsingLockProvider();
+        $this->acquireLockExpectations($lock, true);
+
+        $actual = $this->mutex->create($this->command);
+
+        $this->assertTrue($actual);
+    }
+
+    public function testCanCreateMutexWithCustomLockProviderConnection()
+    {
+        $this->mockUsingCacheStore();
         $this->cacheRepository->shouldReceive('getStore')
             ->with('test')
             ->andReturn($this->cacheRepository);
@@ -75,4 +109,55 @@ public function testCanCreateMutexWithCustomConnection()
 
         $this->mutex->create($this->command);
     }
+
+    public function testCannotCreateMutexIfAlreadyExistWithLockProvider()
+    {
+        $lock = $this->mockUsingLockProvider();
+        $this->acquireLockExpectations($lock, false);
+        $actual = $this->mutex->create($this->command);
+
+        $this->assertFalse($actual);
+    }
+
+    public function testCanCreateMutexWithCustomConnectionWithLockProvider()
+    {
+        $lock = m::mock(LockProvider::class);
+        $this->cacheFactory->expects('store')->once()->with('test')->andReturn($this->cacheRepository);
+        $this->cacheRepository->expects('getStore')->twice()->andReturn($lock);
+
+        $this->acquireLockExpectations($lock, true);
+        $this->mutex->useStore('test');
+
+        $this->mutex->create($this->command);
+    }
+
+    /**
+     * @return void
+     */
+    private function mockUsingCacheStore(): void
+    {
+        $this->cacheFactory->expects('store')->once()->andReturn($this->cacheRepository);
+        $this->cacheRepository->expects('getStore')->andReturn(null);
+    }
+
+    private function mockUsingLockProvider(): m\MockInterface
+    {
+        $lock = m::mock(LockProvider::class);
+        $this->cacheFactory->expects('store')->once()->andReturn($this->cacheRepository);
+        $this->cacheRepository->expects('getStore')->twice()->andReturn($lock);
+
+        return $lock;
+    }
+
+    private function acquireLockExpectations(MockInterface $lock, bool $acquiresSuccessfully): void
+    {
+        $lock->expects('lock')
+            ->once()
+            ->with(m::type('string'), m::type('int'))
+            ->andReturns($lock);
+
+        $lock->expects('get')
+            ->once()
+            ->andReturns($acquiresSuccessfully);
+    }
 }