Page cache isn't invalidated

Created on 17 October 2017, over 7 years ago
Updated 17 July 2023, over 1 year ago

I installed this module because I have a custom module that sets max-age in render arrays that don't invalidate the page cache properly. While debugging I found that Cache-Control is being set propely. But it doesn't affect internal page cache. Am I missing something?

πŸ’¬ Support request
Status

Needs review

Version

1.0

Component

Code

Created by

πŸ‡¨πŸ‡­Switzerland luksak

Live updates comments and jobs are added and updated live.

Missing content requested by

πŸ‡¦πŸ‡ΊAustralia dpi
over 1 year ago
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @luksak
  • πŸ‡¨πŸ‡­Switzerland luksak

    Looking at the cache_page table, the problem is obvious: all pages have expire set to -1.

  • πŸ‡¨πŸ‡­Switzerland luksak

    My cache metadata was not properly output by Twig.

    After that page cache got disabled completely. How can I debug where in the render process max-age is set in a way that makes all pages uncachable?

  • πŸ‡©πŸ‡ͺGermany rgpublic DΓΌsseldorf πŸ‡©πŸ‡ͺ πŸ‡ͺπŸ‡Ί

    I found debugging stuff like this extremely hard. I had some success going to "lib/Drupal/Core/Cache/CacheableMetadata.php" and e.g. doing a backtrace in merge() when an unwanted max-age:0 is about to get merged in.

    Besides that, I still think this issue is very valid. This module currently only fixes the fact that a default Drupal installation doesnt properly bubble the Max-Age to the Cache-Control HTTP header. It doesnt do anything about the fact that the page cache doesnt support time-based expiration *at all*. Of course, I could write another module for this. But I think it might make sense to integrate it here, so we don't get to a point where we require a whole library of cache fixing modules to get a decent working caching mechanism in Drupal...

  • πŸ‡©πŸ‡ͺGermany rgpublic DΓΌsseldorf πŸ‡©πŸ‡ͺ πŸ‡ͺπŸ‡Ί

    Ah. I see, I'm wrong. Page Cache seems to use $expire in core/modules/page_cache/src/StackMiddleware/PageCache.php, storeResponse. But I also found that only -1 ends up in the cache_page table. Weird.

  • πŸ‡«πŸ‡·France prudloff Lille

    Hello,

    The issue here is that PageCache::storeResponse() uses the Expires header but not Cache-Control to invalidate the page cache.
    The attached patch helps workaround this problem.

  • πŸ‡«πŸ‡·France prudloff Lille

    I just noticed that #2962699 contains a similar patch.

  • πŸ‡ΊπŸ‡ΈUnited States DamienMcKenna NH, USA
  • πŸ‡«πŸ‡·France prudloff Lille

    Here is a reroll for 2.0.0-rc2.

  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 8
    last update over 1 year ago
    4 pass
  • πŸ‡ΊπŸ‡ΈUnited States DamienMcKenna NH, USA

    Rerolled for 8.x-1.x.

  • πŸ‡«πŸ‡·France Grimreaper France πŸ‡«πŸ‡·

    Hi,

    Thanks for the reroll on 2.0.0-rc2. I use this patch now instead of #2962699-5: Expires header not set β†’ for the version 8.x-1.x.

  • πŸ‡«πŸ‡·France Grimreaper France πŸ‡«πŸ‡·

    cache_control_override needs to take global max-age into account.

  • Open on Drupal.org β†’
    Core: 10.1.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update over 1 year ago
    Not currently mergeable.
  • Open on Drupal.org β†’
    Core: 10.1.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update over 1 year ago
    Not currently mergeable.
  • Assigned to Grimreaper
  • πŸ‡«πŸ‡·France Grimreaper France πŸ‡«πŸ‡·
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.1.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update over 1 year ago
    4 pass
  • Issue was unassigned.
  • Open in Jenkins β†’ Open on Drupal.org β†’
    Core: 10.1.x + Environment: PHP 8.1 & MariaDB 10.3.22
    last update over 1 year ago
    4 pass
  • πŸ‡«πŸ‡·France Grimreaper France πŸ‡«πŸ‡·

    Here is the patch from MR for Composer usage.

  • πŸ‡«πŸ‡·France prudloff Lille

    We should probably decide which patch to keep between πŸ› Expires header not set Needs review and πŸ’¬ Page cache isn't invalidated Needs review .

  • Pipeline finished with Failed
    7 months ago
    Total: 135s
    #262023
  • πŸ‡«πŸ‡·France Grimreaper France πŸ‡«πŸ‡·

    MR rebased for version 2.0.1

  • Pipeline finished with Success
    26 days ago
    Total: 149s
    #442685
  • πŸ‡«πŸ‡·France prudloff Lille

    Hiding patches to make it clear the MR should be reviewed.

  • πŸ‡«πŸ‡·France prudloff Lille

    Added a comment on the MR.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    FWIW, I disagree with considering the max age and that is a change to how core handles this.

    Core specifically only sets the max age setting on the response and the internal page cache is forever. If you have no reverse proxy or something like this then it is common to set cache.page.max_age to a low value, because you have no means to invalidate it.

    The UI label for the core setting is specifically " Browser and proxy cache maximum age".

    Then again, our custom implementation of this only sets the Expires header and doesn't touch max-age at all. This is also how I imagine the core implementation will behave, it will consider cacheability max-age for the internal page cache, without altering the Cache-Control header.

  • πŸ‡«πŸ‡·France prudloff Lille

    @berdir I understand your point but I think people using cache_control_override often use a reverse proxy and a high max age setting.

    However I agree the patch should not use the cache.page.max_age setting from core.
    Since #3003716: Coerce bubbled max-age to a floor or ceiling β†’ , cache_control_override has a specific max age setting different from the core one and the patch should use that.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Yeah, it's tricky.

    FWIW, the project page explicitly talks about solving the problem of the referenced core issues, which it does in fact not do yet, it does something different. So that seems a quite weird. This issue then actually makes it do what it says there, but it still does the max-age, which might result in quite unpredictable results.

    The max max age check is already there above, so yes, I think the extra check can be removed.

    Still, the problem IMHO is the combination of the two headers and applying the same rules to them.

    For external/proxy caching, having a maximum max-age makes a lot of sense, most sites I think don't really have any means of cache tag based invalidation, definitely not in browsers ([#/3350593]). But applying that same max-age on the internal page cache could result in considerably lower cache hit rates. Then again, it will only run if there is a specific max-age set that's not permanent. permanent is still default core setting.

    It all seems quite unpredictable. I'm not a maintainer, I just ended up here because I've been telling people in the past, including in those core issues to use this project, only to realize that it doesn't actually do what I expect.

    As you said, it probably also depends on your use case and how you handle caching. For us, that's basically two things:
    1. We set a low max-age (like 15min), which means that every 15min or so, a given page needs to be revalidated against the internal page cache, which caches by default forever.
    2. For a few specific pages, we want to have limited and lower max age/expire in the internal page cache, for example event listings.

    So this currently, with and without this issue, isn't a good fit for us and I'd assume also many other small to medium sites that don't have a custom reverse proxy with invalidation support.

    I think the most sensible thing for this project would be to have 3 different on/off settings for max-age, s-max-age and expires, each with it's own separate min and max. And for a consistent result, I'd then always enforce the given max-age setting, even if the cacheability metadata says permanent. Then you can customize it to fit your use case.

  • Pipeline finished with Canceled
    8 days ago
    Total: 152s
    #457113
  • Pipeline finished with Failed
    8 days ago
    Total: 162s
    #457114
  • Pipeline finished with Success
    8 days ago
    Total: 145s
    #457120
  • πŸ‡«πŸ‡·France prudloff Lille

    I added logging if setExpires() fails.

    And I stopped using the page cache max age setting as @berdir suggested.

Production build 0.71.5 2024