bug symfony#60502 [HttpCache] Hit the backend only once after waiting for the cache lock (mpdude)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[HttpCache] Hit the backend only once after waiting for the cache lock

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

When the `HttpCache` has to wait for a lock held by another, concurrent process, the `lock()` method wants to make sure we continue operation based on the most recent cache entry, possibly updated by the concurrent process.

So, after waiting for lock release, it calls `lookup()` to obtain this cache entry. This is, in fact, a reentrant call up into the current call stack.

Having `lookup()` multiple times on the call stack opens up a way to call the backend for validation multiple times. I have observed this in practice at least in combination with the `no-cache` cache-control header, causing surprising side effects™️ ✨.

Also without `no-cache` you can get strange-looking cache traces like `stale, valid, store, fresh`. Those occur only when concurrent locking is a issue.

I am not super happy with using an exception for a control flow issue like this. But, rolling back to `lookup` seems to be the most sensible decision for me, and using special return values to indicate this condition isn't really pretty either.

Commits
-------

df064b0 [HttpCache] Hit the backend only once after waiting for the cache lock

Author: nicolas-grekas

src/Symfony/Component/HttpKernel/HttpCache/CacheWasLockedException.php

@@ -0,0 +1,19 @@
+<?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\HttpKernel\HttpCache;
+
+/**
+ * @internal
+ */
+class CacheWasLockedException extends \Exception
+{
+}

src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php

@@ -219,7 +219,13 @@ public function handle(Request $request, int $type = HttpKernelInterface::MAIN_R
             $this->record($request, 'reload');
             $response = $this->fetch($request, $catch);
         } else {
-            $response = $this->lookup($request, $catch);
+            $response = null;
+            do {
+                try {
+                    $response = $this->lookup($request, $catch);
+                } catch (CacheWasLockedException) {
+                }
+            } while (null === $response);
         }
 
         $this->restoreResponseBody($request, $response);
@@ -576,15 +582,7 @@ protected function lock(Request $request, Response $entry): bool
 
         // wait for the lock to be released
         if ($this->waitForLock($request)) {
-            // replace the current entry with the fresh one
-            $new = $this->lookup($request);
-            $entry->headers = $new->headers;
-            $entry->setContent($new->getContent());
-            $entry->setStatusCode($new->getStatusCode());
-            $entry->setProtocolVersion($new->getProtocolVersion());
-            foreach ($new->headers->getCookies() as $cookie) {
-                $entry->headers->setCookie($cookie);
-            }
+            throw new CacheWasLockedException(); // unwind back to handle(), try again
         } else {
             // backend is slow as hell, send a 503 response (to avoid the dog pile effect)
             $entry->setStatusCode(503);

src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php

@@ -18,6 +18,7 @@
 use Symfony\Component\HttpKernel\Event\TerminateEvent;
 use Symfony\Component\HttpKernel\HttpCache\Esi;
 use Symfony\Component\HttpKernel\HttpCache\HttpCache;
+use Symfony\Component\HttpKernel\HttpCache\Store;
 use Symfony\Component\HttpKernel\HttpCache\StoreInterface;
 use Symfony\Component\HttpKernel\HttpKernelInterface;
 use Symfony\Component\HttpKernel\Kernel;
@@ -717,6 +718,7 @@ public function testDegradationWhenCacheLocked()
          */
         sleep(10);
 
+        $this->store = $this->createStore(); // create another store instance that does not hold the current lock
         $this->request('GET', '/');
         $this->assertHttpKernelIsNotCalled();
         $this->assertEquals(200, $this->response->getStatusCode());
@@ -735,6 +737,64 @@ public function testDegradationWhenCacheLocked()
         $this->assertEquals('Old response', $this->response->getContent());
     }
 
+    public function testHitBackendOnlyOnceWhenCacheWasLocked()
+    {
+        // Disable stale-while-revalidate, it circumvents waiting for the lock
+        $this->cacheConfig['stale_while_revalidate'] = 0;
+
+        $this->setNextResponses([
+            [
+                'status' => 200,
+                'body' => 'initial response',
+                'headers' => [
+                    'Cache-Control' => 'public, no-cache',
+                    'Last-Modified' => 'some while ago',
+                ],
+            ],
+            [
+                'status' => 304,
+                'body' => '',
+                'headers' => [
+                    'Cache-Control' => 'public, no-cache',
+                    'Last-Modified' => 'some while ago',
+                ],
+            ],
+            [
+                'status' => 500,
+                'body' => 'The backend should not be called twice during revalidation',
+                'headers' => [],
+            ],
+        ]);
+
+        $this->request('GET', '/'); // warm the cache
+
+        // Use a store that simulates a cache entry being locked upon first attempt
+        $this->store = new class(sys_get_temp_dir() . '/http_cache') extends Store {
+            private bool $hasLock = false;
+
+            public function lock(Request $request): bool
+            {
+                $hasLock = $this->hasLock;
+                $this->hasLock = true;
+
+                return $hasLock;
+            }
+
+            public function isLocked(Request $request): bool
+            {
+                return false;
+            }
+        };
+
+        $this->request('GET', '/'); // hit the cache with simulated lock/concurrency block
+
+        $this->assertEquals(200, $this->response->getStatusCode());
+        $this->assertEquals('initial response', $this->response->getContent());
+
+        $traces = $this->cache->getTraces();
+        $this->assertSame(['stale', 'valid', 'store'], current($traces));
+    }
+
     public function testHitsCachedResponseWithSMaxAgeDirective()
     {
         $time = \DateTimeImmutable::createFromFormat('U', time() - 5);

src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTestCase.php

@@ -30,7 +30,7 @@ abstract class HttpCacheTestCase extends TestCase
     protected $responses;
     protected $catch;
     protected $esi;
-    protected Store $store;
+    protected ?Store $store = null;
 
     protected function setUp(): void
     {
@@ -115,7 +115,9 @@ public function request($method, $uri = '/', $server = [], $cookies = [], $esi =
 
         $this->kernel->reset();
 
-        $this->store = new Store(sys_get_temp_dir().'/http_cache');
+        if (! $this->store) {
+            $this->store = $this->createStore();
+        }
 
         if (!isset($this->cacheConfig['debug'])) {
             $this->cacheConfig['debug'] = true;
@@ -183,4 +185,9 @@ public static function clearDirectory($directory)
 
         closedir($fp);
     }
+
+    protected function createStore(): Store
+    {
+        return new Store(sys_get_temp_dir() . '/http_cache');
+    }
 }