[HttpKernel] PSR-6 HttpCache store #42295

Toflar · 2021-07-27T15:50:02Z

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

As per @nicolas-grekas' request, here's a PR to merge my alternative PSR-6 HttpCache store into Symfony itself.
This is a simple copy & paste with a few namespace and test adjustments to get the discussion started for now.

Here are some additional thoughts:

Issues with the current `StoreInterface` / `HttpCache`

Does not support auto pruning which means it grows forever. This is has become an even bigger issue since the introduction of the CachingHttpClient which also leverages the HttpCache. So you also grow endlessly big cache directories for your external HTTP requests with Symfony's HttpClient.
Does not support cache tagging which is an extremely useful feature for reverse proxies.
Does some custom lock handling instead of relying on Symfony's Lock component.
Does not support caching BinaryFileResponses.

Issues with this PR, things to be clarified

It's not a 100% the same as my current implementation. Specifically, the current StoreInterface lacks the methods for the following three capabilities:
- clear() to be able to flush a cache completely.
- invalidateTags() to be able to invalidate certain cache items of a given set of cache tags only.
- prune() to be able to prune expired cache items.
If you look at the things the Psr6Store has to do, it kind of becomes evident, that HttpCache currently delegates too many responsibilities to the StoreInterface. Vary handling and cache key generation e.g. should be the same for all stores.
The current CachingHttpClient relies on HttpCache as well and does not support greedy caching which is something that should be supported too, imho.
I think, we should rather replace the current HttpCache with an alternative and deprecate the current one together with the current StoreInterface. Imho that alternative could rely on the Symfony Cache abstraction directly. However, we should see if we can add the missing three capabilities to the Cache contracts as they are vital for a reverse proxy cache. We already have TagAwareCacheInterface for invalidateTags() but we're lacking prune() and clear() which would both make sense imho.

List of things to do:

Update changelog
Docs

OskarStark

I left some comments, great work, thank you!

OskarStark · 2021-08-01T16:11:22Z

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

+    private $locks = [];
+
+    /**
+     * When creating a Psr6Store you can configure a number options.


Suggested change

* When creating a Psr6Store you can configure a number options.

* When creating a Psr6Store you can configure a number of options.

OskarStark · 2021-08-01T16:11:54Z

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

+
+    /**
+     * When creating a Psr6Store you can configure a number options.
+     * See the README for a list of all available options and their description.


I would rather remove this sentence and add the options to the Symfony docs

OskarStark · 2021-08-01T16:13:20Z

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

+    }
+
+    /**
+     * Invalidates all cache entries that match the request.


Sometimes you write Request, sometimes request, only one should be used

OskarStark · 2021-08-01T16:13:49Z

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

+    /**
+     * Releases the lock for the given Request.
+     *
+     * @param Request $request A Request instance


Not sure we need this @param annotation 🤔

OskarStark · 2021-08-01T16:15:47Z

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

+        $response->headers->set('X-Content-Digest', $contentDigest);
+
+        // Make sure the content-length header is present
+        if (!$response->headers->has('Transfer-Encoding')) {


What if it already has a Content-Length header? Do we always override it?

OskarStark · 2021-08-01T16:16:46Z

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

+     * @param int   $expiresAfter
+     * @param array $tags
+     */
+    private function saveDeferred(CacheItemInterface $item, $data, $expiresAfter = null, $tags = []): bool


Suggested change

private function saveDeferred(CacheItemInterface $item, $data, $expiresAfter = null, $tags = []): bool

private function saveDeferred(CacheItemInterface $item, $data, int $expiresAfter = null, array $tags = []): bool

and remove the doc block?

OskarStark · 2021-08-01T16:17:37Z

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

+use Symfony\Component\Lock\LockInterface;
+use Symfony\Component\OptionsResolver\Exception\MissingOptionsException;
+
+class Psr6StoreTest extends TestCase


Suggested change

class Psr6StoreTest extends TestCase

final class Psr6StoreTest extends TestCase

OskarStark · 2021-08-01T16:18:00Z

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

+        $this->store->cleanup();
+    }
+
+    public function testCustomCacheWithoutLockFactory(): void


No void return in test classes

OskarStark · 2021-08-01T16:18:49Z

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

+    /**
+     * @param null $store
+     */
+    private function getCache($store = null): TagAwareAdapterInterface


Can we be more precise about the type here?

OskarStark · 2021-08-01T16:19:09Z

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

+        // This test will fail if an exception is thrown, otherwise we mark it
+        // as passed.


Can be on one line

OskarStark · 2021-08-01T16:20:48Z

I think it makes more sense to target 6.0 and introduce this new Store as experimental. Or is it not worth, because the API is already designed and proven?

jderusse · 2021-08-08T21:01:05Z

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

+        $this->locks[$cacheKey] = $this->lockFactory
+            ->createLock($cacheKey);
+
+        return $this->locks[$cacheKey]->acquire();


if lock is not acquired, should you unset this->locks[$cacheKey]?

one could retry to lock the resource several time until it works

peter17 · 2022-06-27T12:00:19Z

Any news on this topic? Anything I can do to help? Thanks

fabpot · 2022-06-27T12:30:18Z

I'm very interested in this PR as well. I've thought a bit about the best path forward, but I'm not sure yet.
I will try to comment again soon about my thoughts.

stof · 2022-06-27T12:32:36Z

The current CachingHttpClient relies on HttpCache as well and does not support greedy caching which is something that should be supported too, imho.

To me, the mistake there was reusing the HttpCache store for the CachingHttpClient while it was designed for a reverse proxy cache, which has slightly different needs.
We will probably need to separate them at some point (we used to have a ticket about that, but I think carsonbot closed it as stale).

fabpot · 2022-06-27T13:15:47Z

I'm very aware of the issue @stof and this is part of my thoughts; I think I would like to start a new component that would include both types of proxies.

Propose to merge PSR-6 HttpCache store

59fca0f

carsonbot added Status: Needs Review Feature labels Jul 27, 2021

derrabus added the HttpKernel label Jul 27, 2021

carsonbot changed the title ~~PSR-6 HttpCache store~~ [HttpKernel] PSR-6 HttpCache store Jul 27, 2021

derrabus added this to the 5.4 milestone Jul 27, 2021

OskarStark reviewed Aug 1, 2021

View reviewed changes

jderusse reviewed Aug 8, 2021

View reviewed changes

fabpot modified the milestones: 5.4, 6.1 Oct 29, 2021

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

jlsuarez7 mentioned this pull request Jun 25, 2024

HttpCache stores metadata but not content for BinaryFileResponse which causes cache hits with no content. #9128

Open

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

	* When creating a Psr6Store you can configure a number options.
	* When creating a Psr6Store you can configure a number of options.

	private function saveDeferred(CacheItemInterface $item, $data, $expiresAfter = null, $tags = []): bool
	private function saveDeferred(CacheItemInterface $item, $data, int $expiresAfter = null, array $tags = []): bool

	class Psr6StoreTest extends TestCase
	final class Psr6StoreTest extends TestCase

		// This test will fail if an exception is thrown, otherwise we mark it
		// as passed.

Uh oh!

[HttpKernel] PSR-6 HttpCache store #42295

Are you sure you want to change the base?

[HttpKernel] PSR-6 HttpCache store #42295

Conversation

Toflar commented Jul 27, 2021

Issues with the current StoreInterface / HttpCache

Issues with this PR, things to be clarified

Uh oh!

OskarStark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OskarStark commented Aug 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter17 commented Jun 27, 2022

Uh oh!

fabpot commented Jun 27, 2022

Uh oh!

stof commented Jun 27, 2022

Uh oh!

fabpot commented Jun 27, 2022

Uh oh!

Uh oh!

Issues with the current `StoreInterface` / `HttpCache`