Consider whether HttpCache offers any significant benefit over the existing page cache

This needs benchmarks, we may well want to do it even if there's a regression, but let's not have another nasty surprise like #1064212: Page caching performance has regressed by 30-40% in D7 (which was due to a very tiny, innocent patch).

I'm also interested to see how this could tie in with cache tag support. We don't have proper content-based cache tags attached to cache items yet, but there's proof of concept code that integrates with cache tags with varnish already, so if varnish can handle it, then HttpCache should be able to as well.

is that OK? Where is that OK?

I think that's a site-specific question. You can currently set Drupal up to take either of these approaches via contrib and/or configuration, and we should be careful not to close either of them off to much (for example if you don't want content to clear on cache tags, it's 5 minutes to write a cache backend that ignores them).

By cache tags you mean a unique identifier for the content, so that if the tags are equal, then the content does not need to be regenerated? HTTP (and Symfony2) supports the validation approach of caching via ETags.

Here's a rough POC that I've been working on.

This should replace our default page caching with the file-based HttpCache. We'll obviously need to make additional storage backends, but I'm not against changing our default MySQL backend for flat files.

The biggest problem I see thus far is that the earliest we can serve a page from cache is after DRUPAL_BOOTSTRAP_CODE. This is much later than I'd like, but as long as we know there's work to be done to instantiate the HttpKernel earlier, this shouldnt be a blocker.

As soon as the bundles patch goes in, which I believe puts the config system into the DIC, we should start injecting it and eliminating the use of the config() wrapper function.

Agreed.

I especially like how this is 2/3 removing code rather than adding it. :-) Is that because the existing Symfony code does what we were doing, or because you just didn't get to that part yet?

A little of both. Its hard to follow at the moment because we left in a bunch of code that calls header() directly after the initial kernel patch. In this case it was easier to just rip that code out and get the cache working as we'd expect it to. A lot of what is removed is handling around 304s, which I think we're getting already. It's possible that we'll need to use HttpCache's validation to do some additional 304 checking.

Bundles patch is in. Hoping someone can get to this soon.

The patch is not what I would expect. The HttpCache is added at the end just before the kernel handle's the request. This is after the complete bootstrap. Shouldn't the cache be utilized in the DRUPAL_BOOTSTRAP_PAGE_CACHE phase. Is this even possible with the Kernel + HttpCache at that early stage?

Another thing we have to think about is the File Store because we can't assume we can use /tmp. How can we retrieve the file store location without getting the configuration from db?

@#15 I guess that full bootstrap is slowly being freed from its fat while services are being ported into the DIC. I hope that in stable 8.x bootstrap will only be a matter of a few millisec.

@#16 I think that this kind of early needed configuration variables should be set in settings.php (pretty much like advanced cache backends configuration).

What would be the proper solution to be able to override HttpCache and Store in for examply your settings.php?

I don't think I can answer that much, I'm, just like you, waiting to see a working patch to know :)

@#17 so should we just aim for removing the DRUPAL_BOOTSTRAP_PAGE_CACHE completely?

@#18 Then settings.php should be changed to have the configuration in it and it should be set during install. Also what to do with the UI where you can set the temporary directory?

@#20 I'll do something similar like the cache_backends:

$class = isset($cache_backends[$bin]) ? $cache_backends[$bin] : $cache_backends['cache'];
$cache_objects[$bin] = new $class($bin);

My latest version of the patch. Lets meet up at the sprint.

Forgot to add the HttpCache.

So these are the things I think we need to sort out:

1. Is FinishResponseSubscriber.php the correct place to set these headers with latest wscii branch?
2. How are we going to deal with stores. We can go for a few directions
   • a Drupal Store which will just use the current Drupal cache backend and therefore the modules like memcached, apc, filestore etc.
   • Use the default Symfony (File) Store. If people want to use a different store they can get the Components from Symfony
   • Create a DocTrine Cache Store which is able to leverage the caches of Doctrine currently in core.
3. Whats going to be the default store?
4. At the moment the cache is checked early in the bootstrap just after loading the settings file (DRUPAL_BOOTSTRAP_PAGE_CACHE) . At that point we do not have access to the kernel so a suggestion would be to just remove that fase (Though we would have to move the drupal_block_denied()). We can also remove the whole cache_backends part which was introduced for early page cache. One thing that's important would be that the bootstrap is just setting up but not actually booting the database connection and other intensive parts when the kernel is just going to serve the cache Response.
5. We did some changes to index.php. Are those ok?
6. The function drupal_page_is_cacheable() is something to set the state whether or not the page is cacheable and does some extra checks. For example in session.inc when it's an authenticated user a FALSE would be saved so the page isn't cached. The function however doesn't have access to kernel. Also the checks like it's a GET or HEAD request should be handled by the Response / HttpCache. I think one approach would be override the Response isCacheable method though I'm not sure how we can handle a total page state, HttpKernel?
7. When we are using symfony or doctrine store we should remove the cache_page cruft.
8. Make drupal_flush_all_caches work with HttpCache
9. How are we going to deal with the seperate caching systems. Contrib owners are used to do cache_clear_all(''). But after HttpCache there will be two different caceh clear approaches. Is this something we really want?
10. There is some If-Modified-Since logic in the old page cache which I do not understand fully yet. I don't think the HttpCache will deal with this for us.
11. In Drupal 7 we have cache_page which is stored as CACHE_PERMANENT unless you utilize the minimum page cache variable. The HttpCache however assumes a Ttl and does isFresh calls. There for we need a default_value (max_age) the page will be cached. We can set this on for example 5 minutes and allow the minimum page cache variable to change this. However it should be cool if you could change this for different types of Responses for example blocks, content areas, views on frontpage etc.

UPDATE: added 11

On 11: D7 stores cache_page (and cache_block) entries as CACHE_TEMPORARY, even with minimum cache lifetime.

If HttpCache assumes a ttl, it sounds like we may be scrapping cache minimum for cache maximum.

Triggering bot

+    return array(
+      'debug'                  => TRUE

Formatting issue

+++ b/core/lib/Drupal/Core/HttpCache.phpundefined
@@ -0,0 +1,29 @@
+    $config = config('system.performance');
+    return array(
+      'debug'                  => TRUE

Why define $config here if we're not going to do anything with it?

+++ b/index.phpundefined
@@ -13,7 +13,9 @@
 use Drupal\Core\DrupalKernel;
 use Symfony\Component\HttpFoundation\Request;
-
+use Drupal\Core\HttpCache;
+use Symfony\Component\HttpKernel\HttpCache\Store;

Would be nice if we could group all included Symfony libraries and all Drupal libraries, Symfony first.

+++ b/index.phpundefined
@@ -30,6 +32,11 @@ drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);
+if (config('system.performance')->get('cache') && drupal_page_is_cacheable()) {
+  $kernel = new HttpCache($kernel, new Store("/tmp/meuk"));

Another reason not to define $config within Drupal\Core\HttpCache if we're not going to use it. We're loading it again here.

Thanks to everyone who has recently reviewed this patch. Although many points brought up here are valid, I'd like to postpone most of them until we get the basics working with tests passing. For example, I'm purposefully not dealing with alternative stores atm. Just need to get the default working first. Also, being called so late in the bootstrap is a problem, but perhaps not this patch's problem. Let's revisit once it works.

Some changes in #1698108: Update Drupal's dependencies broke this patch pretty badly, so here's a quick reroll which I believe only takes those changes into account. Actual caching of pages seems to work correctly, but the tests that aren't passing are legit. We still need to figure out how and where to handle the logic around "page_cache_invoke_hooks".

Let's try that again.

if we intend to completely replace our caching mechanisms with the HttpCache, it might be beneficial to rip that out now and see what breaks. There seems to be a lot of issues involving the caching of user information. It's unclear to me what's the cause. With guidance I can try to reproduce the tests and focus on part of this patch.

Found quite a few problems caused by my subclassing HttpCache from HttpKernel instead of from the framework bundle which the docs show. That said, there's parts of the framework bundle version that we don't want, so I just pulled in the relavant bits into our HttpCache class. Attached is a new patch that fixes some of this.

Unfortunately, I don't think we can fix the bootstrap hook issues with the way things are now. We need to move all of our bootstrap, with maybe DRUPAL_BOOTSTRAP_CONFIGURATION as an exception, inside the kernel. It's very awkward that we're mixing them right now and too much work is already done by the time we even instantiate the kernel.

This would allow us to fix the hook issues and it would also eliminate the performance regression I mentioned earlier, so it's worth doing asap. I'm considering this issue blocked until that is done.

if we intend to completely replace our caching mechanisms with the HttpCache, it might be beneficial to rip that out now and see what breaks. There seems to be a lot of issues involving the caching of user information.

We can eventually replace all our HTML caching with HttpCache (i.e. page/block/drupal_render() in core), but there's plenty else that's not replaceable, and Symfony has plenty of its own caching (just renamed to compiling so it can pretend it's not).

@msonnabaum I'd consider having only one bootstrap path a critical task for Drupal 8 - it's OK if we have a small Drupal bootstrap before the kernel, but it's not really OK to have these two bootstraps mixed in with each other wrapping around.

I agree with catch that we should move it all in, but it turned out that it was a tiny change to get what I need and also not break anything by leaving DRUPAL_BOOTSTRAP_CONFIGURATION out.

However, this has the rest of the bootstrap in the DrupalKernel, not the HttpCache wrapper. We all talked in IRC and were mostly in favor of removing the "normal" cache setting in favor of "aggressive" only. Supporting bootstrap hooks will be messy, so whether we can do it eventually or not, this is the simplest version start with.

Actually, as beejeebus pointed out to me, having the bootstrap in init() doesn't fix anything. It needs to go in boot().

Hi,

Just my 2 cents after the discussion with beejeebus on IRC to give the same knownledge to others.

Wrapping the kernel in the HttpCache will indeed be bad when you want to use Varnish, as it would continue to handle the ESI in PHP instead of letting Varnish doing it.

However, it is possible to check very easily if the HttpCache is needed. See https://gist.github.com/3551077

I haven't read the full discussion and I haven't checked the latest status of Drupal, so forgive me if the following advice is useless now.
When using the HttpCache, the instantiation of the DrupalKernel should be as lightweight as possible, as it will be done even when the cache is used (and so the drupal code does not need to be called). The heavy initialization should be done only when booting the kernel (which will occur only on cache miss, when the HttpCache needs to forward the request to the DrupalKernel).
If the instantiation of the kernel is heavy, you will have a bigger performance impact when switching between HttpCache and Varnish (as Varnish will also avoid the instantiation when the cache is used of course).

EDIT: here the code so that you don't need to go to the gist, as your comments support syntax highlighting

use Drupal\Core\DrupalKernel;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\HttpCache\Esi;
use Symfony\Component\HttpKernel\HttpCache\HttpCache;
use Symfony\Component\HttpKernel\HttpCache\Store;

// Build a drupal kernel
$kernel = new DrupalKernel()

$request = Request::createFromGlobals()

$esi = new Esi()
if (!$esi->hasSurrogateEsiCapability($request)) {
    // There is no reverse proxy supporting ESI in front, so add the Symfony HttpCache
    // this is not executed when you have Varnish in front and properly configured
    $kernel = new HttpCache($kernel, new Store(), $esi);
}

$response = $kernel->handle($request);

The hasSurrogateEsiCapability() function depends on headers from the proxy:

    public function hasSurrogateEsiCapability(Request $request)
    {   
        if (null === $value = $request->headers->get('Surrogate-Capability')) {
            return false;
        }   

        return false !== strpos($value, 'ESI/1.0');
    }  

The ESI module already has a client-side JavaScript implementation for ESI, so there's a use-case where ESIs should be delivered, but there are no HTTP headers to indicate this.

I'd prefer to handle it via config instead.

@manarth Could the JS add additional headers instead as part of the downgrade behavior?

I completely agree that the "Surrogate-Capability" header is not enough to decide whether or not to use HttpCache. We should use either config or a $conf variable.

One thing that occurred to me while looking at this again however, is that our cache settings don't make much sense now that we've removed CACHE_TEMPORARY. We still have a cache.page.enabled option, yet it does nothing unless you set cache.page.max_age.

I'm thinking we should remove cache.page.enabled and replace it with http.cache.use.internal. If you are using a reverse proxy, all you have to do is set the max_age to something. I started on that but noticed we're checking the existing cache.page variables in some odd places, so I wanted to make sure we had consensus before making that change.

We should also conditionally pass the esi object in \Drupal\Core\HttpCache's constructor since we don't need that running if you don't have the equivalent of block_cache = TRUE. That can probably just be a @todo for now since removing block cache is out of scope for this issue.

I'm saying that the max age setting determines whether pages get cached. If it's at 0, drupal sends out the typical "Expires: Sun, 19 Nov 1978 05:00:00 GMT" that you'd get when page cache is off. If it's > 0, we send out the appropriate cache headers.

The only thing you explicitly enable is the internal page cache, which would mean we wrap the kernel with HttpCache.

I'd be fine with #58, we'll need to update the help text on the performance settings to reflect the change. Also fwiw completely fine with having the default max_age as 5 minutes (probably only in the standard profile).

Created #1853086: Remove cache.page.enabled in favor of an explicit internal cache setting for the changes I described in #58.

Why???
- Because we want to have a single cache API all the way through, regardless of where it's getting cached. That cache API is Http. This lets a controller set a response object and headers on it, and they'll be honored regardless of where or how the response is being cached.
- Because we want to eliminate the "anonymous caches forever, authenticated never caches" distinction and move toward "things cache when they should cache, and don't when they shouldn't". This is a step in that direction.

None of this has anything to do with wrapping $kernel with HttpCache.

There are some worthwhile changes here, but it really should be handled outside of trying to use HttpCache. This issue should be about making page cache work with the new Request/Response objects.

+++ b/core/includes/bootstrap.incundefined
@@ -2207,6 +2104,11 @@ function drupal_handle_request($test_only = FALSE) {
   $kernel->boot();
   drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);
 
+  if (settings()->get('use_http_cache', TRUE)) {
+    $store_path = variable_get('file_public_path', conf_path() . '/files/http_cache');
+    $kernel = new HttpCache($kernel, new Store($store_path));
+  }
+

Pretty sure we don't want the kernel booted for cached pages.

looking at the patch in #64, my primary comment is along the lines of katbailey's in #66 - if we're going to wrap the main kernel with the cache kernel, it needs to be done a little earlier. i'm not sure about booting/not booting the kernel, but i am sure that we should be slotting in this caching mechanism as early in bootstrap as possible, at least similar to where the old page caching implementation was. that may have implications on where/how we set the storage location var, but that shouldn't be awful - sys_get_temp_dir() is diiirty, but perhaps adequate for these purposes?

beyond that, i agree with #65 - the rest of this issue is about fixing our interaction with Request/Response elsewhere in the code. fixing the failed tests should be a good start to that.

Re-roll of #64.

To clarify, we have NOT removed support for enabling cache from the UI, and I do not think we should.

Is this what you're referring to? #1853086: Remove cache.page.enabled in favor of an explicit internal cache setting

Yes, my patch only removed the "turn cache on" setting because it was redundant with max age. The setting is now "use internal cache", which you'd only check if you didn't have a reverse proxy cache.

Yeah, that would make me very sad. It's not clear to me why it would be so terrible to remove support for enabling the internal cache via the UI. Would be good to get some clarification on that.

Why are we talking about needing page cache to come before the kernel is booted? We're again trying to solve problems we don't have while we have many other caching related problems to work on.

Changing the title to better reflect reality. This is still postponed and dependent on the rest of core using the kernel properly.

Doctrine/commons is recommended here for caching instead of httpcache. Great read, take a look http://nerdpress.org/2012/07/10/caching-data-in-symfony2/

That article refers to using Doctrine/commons for caching data, rather than rendered pages/page fragments, and does acknowledge that for HTTP caching, the httpcache is fine.

Note that ~/www/d8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpCache/HttpCache.php has an eval() in it which should be considered a blocker.

#2263981: Introduce a robust and extensible page cache-policy framework just landed.

According to #2257695: [meta] Modernize the page cache, this is now the next step. Not sure though, since this is now marked for 9.x-dev…

Moves the pieces into the right place, but currently fails because session_name() is set too late (should be fixed in #2331909: Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator).

Moves the pieces into the right place, but currently fails because session_name() is set too late (should be fixed in #2331909: Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator).

Should this also remove some overhead and make Drupal 8 faster? I have no idea, I'm curious :)

This and other failures in PageCacheTest suggest that HttpCache doesn't follow the HTTP spec:

Conditional request without If-None-Match returned 200 OK.

… or that Drupal's current page cache doesn't, of course. At least it's a change in behavior that we should assess.

Why are we trying to fix something that's not broken again?

*claps*

That's some masterful trolling there.

We are not adding another eval() into Drupal after years of work removed them. Just say no.

Especially not when

I can't tell if the purpose of it is to protect against code injection or to specifically allow it in a special circumstance.

so postponed until at least we know more about that eval(). This status was made for it.

+++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
@@ -7,49 +7,113 @@
+    // Disable the file inclusion / evaluation feature of the parent class.
+    $response->headers->remove('X-Body-Eval');
+    $response->headers->remove('X-Body-File');

The eval is indeed ugly, but we can actually disable that part. Probably we'd need to do that in lookup as well.

That said I'm okay with postponing this. Given that the page cache now lives in a stack middleware it is easy enough to swap it out.

This is the offending code:

    /**
     * Restores the Response body.
     *
     * @param Request  $request  A Request instance
     * @param Response $response A Response instance
     */
    private function restoreResponseBody(Request $request, Response $response)
    {
        if ($request->isMethod('HEAD') || 304 === $response->getStatusCode()) {
            $response->setContent(null);
            $response->headers->remove('X-Body-Eval');
            $response->headers->remove('X-Body-File');

            return;
        }

        if ($response->headers->has('X-Body-Eval')) {
            ob_start();

            if ($response->headers->has('X-Body-File')) {
                include $response->headers->get('X-Body-File');
            } else {
                eval('; ?>'.$response->getContent().'<?php ;');
            }

            $response->setContent(ob_get_clean());
            $response->headers->remove('X-Body-Eval');
            if (!$response->headers->has('Transfer-Encoding')) {
                $response->headers->set('Content-Length', strlen($response->getContent()));
            }
        } elseif ($response->headers->has('X-Body-File')) {
            $response->setContent(file_get_contents($response->headers->get('X-Body-File')));
        } else {
            return;
        }

        $response->headers->remove('X-Body-File');
    }

As you can tell, wonderfully undocumented. Googling for it only yields results that simply list source code. Zero documentation to be found.

This seems to be a case where the use of HTTP as standardtool even for internal documentation, falls down. On the other hand I don't even understand why this is needed.

Also, HttpCache can do its own ESI tag handling whereas our current cache system does not, as far as I am aware.

Is there any reason why our system could not be expanded to do that?

Per #99 I looked at blame and this was added by Fabien Potencier in the initial commit of the cache system during 2010, was also unable to find any documentation.

HttpCache "doing it's own ESI tag handling" is a red herring and we should really examine what this means between HttpCache and Drupal's render caching and the various use cases. Have had enough of empty assertions constantly repeated.

Much of Symfony's use of ESI is to avoid having to deal with cache invalidation. They emphasise setting short expires on content listings so that they're 'fresh', then longer expires on the overall page. http://symfony.com/doc/current/book/http_cache.html has quite a long lead in, that boils down to "it's hard to do cache invalidation so we don't really bother but you can try if you want".

By implementing cache tags in Drupal 8, and specifically the bubbling of them from child elements to parents, we've almost entirely removed the need for different TTLs on cached content. All elements of the page, as well as the page itself, can have a very long TTL. When we invalidate, only parents of the cleared content get affected, children and siblings (etc.) can still be fetched from cache. Varnish has support for invalidating cache tags already since we put them in a custom header, so this also works with reverse proxies.

This means that for anonymous users, our cache hit rates should be considerably higher already than if we were using Symfony's short TTL + ESI tag approach - we don't need ESI to fix that particular performance issue, it's already solved with cache tags.

There's still things to clean up, like figuring out Etags #2259489: Leverage symfony for HTTP revalidation, but again HttpCache doesn't help with that at all.

Apart from different TTLs on content, the actual valid use case for ESI remaining is personalisation.

#post_render_cache is what we settled on for handling placeholders and replacement within HTML. Rather than using this for different TTLs, we only use that in Drupal for personalisation at the moment, because we just don't need different TTLs any more.

Since #post_render_cache replacement in PHP can be bypassed if necessary, it's possible to send the render cache placeholders to the reverse proxy or browser and replace them there instead of PHP (via Redis + memcache, Varnish or Akamai ESI tag with some massaging, or JavaScript). We don't need anything extra for actual ESI support except for ensuring this works and cleaning up various bits of work that are quite far along.

So the remaining question is whether we should add #post_render_cache support to the full page cache. With cache tag support, there is no value to doing this for anonymous users - we already have a viable model for all but the most extreme edge cases (and those can and should be handled by JavaScript usually).

What there could be value in doing though, is implementing 'full page caching' for authenticated users, then using #post_render_cache for personalisation. This means we'd no longer need to render the page template each time, which is one of the main places we could save time in HTML rendering that is not viable to cache for authenticated users. I think Fabianx has plans to do this, but again the benefit is for authenticated users - it'd be making the page cache viable for them too.

Another reason that Symfony's approach to ESI doesn't really help us, is that the recommended way to implement ESI is by doing subrequests inside Twig templates (see the docs at http://symfony.com/doc/current/book/http_cache.html#using-edge-side-incl...). That only works if you can decide beforehand where the dynamic content should be included in your template. I don't think that ESI caching is a decision that should be made in a Twig template.

See Wim, Marco and Fabian's talk here for a lot more detail: https://amsterdam2014.drupal.org/session/render-caching-drupal-7-and-8

So as Nat said, with using a placeholder based approach where placeholders are better defined than just post_render_cache random functions and everything does our own we can easily do:

- ESI
- AJAX
etc.

The advantage is that we can get data from the cache also directly, which is superior to internal micro ESI requests to assemble a page, which would not even propagate the render chain correctly.

The only advantage in my opinion is that HttpCache allows to save pages to disk, but that is something we can also have easily with the layer based system we have been building.

But we would need to extend it anyway to handle #attached, so in this case we gain not much as we can't use stock anyway, the way Drupal works ...

And for ESI and Varnish caching, etc. to work properly there are other things we need to take care of first, too - for example fix form and route CSRF handling.

It is OSS software, so if this will be better and cool and allow some really nice things that we really need, for all means lets implement it and let the implementation prove why its so more cooler for itself, but just for the sake of using it, I would say this needs to show why it is so much better first.

- So using a persistent page cache on disk? Yes, we can do that.
- Using HttpCache out of the box? We probably do not gain much at this point anymore.

Leaving as postponed for now.

I will create the meta for the authenticated user page cache as we are sooo close to having that working - or at least empowering contrib to do so and do it in 8.1 or such.

The next step in #2257695: [meta] Modernize the page cache is #2348679: Move the remaining procedural page cache code to the page cache stack middleware which will pack remaining scattered bits of internal page cache into a swappable middleware.

I will create the meta for the authenticated user page cache as we are sooo close to having that working - or at least empowering contrib to do so and do it in 8.1 or such.

He, the very reason I actually started working on the now superseeded #2177461: Refactor page caching into a service issue was that I realized that a D8 port of Authcache will be much easier when core gets more flexible in this regard.

So there's some capability to set ESI tags and then ... you resolve them request time by making more Drupal requests? That may be useful for authcache but for normal cache I would rather not sure anything like that...

We do support something like that via #post_render_cache already now in Core (without eval) and we need to store additional information anyway like cache tags, so as already stated this Component is not useful at the moment.

But thanks for the information!

Symfony 2.6.6 was just released. With a security hole in their ESI support due to that eval().

See http://symfony.com/blog/symfony-2-6-6-released, and the fix at https://github.com/symfony/symfony/commit/195c57e1f50765aff33137689b16e1... ("Safe escaping of fragments for eval()").

The code remains equally undocumented and has become only less comprehensible.

chx++ for saying 'no'

Also we are close to have #102 working in core now.

http://symfony.com/blog/cve-2015-2308-esi-code-injection

Sounds at least like a 8.1 work if not 9.x or never

+1

This has been PNMI for 7+ years. Think it can be closed out but if still a valid task in 11.x we should move to Active or NW with an updated issue summary. Till then closing out though.