Issue #342: CacheResponse race condition with has and get (#434)

* Issue #342: Resolve issue with has/get race condition * Issue #342: Align more closely with original code * Update CacheResponse.php --------- Co-authored-by: Freek Van der Herten <[email protected]>
spatie · Apr 7, 2023 · 8f53ee8 · 8f53ee8
1 parent 1c87452
commit 8f53ee8
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 10 deletions.
diff --git a/src/Middlewares/CacheResponse.php b/src/Middlewares/CacheResponse.php
@@ -12,6 +12,7 @@
 use Spatie\ResponseCache\Replacers\Replacer;
 use Spatie\ResponseCache\ResponseCache;
 use Symfony\Component\HttpFoundation\Response;
+use Throwable;
 
 class CacheResponse
 {
@@ -30,17 +31,11 @@ public function handle(Request $request, Closure $next, ...$args): Response
         if ($this->responseCache->enabled($request) && ! $this->responseCache->shouldBypass($request)) {
             try {
                 if ($this->responseCache->hasBeenCached($request, $tags)) {
-                    event(new ResponseCacheHit($request));
 
-                    $response = $this->responseCache->getCachedResponseFor($request, $tags);
-
-                    $response = $this->addCacheAgeHeader($response);
-
-                    $this->getReplacers()->each(function (Replacer $replacer) use ($response) {
-                        $replacer->replaceInCachedResponse($response);
-                    });
-
-                    return $response;
+                    $response = $this->getCachedResponse($request, $tags);
+                    if ($response !== false) {
+                        return $response;
+                    }
                 }
             } catch (CouldNotUnserialize $e) {
                 report("Could not unserialize response, returning uncached response instead. Error: {$e->getMessage()}");
@@ -61,6 +56,31 @@ public function handle(Request $request, Closure $next, ...$args): Response
         return $response;
     }
 
+    protected function getCachedResponse(Request $request, array $tags = []): false|Response
+    {
+        try {
+            $response = $this->responseCache->getCachedResponseFor($request, $tags);
+        }
+        catch (CouldNotUnserialize $exception) {
+            throw $exception;
+        }
+        catch (Throwable $exception) {
+            report("Unable to retrieve cached response when one was expected. Error: {$exception->getMessage()}");
+
+            return false;
+        }
+
+        event(new ResponseCacheHit($request));
+
+        $response = $this->addCacheAgeHeader($response);
+
+        $this->getReplacers()->each(function (Replacer $replacer) use ($response) {
+            $replacer->replaceInCachedResponse($response);
+        });
+
+        return $response;
+    }
+
     protected function makeReplacementsAndCacheResponse(
         Request $request,
         Response $response,

diff --git a/tests/ResponseCacheRepositoryTest.php b/tests/ResponseCacheRepositoryTest.php
@@ -1,6 +1,7 @@
 <?php
 
 use Illuminate\Cache\Repository;
+use Illuminate\Testing\TestResponse;
 use Spatie\ResponseCache\Exceptions\CouldNotUnserialize;
 use Spatie\ResponseCache\ResponseCacheRepository;
 use Spatie\ResponseCache\Serializers\Serializer;
@@ -15,3 +16,36 @@
     $repository = new ResponseCacheRepository($responseSerializer, $cacheRepository);
     $repository->get('missed-cache');
 })->throws(CouldNotUnserialize::class);
+
+it('will handle race conditions between has and get', function () {
+
+    /** @var Serializer $responseSerializer */
+    $responseSerializer = app(Serializer::class);
+    /** @var Illuminate\Cache\ArrayStore $cacheStore */
+    $cacheStore = app('cache')
+        ->store('array')
+        ->getStore();
+
+    // This order of operations simulates a cache lookup happening during a
+    // cache expiration or purge event. The `has()` call should succeed, but
+    // after that the cache has 'expired' and is unavailable.
+    $cachedValues = [
+        $responseSerializer->serialize(createResponse(200)),
+        null,
+    ];
+
+    // We cannot use the partialMock helper because the cache store must be
+    // available and partialMock does not allow constructor arguments.
+    $cacheRepository = Mockery::mock(Repository::class, [$cacheStore]);
+    $cacheRepository
+            ->shouldReceive('get')
+            ->twice()
+            ->andReturns($cachedValues);
+    $cacheRepository->makePartial();
+    $this->instance(Repository::class, $cacheRepository);
+
+    /** @var TestResponse $response */
+    $response = $this->get('/random');
+    assertRegularResponse($response);
+    expect($response->exception)->toBeNull();
+});