Problem/Motivation

When the http.response.debug_cacheability_headers container parameter is set to true, Drupal will add X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts headers to the HTTP response, so that you can see/debug what tags and contexts have bubbled to the response. It's possible that the number of tags (or contexts, though less likely) on a page can be so numerous that the length of the header lines exceed the ~8k line length limit enforced by Apache. If this happens, Apache will show a blank page with a 5xx status code. It can be difficult for developers to debug what has happened, because there will be no entries in the Drupal log nor any exceptions or errors at the PHP level. The only evidence usually is an opaque error message in the Apache logs.

Generally this issue is limited to local or dev environments where http.response.debug_cacheability_headers is purposefully set to TRUE for development, but it can be possible for this setting to make it to production, either mistakenly or intentionally.

Note: a previous version of this IS mentioned CDNs and reverse proxies not being able to handle large cache tag headers. Issues with those should be addressed in the contrib or custom modules that provide the integrations, since every provider has different methods, whether through other custom response headers or through API requests, to do cache tag invalidation. This issue only addresses the X-Drupal-Cache-* headers used for debugging and development.

Steps to reproduce

Enable http.response.debug_cacheability_headers: true and look for a page with many related cache tags so the X-Drupal-Cache-Tags header is larger than 8K. On servers running Apache, the response will be a WSOD with a 5xx status code.

Proposed resolution

  1. Limit X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts headers to 8K.
  2. If any header is bigger than that split the value into multiple instances of the header. Apache will usually be configured to merge the headers back together on output
  3. Apply this to any headers that potentially exceed the maximum length.

Update documentation.

Remaining tasks

Review and comment on what was done to review this issue and the MR.
Resolve issues on MR
Issue summary update. Does this need an release note snippet
Release note and/or change record seem unnecessary. The fix does not affect anything functionally or visually.

What Documentation needs to be updated? Is it the list in #130?
See #169:
If anything:

one or both of the following would suffice https://www.drupal.org/docs/drupal-apis/render-api/cacheability-of-rende...
and https://www.drupal.org/docs/8/api/responses/cacheableresponseinterface#d....

Something like "Note that on some servers, such as those running nginx, the headers could be split across multiple lines with the same name if there are a lot of tags or contexts. On those running Apache, the tags or contexts will be on a single line, but may be separated by commas at intervals of around 8000 characters in length."

User interface changes

None.

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

Issue fork drupal-2844620

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

adam.weingarten created an issue. See original summary.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

wim leers’s picture

Title: Need warning/error when headers exceed 16k » FinishResponseSubscriber: Need warning/error when headers exceed 16k
Component: cache system » request processing system
dawehner’s picture

Downstream systems are being updated to support this. If Drupal sends a request that exceeds 16k the downstream systems will fail with inconclusive errors. For example Varnish will return "HTTP/1.1 502 Bad Gateway." Very few people will be able to debug this sort of situation. We recently encountered a situation with a 50k header!

It is a setting, right? I wonder whether the status page, similar to clean URL detection could maybe send out a couple of HTTP requests to test the size we support?
Hardcoding to some limit feels a little bit weird to be honest.

adam.weingarten’s picture

So partners from Fastly, Akamai, and CloudFlare are asking for a standard so that they can ensure that their systems will reliably work with drupal. Tons of people also use varnish.

If we leave it variable then this could get very messy. We need to come up with a number that people can expect to work. We could make it overridable via settings.php, to address the concerns that you are raising, but that should be exception.

Also if the headers exceed a number like 16k something is really wrong with your content model!

wim leers’s picture

I wonder whether the status page, similar to clean URL detection could maybe send out a couple of HTTP requests to test the size we support?

I like that!

Also if the headers exceed a number like 16k something is really wrong with your content model!

This.

adam.weingarten’s picture

Ultimately we need to define interoperability standards with respect to headers so external systems can be built to work with Drupal.

wim leers’s picture

dawehner’s picture

Thank you @Wim Leers for clarifying the docs!

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fgm’s picture

Just got bit by this today, while working on a site without even a reverse proxy: a page had 9kB of the single X-Drupal-Cache-Tags header (enabled for debug), which will probably translate to a comparable header length once converted for Varnish xkey.

The problem in this case happens in Apache, before touching the RP itself, as can be demonstrated by this short fragment outside Drupal itself to avoid side effects:

$len = 8179; // Works with 8178 and below.
$tooMuch = str_repeat('z', $len);
$header = "X-Toomuch: $tooMuch";
$maxLen = strlen($header) + 2; // CR/LF
header($header);
echo "Hello $maxLen, goodbye\n";

The problem here is that one can debug what's going on and step to the last execution line without any error, and still get a 500 in the client. Worse, there is no information in the PHP error log because there is no PHP error involved, and no information in Drupal logs, because it is not strictly Drupal-related either, so it is very hard to pinpoint indeed.

So that double limit should probably also be enabled and documented. Being a per-environment information, it should probably be a setting, with reasonable defaults like 8000/line, 16k for the whole set. And when caught, should probably truncate or drop/replace the offending header AND log and error, not even a warning.

fgm’s picture

Version: 8.5.x-dev » 8.4.x-dev
Category: Feature request » Bug report
Status: Active » Needs work

FWIW, here is an example of a quick workaround I added to solve the issue. It is not really good since it means the header will be wrong, but at least it prevents the 500 and logs an error.

Also, converting to a bug against current version as it is poised to occur more and more often as sites get more complex every day.

# In <mymodule>.services.yml
  mymodule.finish_subscriber:
    class: Drupal\mymodule\EventSubscriber\FinishResponseSubscriber
    arguments:
      - '@current_route_match'
      - '@logger.channel.mymodule'
      - '@settings'
    tags:
      - { name: event_subscriber }

namespace Drupal\mymodule\EventSubscriber;

use Drupal\Core\Routing\CurrentRouteMatch;
use Drupal\Core\Site\Settings;
use Psr\Log\LoggerInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;

/**
 * Class FinishResponseSubscriber
 *
 * @see \Drupal\Core\EventSubscriber\FinishResponseSubscriber
 */
class FinishResponseSubscriber implements EventSubscriberInterface {

  /**
   * A hash of default maximum header length by header name.
   */
  const HEADERS = ['X-Drupal-Cache-Tags' => 8000];

  const LENGTH_SETTING = 'max_header_length';

  /**
   * The logger.channel.mymodule service.
   *
   * @var \Psr\Log\LoggerInterface
   */
  protected $logger;

  /**
   * The current_route_match service.
   *
   * @var \Drupal\Core\Routing\CurrentRouteMatch
   */
  protected $match;

  /**
   * The settings service.
   *
   * @var \Drupal\Core\Site\Settings
   */
  protected $settings;

  public function __construct(
    CurrentRouteMatch $match,
    LoggerInterface $logger,
    Settings $settings
  ) {
    $this->logger = $logger;
    $this->match = $match;
    $this->settings = $settings;
  }

  /**
   * {@inheritdoc}
   *
   * @see \Drupal\Core\EventSubscriber\FinishResponseSubscriber::getSubscribedEvents()
   */
  public static function getSubscribedEvents() {
    $events[KernelEvents::RESPONSE][] = ['onRespond', -1];
    return $events;
  }

  public function onRespond(FilterResponseEvent $event) {
    $response = $event->getResponse();
    $headers = $response->headers;
    foreach (static::HEADERS as $name => $defaultLength) {
      if ($headers->has($name)) {
        $limit = $this->settings->get(static::LENGTH_SETTING, $defaultLength);
        $header = $headers->get($name);
        $actualLength = strlen($header);
        if ($actualLength > $limit) {
          $headers->set($name, substr($header, 0, $limit));
          $this->logger->error('Header line @name too long: @actual (limit: @limit) on @route', [
            '@name' => $name,
            '@actual' => $actualLength,
            '@limit' => $limit,
            '@route' => $this->match->getRouteName(),
          ]);
        }
      }
    }

    return;
  }
}
regilero’s picture

The HTTP RFC has nevers contained an official limitation on the size of one header, just this note:

3.2.5. Field Limits

HTTP does not place a predefined limit on the length of each header
field or on the length of the header section as a whole, as described
in Section 2.5. Various ad hoc limitations on individual header
field length are found in practice, often depending on the specific
field semantics.

(...)

And in 2.5 we have this very generic sentance:

HTTP does not have specific length limitations for many of its
protocol elements because the lengths that might be appropriate will
vary widely, depending on the deployment context and purpose of the
implementation. Hence, interoperability between senders and
recipients depends on shared expectations regarding what is a
reasonable length for each protocol element. Furthermore, what is
commonly understood to be a reasonable length for some protocol
elements has changed over the course of the past two decades of HTTP
use and is expected to continue changing in the future.

But for decades HTTP servers have hard coded security limits on length of headers. And 8k has been the hard coded are-you-crazy-or-what limit in Apache httpd.

Trying to reach 16k seems strange, I'm pretty sure a lot of WAF would silently remove http messages using such headers.

fgm’s picture

There are apparently two different limits involved:

  • a maximum line length, apparently around 8170 bytes (not 8192) in apache 2.4
  • a maximum total byte count for all the headers, apparently at 16kB

I'm not sure what your point is about "trying to reach 16kB" and WAFs: in most cases, the configurable limitations are on request headers, not response, and the problem we have with cache tags is about responses. Complex pages can quite easily go above either (or both) limits.

regilero’s picture

Limits are usually shared also by responses. I once reached nginx buffers limits by settings a big CSP header on responses. On this case it was fixed by altering the buffer limit, but as stated earlier you cannot always alter settings on cloud hosting.

fgm’s picture

In Apache at least, they are separate: the request limit is configurable, while the response limit appears not to be.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mr.baileys’s picture

vaza18’s picture

This patch is taken from X-Drupal-Cache-Tags / X-Drupal-Cache-Context header tags can exceed HTTP Server limits and cause WSOD issue which was closed as a duplicate of this one.

The patch is actually doesn't give a warning but removes an issue at all by splitting too long headers into pieces. Considering those headers are used only for development purposes this looks working solution for me.

vaza18’s picture

Version: 8.6.x-dev » 8.7.x-dev
Status: Needs work » Needs review
fgm’s picture

Patch is interesting. However, it has two issues:

  • the enumerated names make it difficult to work with it. It would probably be better to use just repeated headers: repeated headers are allowed as detailed on https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2 . However, they should be equivalent to a single comma-separated header while ours is space-separated, using the commas for another purpose
  • it breaks compatibility with the existing format: some sites are more than likely to be using these debug headers as actual production headers, even if they don't expose them after Varnish, and it would break them
mr.baileys’s picture

Aside from the concerns in #22, I'm not convinced that the suggested approach of splitting the headers will actually work, as some proxies (like nginx) have a single limit for the request line and all headers combined, not for each header line individually.

trevorbradley’s picture

Just a note that the patch in #20 just fixed an absolutely infuriating issue for me that's been plaguing me for months.

I'm working in a docker environment, where a rather large search_api_solr page request was being passed to proxy_fcgi. On smaller pages, the page would render just fine, but on larger ones, the web server would throw an error 500 with a "Premature end of script headers". This was only on my local docker dev environment - on a full apache deploy, there were no issues.

I finally watched the debug output, and noted that the fcgi proxy server was generating the page perfectly. It would attempt to send headers then die when trying to send the response. I dug deeper into the headers and found a monstrosity in Response.php: there was an 'x-drupal-cache-tags' header that was 12581 characters long!

I droped patch #20 into place to see if the issue would go away, and the page loaded perfectly.

Now, there's likely an issue independent of this one - should this particular header be so big? My page is assembled from hundreds of paragraphs, taxonomy terms and nodes, but should that generate such a large header?

jimmynash’s picture

I was also experiencing this issue in a docker environment on mac (Docksal).

Applying the patch in #20 did the trick for me.

berliner’s picture

Same situation as #25 and I can confirm that the patch fixes the issue. Didn't look at all at what the patch does though.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.9 was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. (Drupal 8.8.0-beta1 is available for testing.)

Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

sittard’s picture

For others experiencing this issue we noticed our development.servives.yml file had the following configuration:

http.response.debug_cacheability_headers: true

Changing this to false sorted the issue.

anybody’s picture

I can also confirm this issue in a Plesk driven environment. #28 fixed it for us!
The patch in #20 but I'm not experienced enough to prove the concerns for a logical / technical RTBC. My RTBC+1 is just empirically. At least I can surely confirm we have a bad issue / bug here for sure. ;)

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

wells’s picture

+1 for the patch in #20 -- resolved the same issue for me of excessive cache tags in the header. What can done to move this issue forward wrt to the concerns raised in #22 and #23?

hoporr’s picture

#28 fixed this in our server environment (Plesk).

In this case it is a geomap view, that pulled a lot of points (runs fine on local box, but had this error on server).

司南’s picture

This patch should be merge or we can not use the dev site mode.

bappa.sarkar’s picture

psf_’s picture

The patch in #20 works for me. My problem is described in the old issue #2954339.

wells’s picture

Version: 8.9.x-dev » 9.3.x-dev

Patch in #34 applies and resolves the issue in 9.x as well. Thanks, @bappa.sarkar!

The concerns in #22 still seem valid and should be considered and addressed. It seems that changing the approach to use repeated instead of enumerated headers would resolve both concerns, correct? I'm going to retarget this to 9.3.x-dev as this feels more like a new feature/disruptive change than a bug fix at this point.

wells’s picture

Status: Needs review » Needs work

Oops -- forgot to also set back to needs work pending follow up on #22.

oheller’s picture

I'm using php7.4, Drupal 9.2.1 with docksal. None of the above seem to work. I've set all `http.response.debug_cacheability_headers: false` cleared the caches, applied patch 34 and the latest patch from 2954339, restarted docksal, restarted docker.

I've also added .docksal/etc/apache/httpd-vhost-overrides.conf with

LimitRequestFieldSize 20000
LimitRequestLine 20000

And I continue to get the proxy_fcgi:error.

web_1   | [proxy_fcgi:error] [pid 106:tid 140555449727720] [client 172.20.0.7:43560] Premature end of script headers: index.php
web_1   | [proxy_fcgi:error] [pid 106:tid 140555449727720] [client 172.20.0.7:43560] AH01070: Error parsing script headers
web_1   | [proxy_fcgi:error] [pid 106:tid 140555449727720] (22)Invalid argument: [client 172.20.0.7:43560] AH01075: Error dispatching request to : 
web_1   |  "GET /admissions-aid/paying-college HTTP/1.1" 500 -

Any further suggestions?

naveenvalecha’s picture

I'm using https://www.drupal.org/project/remove_http_headers module to strip out the headers on 9.2.3 and its working fine for me.

oheller’s picture

Thanks. It is an issue with the pantheon_advanced_page_cache module, enabled locally. I'm not sure why on this site when we have it enabled on many others. But hopefully this will aid in anyone else having this specific issue.

ronchica’s picture

Thank you @oheller. I just experienced this issue and it was being caused by pantheon_advanced_page_cache. The issue seemed to only occur on pages with lots of content/paragraphs.

niles38’s picture

None of the above patches, modules, or other suggestions worked for my site. It's 500ing on an edit node page with a lot of complex paragraphs.

sittard’s picture

@niles38 - For others experiencing this issue we noticed our development.services.yml file had the following configuration:

http.response.debug_cacheability_headers: true

Changing this to false sorted the issue.

I think this removes the extra debug information from the HTTP headers allowing it to be < 16kb.

niles38’s picture

Thanks for your reply @sittard. Unfortunately, that didn't work for me. I'm going to continue to work on this today (hopefully using x-debug). It only happens on edit node with a few types of paragraphs.

niles38’s picture

I finally figured out our issue. It turns out it was the Purge module. The patch on this ticket works for us: https://www.drupal.org/project/purge/issues/2976480 . Hope this can help someone else :)

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bappa.sarkar’s picture

Steven McCoy’s picture

StatusFileSize
new1.85 KB

Comment 47 had a typo. Missing a $ on $cache_contexts

charginghawk’s picture

Status: Needs work » Needs review
stephane888’s picture

Patch #48 works fine for me, thanks.

Using Drupal 9.3.0

abrar_arshad’s picture

StatusFileSize
new1.9 KB

Patch attached for Drupal 9.3.3

michaellenahan’s picture

I notice that drupal 9.3 introduced some caching improvements ... see https://www.drupal.org/node/3225328

Since upgrading to 9.3, I am not seeing the problem with the 5XX errors any more, so I am finding I don't need this patch any more ...

Interested to know if anyone else out there has the same experience ...

UPDATE ... sorry, it seems we are still getting the 5XX errors and need the patch still ...

We get these errors on our local dev environments and on our ci-runner (not on dev/stage/live environments) ...

[Mon Feb 07 11:21:46.745190 2022] [proxy_fcgi:error] [pid 887:tid 140424668759808] [client 192.168.66.1:55398] Premature end of script headers: index.php
[Mon Feb 07 11:21:46.745241 2022] [proxy_fcgi:error] [pid 887:tid 140424668759808] [client 192.168.66.1:55398] AH01070: Error parsing script headers
[Mon Feb 07 11:21:46.745247 2022] [proxy_fcgi:error] [pid 887:tid 140424668759808] (22)Invalid argument: [client 192.168.66.1:55398] AH01075: Error dispatching request to :
mxr576’s picture

Okay, it looks like I was trying to solve the same problem in #3268305.

@michaellenahan out of curiosity, can you test the latest MR from #3268305? Although, how #51 splits header values should be better than I did.

mxr576’s picture

Patch #51 was added to my MR. Added some code quality improvements. Needs test coverage?

mxr576’s picture

mxr576’s picture

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

anybody’s picture

Came here as I had the same issue with http.response.debug_cacheability_headers: true enabled.

@mxr576 what is merge request !1943 based on? Which is the latest patch to review to get this committed?

mxr576’s picture

@Anybody, according my first commit in the MR, it is based on #51? (I would not remember without that commit :) )

https://git.drupalcode.org/project/drupal/-/merge_requests/1943/diffs?co...

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dunebl’s picture

mr !143 apply and solve my problem on D9.5
Many thanks!!!

simobm’s picture

mr! 1943 Worked for me too on 9.3.22. Thanks!

smustgrave’s picture

Status: Needs review » Needs work

Moving to NW for the test cases before reviewing.

aiphes’s picture

Hello
I'm facing off this on D9.5.3, do I need to patch ? if yes, which files ?

Thanks

dunebl’s picture

@aiphes : you must look at the mr !1943 (see #57 for the link). From there, you can download the patch (in the upper right part of the screen see Code > download email patches)

aiphes’s picture

@Dunebl, so this is the patch https://www.drupal.org/files/issues/2021-12-09/2844620x93.patch , and in which folder do I need to apply it ?

dunebl’s picture

@aiphes this is not the last/full patch. Download it from the explanations I provide in #66 then apply it from within your Drupal root.

charginghawk’s picture

Here's a link to the MR:

https://git.drupalcode.org/project/drupal/-/merge_requests/1943

And here's a direct link to the patch:

https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff

If you manage patches using composer and the cweagans/composer-patches library, it's best practice to download the diff and use it as a local patch file, rather than reference this link. That's because attackers could conceivably update this MR to introduce vulnerabilities, and then you'd just automatically pull them in during composer install.

aiphes’s picture

@DuneBL, ok, can I rollback if something goes wrong ? because I need to patch a production website... if yes, what the command to roolback ?
@charginghawk coudl I use your patch link to apply it ?

dunebl’s picture

You can unpatch with this command:

cd "${drupal_root}"
patch -p0 -R < "${path_to_patch}"
aiphes’s picture

@DuneBL & @charginghawk
Thanks for your help, patch seem to fix my random 500 error on editing certain content types. Hope this is durable. I will do the same on my others D9 websites.
Will I need to repatch after core updating in future ?

dunebl’s picture

@aiphes: yes, you will need to repatch after each core update. (you can use composer and add the patch in composer.json)

aiphes’s picture

@DuneBL for sure, but how can I do that ?

dunebl’s picture

aiphes’s picture

ok, but I get errors of syntax:

 "extra": {
        "drupal-scaffold": {
            "locations": {
                "web-root": "web/"
            },
		"file-mapping": {
             "[web-root]/robots.txt": false,
			 "[web-root]/sites/default/default.*": false
			}
        },
		"patches": {
			"drupal/core": {
				"Patch pour Premature end of script headers - 2844620": "https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff"
			}
		}
    },
 "./composer.json" does not contain valid JSON
  Parse error on line 290:
  ...   ]        }    },    "require-dev":
  --------------------^
  Expected: 'EOF'
rgpublic’s picture

Um, sorry to interrupt the party, but I'd like to note that usually the patch is not needed for live websites. Make sure the development mode is not active (i.e. the line "include $app_root . '/' . $site_path . '/settings.local.php';" is commented/not enabled in sites/default/settings.php). Development mode should not be enabled for live websites! If you run into this problem with development mode enabled, make sure "http.response.debug_cacheability_headers" is set to false in sites/development.services.yml. Make sure you clear the cache after any changes in these files. These things should usually avoid this error - no need to patch anything.

aiphes’s picture

@rgpublic So you tell that we doesn't need to use settings.local.php on production websites ?
Because I do this every time, whithout issues except this one after migrating from D6 or D8 to D9.

rgpublic’s picture

@aiphes: Exactly. You can open and read that file if you don't believe me. You will find that contains only things that are needed for *development*. It will significantly slow down your live website. It disables caches, enables debugging headers and so on. It will also leave your site vulnerable to some attacks. You *can* run with this enabled. But it's not recommended.

aiphes’s picture

@rgpublic ok, I will plan to use the settings.php and see after the future update ( that will drop the patch) , my 500 errors still goes out. Finger cross.

aiphes’s picture

@DuneBL Trying to rollback and it doesn't work
/sited9$ patch -p0 -R < "https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff" or

/sited9/web$ patch -p0 -R < "https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff"

give
-bash: https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff: No such file or directory

dunebl’s picture

@aiphes I don't think that the patch command works with https. You should use the real patch file in your local hard drive

aiphes’s picture

@DuneBL But I don't know where are stored patches. I've nothing in the ~/patchs folder :/ I go to ask to my web host.

dunebl’s picture

There is a misunderstanding: you must download by yourself the patch file and put it in the HD of your web host. After that you can run the command to apply or un-apply this patch
I am a little bit annoyed because I think we are polluting this thread with other concerns

aiphes’s picture

I am a little bit annoyed because I think we are polluting this thread with other concerns

Totally right :) ;)

aiphes’s picture

@rgpublic after done an update - Upgrading drupal/core (9.5.3 => 9.5.7) and disable settings.local before, it seem to work without 500 errors on known pages. Your tips seem to be the real fix :)

marios anagnostopoulos’s picture

StatusFileSize
new6.7 KB

You should have used it like so https://git.drupalcode.org/project/drupal/-/merge_requests/1943/diffs.patch

Preferably download it and use it as a local patch in your composer.json since, if the code in the pr changes, the patch will change as well with your next composer install/update whatever, and that might be something you don't want to happen uncontrollably.

I uploaded a patch you can use, and it will also be useful (probably) for allowing drupal.org test suite to run.

aiphes’s picture

Thanks Marios, but I don't understand anymore. Some says patch isn't a good way, prefer using settings.php only, others say patch is needed. :/

charginghawk’s picture

Great discussion about patches, but let's please, out of respect for the 87 people following this issue, stop talking about patches here, and do not even reply to say "I agree" or "I understand" or something. We're done talking about patches! Thank you! Direct any further patch discussion here:

https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...
https://www.drupal.org/node/1399218/discuss

Regarding settings.local.php and development.services.yml (which, rgpublic is correct, should not be enabled on production environments!), please refer to the files themselves and you can discuss it on the "Disable caching" page:

https://git.drupalcode.org/project/drupal/-/blob/10.1.x/sites/example.se...
https://git.drupalcode.org/project/drupal/-/blob/10.1.x/sites/developmen...
https://www.drupal.org/docs/develop/development-tools/disable-caching
https://www.drupal.org/node/3340861/discuss

You can also always go to Forums, drupal.stackexchange.com, Slack to get answers to your questions:

https://www.drupal.org/community/contributor-guide/reference-information...

My next comment here is going to summarize the state of this ticket and hopefully get us back on track.

charginghawk’s picture

The most recent MR/patch is still mxr576's, !1943, from 8 March 2022.

https://git.drupalcode.org/project/drupal/-/merge_requests/1943

It is reviewed by the community (#62, #63) but it needs tests (#64).

dynamdilshan’s picture

Hi, I recently upgraded my site to D9.5 and I'm getting this error on some of my pages. So far I tried the following:
1) Created a patch from 1943 MR and applied as a local patch
2) Updated the `http.response.debug_cacheability_headers` to `FALSE`

None of these above works for me. The errors I see apart from the WSOD can be found below:

web_1  | [Wed Apr 12 07:08:19.863755 2023] [proxy_fcgi:error] [pid 9:tid 281473321831168] [client 172.23.0.2:34096] Premature end of script headers: index.php, referer: http://mysite.docksal.site/admin/content
web_1  | [Wed Apr 12 07:08:19.864173 2023] [proxy_fcgi:error] [pid 9:tid 281473321831168] [client 172.23.0.2:34096] AH01070: Error parsing script headers, referer: http://mysite.docksal.site/admin/content
web_1  | [Wed Apr 12 07:08:19.864194 2023] [proxy_fcgi:error] [pid 9:tid 281473321831168] (22)Invalid argument: [client 172.23.0.2:34096] AH01075: Error dispatching request to : , referer: http://mysite.docksal.site/admin/content
web_1  | 172.23.0.2 - - [12/Apr/2023:07:08:00 +0000] "GET /node/1901/edit?destination=/admin/content HTTP/1.1" 500 -

Any idea what else I can do ? Thanks in advance.

charginghawk’s picture

@dynamdilshan The current MR deals with limiting the X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts headers. With http.response.debug_cacheability_headers set to false, these should not be an issue anyway. You can use xdebug or other debugging tools to step through the code in the patch and confirm whether or not those headers are limited or even need limiting.

I will note that other headers (including from the fastly and pantheon_advanced_page_cache modules) can get out of control and the current MR doesn't account for them (though you could maybe argue it should). Please refer to the recent comments on this related Docksal issue:

https://github.com/docksal/docksal/issues/1110#issuecomment-1216488250

dynamdilshan’s picture

Thanks @charginghawk for your reply. Due to some reason this patch and the http.response.debug_cacheability_headers set to false not working for me. I put this below code on index.php to debug and get the size of my headers

if ($config['config_split.config_split.local']['status']) {
  $response->headers->set('x-drupal-cache-tags', '');
  $response->headers->set('cache-tags', '');
}

And currently nuking all the cache tags on local is the only way for me to get the page working. I found found out some of my pages giving more than 8k size headers.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tlilleberg’s picture

I was having a similar issue but it turned out not to be an issue with core. My enormous header was edge-cache-tags from the akamai module. Disabling the purge and akamai modules on local took care of it.

herved’s picture

StatusFileSize
new2.25 KB

Here's a patch version of MR 1943 (latest commit bbcb4ac7), if anyone else needs it.
PS: our setup enforces static patches and MR diffs are not allowed (as they can change unexpectedly).

samlerner’s picture

I was able to reduce the number of tags dramatically by updating the cache tag blacklist with all the options listed here: https://www.drupal.org/docs/contributed-modules/akamai/cache-tag-purging

On some pages it reduced the number of tags from ~600 to ~100. Still a lot, but definitely an improvement.

flyke’s picture

Thank you, this fixed a problem in a project that was recently updated to Drupal 10 where I was suddenly unable to edit the homepage.
I could revert the revision of the homepage and then edit it. But after a while editing would again not work and again the /node/xxxx/edit page would return an error 500. This patch (the diff from the issue fork) fixes it in my D10 project.

umac_de’s picture

Same here after update of project to 10.1.7
Updated the `http.response.debug_cacheability_headers` to `FALSE`
+ patch #87 fixed it

tunic’s picture

Issue summary: View changes

I tried to clarify current state in the issue body: we need test to move this to RTBC.

The MR should be used, ignore patches.

The patch is working for me in a real project.

aiphes’s picture

#87 works on D9.5.11 core .

damienmckenna’s picture

Ran into this recently loading the fields settings page for a content type after http.response.debug_cacheability_headers was set to "true" via the development.services.yml file; the site runs on ddev, Safari showed the site giving a 500 error and nothing I could do would give me an explanation of what was going on.

b_sharpe made their first commit to this issue’s fork.

b_sharpe’s picture

Status: Needs work » Needs review

Considering this is still an issue in D11, I figured we should move it forward. Added tests and new MR for 11.x, back to review

b_sharpe’s picture

Issue tags: -Needs tests

smustgrave changed the visibility of the branch 11.x to hidden.

smustgrave’s picture

Added some nitpicky change for the tests.

Leaving in review for additional eyes.

tunic’s picture

Issue summary: View changes

Changes loosk good to me. However, it would be great if we can run the tests only without the patch to confirm the test works ok (fails when no fix has been applied, passes when it is applied). I tried to run the "Test-only changes" stage at https://git.drupalcode.org/issue/drupal-2844620/-/jobs/1342592 but I says:

This job does not run automatically and must be started manually, but you do not have access to it.

b_sharpe’s picture

@tunic this was previously run, but I guess retriggered on rebase. For access I think you just need to request access to the fork and then sign in to gitlab.

Either way, here's the pass on main, and fail on test-only run: https://git.drupalcode.org/issue/drupal-2844620/-/pipelines/148275

tunic’s picture

Ah, of course, I forgot to ask for access rights.

Issue is addressed, code issues have been solved, test-only patch fails as it should and tests passes with patch. I think this can be moved to RTBC but I'll wait for more reviewers.

tunic’s picture

Issue summary: View changes

Updating solution.

BTW, do we need a change record? I would say no because it is not a big change, but the fact that headers have a number added if they are too big bugs me (X-Drupal-Cache-Contexts -> X-Drupal-Cache-Contexts-1, X-Drupal-Cache-Contexts-2).

Change record or not, the following documentation pages would need a quick update to reflect headers can be split:

b_sharpe’s picture

Given this is a debug-specific change, I don't think a change record here is necessary; however, updating any docs referring to debugging headers is definitely a good idea.

tunic’s picture

#114, I think those are related but different headers. In this issue we are handling the headers X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts that are sent by Drupal core when the cacheability debug functionality is on (http.response.debug_cacheability_headers: true). However, the links you provide refer to the Cache-Tags and Purge-Cache-Tags headers (each purger module uses one or the other).

While the information they carry is similar (the cache tag information) and all are affected by the same problem (header too large), the purger headers are not affected by the fix proposed here because that later headers are added by each purger module itself.

It can be interesting to propose a similar fix on the purger modules; however, it will probably be harder for Varnish to process a single header or multiple headers with an incremental sequence (Cache-Tags-1, Cache-Tags-2, etc) so I guess this is something that must be discussed.

o&#039;briat’s picture

ok, sorry for the confusion (TL; DR 😬).
I think purge should just split tags by 16k chunks and make multiple calls to Varnish/CDN.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

b_sharpe’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Ran the test-only feature https://git.drupalcode.org/issue/drupal-2844620/-/jobs/1943211

And see there are 2 tests but testDebugHeaders isn't failing without the change. So does it need to be tweaked or removed?

b_sharpe’s picture

Status: Needs work » Needs review

@smustgrave Sorry I should've been more clear in my description. I added two tests here:

  1. testDebugHeaders()
    • This test was to test the general functionality of debug headers since a test for this didn't exist already, it will pass under current conditions
  2. testLargeTagsAndContexts()
    • This is a test for the added functionality of this issue, and should fail without the patch

I was going to split this to two issues at first for the initial test, but figured given the longevity of this issue it made sense to keep it here. Back to NR.

smustgrave’s picture

@b_sharpe thanks for the explanation

Left 1 nitipicky comment but overall changes look good. +1 from me.

Going to leave in review for additional eyes.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Self applied 1 nitpicky item.

catch’s picture

Status: Reviewed & tested by the community » Needs work

The title no longer matches the issue summary and MR, can it be updated? Also one comment on the MR.

scott_euser changed the visibility of the branch 2844620-finishresponsesubscriber-need-warningerror to hidden.

scott_euser’s picture

Title: FinishResponseSubscriber: Need warning/error when headers exceed 16k » Automatically split headers when they headers into multiple lines when they exceed 8k
Issue summary: View changes
Status: Needs work » Needs review
  1. Resolved feedback
  2. Updated issue summary
  3. Updated title
scott_euser’s picture

Issue summary: View changes

Updated to standard issue summary template

prudloff’s picture

Title: Automatically split headers when they headers into multiple lines when they exceed 8k » Automatically split headers into multiple lines when they exceed 8k
scott_euser’s picture

Ha thanks! Clearly my morning coffee hadn't kicked in yet!

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Appears feedback has been addressed.

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

I read the IS, comments and the MR. I don't see any unanswered questions. Thank you to @charginghawk for working on summarizing and keeping this issue on track.

There is a large number of documentation links in the comment for places this needs to be updated. Does this need to be documented in so many places? I have not looked at all the pages but this should be documented in one place and the others refer to that one place.

Here are the links I gathered from the comments

I left some suggestions and comments in the MR which need to be looked at so setting to NW.

I also updated credit.

erickbj’s picture

I just stumbled upon an issue with Cloudflare Purger and created https://www.drupal.org/project/cloudflare/issues/3482279 for that. I then found this issue and I think both are related to some extent. I was hoping the solution of this issue would apply for the Cloudflare Purger Cache-Tags case, but I don't think it would work if we're adding numerical suffixes such as "-1", "-2" etc when we have multiple "split" response headers.

For Cloudflare specifically their documentation seems to indicate the solution is to have multiple response headers with the same name but different values.

That seems to be applicable to that header as it might for X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts as those adhere to the comma-separated specification for response header values (although the "X-" ones are purely for debugging purposes to my knowledge, while the Cloudflare Purger Cache-Tags is functional). See:

  1. https://stackoverflow.com/q/3241326
  2. https://www.rfc-editor.org/rfc/rfc9110.html#name-field-order (above answer points to a deprecated link, this should be the updated info)

So the question is, if this solution is applicable for any headers, should we allow some to be added multiple times without adding numerical suffixes?

boazpoolman’s picture

StatusFileSize
new16.07 KB

Just created a patch from the MR so I can apply it in my composer.json.

dubois changed the visibility of the branch 2844620-finishresponsesubscriber-need-warningerror to active.

mr.baileys’s picture

As @erikbj also mentions in #131, the official proper way to handle headers with too many values is indeed to split them up into separate lines but with the same header name (see https://www.rfc-editor.org/rfc/rfc9110.html#section-5.2). This would also ensure it remains easy to implement downstream logic basic on the header ("if header contains xyz" is a lot easier than "if header-1 contains xyz or header-2 contains xyz or header-3 contains xyz ...")

So I think splitting the headers but keeping the name is preferable over the current solution appending '-{delta}'. One caveat however: the different header line values are combined with ",", which means the current implementation would have to be converted to generate a comma-separated list instead of a space-separated list (which in turn is not backwards-compatible). On the other hand, this is meant to be a debugging tool, so I don't think this is that big a deal.

godotislate’s picture

Splitting the header into separate lines with the same header name is going to be difficult, given that the Symfony Response object's headers object doesn't allow for multiple headers with the same key.

Also, if the goal here is only to fix headers for the debug cache tags, having the -1, -2 suffixes probably works just fine. If this issue also needs to address headers generally that are too long (such as the ones used by Purge), the current MR does not address that, and the question of whether and how to split headers into multiple lines with the same name is relevant.

znak’s picture

Hi, everyone

I've tried to apply patch #132 to 11.x version but it does not apply. I updated it and created a new patch for version 10.2.x

Please, check if it's working.

znak’s picture

Status: Needs work » Needs review
jwineichen’s picture

#132 is working for me on 10.4.3. Was getting a WSOD on pages with larger views with images results, facets, and having admin toolbar extras installed. Pages load successfully now, without having to limit views result size.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new161 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

jason.ullstam’s picture

Patch from #132 worked for me on a site that was getting 500s. Core 10.4.6.

carolpettirossi’s picture

Status: Needs work » Needs review

MR !7432 applies successfully to Drupal 11 and solves the WSOD on pages with many content/nodes/taxonomies displayed.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

prudloff changed the visibility of the branch 2844620-finishresponsesubscriber-need-warningerror to hidden.

prudloff’s picture

akz’s picture

My two cents on this:

  • Drupal core does not enforce a hard size limit on the cacheability headers. The 16K mentioned here is a recommendation and not an implemented imposed limit.
  • Servers have different header size limits each (some examples are listed here). Some provide configuration for those, while others do not (at least not yet), as is the current status quo for Apache2 with mod_proxy_fcgi.
  • The http.response.debug_cacheability_headers (which exposes the large headers that cause fails in some servers) is a feature that should be enabled and used for development purposes only. It's intended for developers to debug the cache-related headers. It should not be enabled in production environments. If you have implemented a setup that uses these in a prod env and those headers are consumed by CDNs/reverse proxies for tag-based cache invalidation, you are probably in a wrong path and you should reconsider your implementation.

Having said that I strongly believe that it's the servers' responsibility to provide header size configuration options for the developers to use and adjust their envs appropriately. This is not Drupal core's responsibility and definitely not a Drupal core bug. For the cases where server configuration is not an option (yet), yes, a solution should be provided, but not as a core fix, rather than as a contrib module that should be enabled in dev envs only.

Towards this direction I have composed the contrib module Debug Cacheability Headers Split that uses the idea and code provided in this issue. I think this issue should be closed with status "Closed (won't fix)" and the MR should be cancelled.

rgpublic’s picture

I don't think I'd really agree here, I'm afraid. I came to this issue back then when I was investigating why a Drupal site suddenly showed a WSOD. There was no error message. Nothing. This proved to be extremely(!) difficult to debug. Almost impossible for novice users. That's why I think solving this issue is important. It doesn't necessarily mean automatically breaking the headers into multiple lines. But at the very least a proper error message for the common case (>16 KB) should appear with information on how to solve this (install the module or switch of dev settings).

If you installed such a module, you'd already be aware of such an issue. But as described the problem is more severe if you run into this unintentionally. Of course, people should rather switch off the development settings for production servers - I've already mentioned that in a previous comment. Nevertheless, for some reason, many people have this accidentally switched on. They won't notice any problem but when a page suddenly exceeds the limit (it could even be the start page), the website all of a sudden shows a WSOD with not good option to debug this. This limit can be reached just by doing editorial changes (inserting a view etc) so this can happen to a site pretty unexpectedly.

Of course the limit can vary so I think a relatively low limit (4 KB?) should already trigger a warning. Automatically breaking the lines in addition to the warning would be even better IMHO, because it would keep the site from breaking suddenly.

akz’s picture

@rgpublic I am totally in favor of Drupal core raising a warning when a certain size limit is exceeded. I just do not agree that this is a bug, let alone a core bug. A softer approach perhaps would be to re-categorize this issue as a "Feature request" and let the more experienced Drupal core devs to decide if/how such a warning would be useful in order to be included into core code.

rgpublic’s picture

@akz: Personally, I'm absolutely fine with reclassifying this as a feature request. It's not really a "bug", right. I just think it's important that we are a bit more helpful to developers and not leave them in this unpleasant situation (white screen) without any hint on what's going on. They can then install the module or do whatever to resolve this, but at the very least they should be informed about this situation.

jurgenhaas’s picture

StatusFileSize
new6.71 KB

Updated the MR to apply on 10.5.2

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new2.01 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

luke.leber’s picture

I'm not exactly following why the -1, -2, etc header name enumeration exists.

The HTTP spec allows for multiple headers with the same name. IMO changing the name of headers like that might be a new drupalism?

godotislate’s picture

Title: Automatically split headers into multiple lines when they exceed 8k » Automatically split cache debug headers into multiple lines when they exceed 8k

I'm not exactly following why the -1, -2, etc header name enumeration exists.

The HTTP spec allows for multiple headers with the same name. IMO changing the name of headers like that might be a new drupalism?

I'm of two minds about this: by spec, multiple headers with the same name should be implemented as comma-separated values. But these headers are for humans to read in order to debug cache metadata, not for machine ingest, and changing the tags to be separated by commas instead of spaces would make them more difficult to read.

Anyway, the point is that human-readability is more important here than adhering strictly to spec, and I'd say whichever way, numbered headers or no, is easier for more people to understand at a glance is the way to go.

Update issue title, since this is strictly about the core cache debug headers.

nicxvan’s picture

What if we add a special cache tag to the beginning for human debugging.

Something like header_split:1
Then we can follow spec and aid human debugging.

These tags can be used by reverse proxies for clearing right?

godotislate’s picture

These tags can be used by reverse proxies for clearing right?

Not really? Reverse proxy services typically have a documented API for clearing, whether that's a REST endpoint or their own HTTP header and expected values, etc.

If you control your own reverse proxy, you could possibly customize it to use these headers for clearing tags, but I think that's an atypical use case. They're called http.response.debug_cacheability_headers because they're specifically for debugging.

tostinni made their first commit to this issue’s fork.

tostinni’s picture

StatusFileSize
new7.29 KB

Rebasing the MR and adding a patch for Drupal core 11.2.5

artematem’s picture

StatusFileSize
new7.18 KB
artematem’s picture

artematem’s picture

StatusFileSize
new7.29 KB
artematem’s picture

artematem’s picture

StatusFileSize
new7.32 KB

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

godotislate’s picture

Issue summary: View changes
Status: Needs work » Needs review

I hit this several times on my work project, which wouldn't be that big an issue, except that for some reason there are a few different development.services.yml files in the project, and every time I have to flip the values of http.response.debug_cacheability_headers in multiple files before I figure out which one is authoritative.

So I did some investigation and some testing changing this line on main in FinishReponseSubscriber:

      $response->headers->set('X-Drupal-Cache-Tags', implode(' ', $cache_tags));

to

      $line_length = 8000;
      $line_count = 1;
      $tags_header = str_repeat('1', $line_length);
      $response->headers->set('X-Drupal-Cache-Tags', array_fill(0, $line_count, $tags_header));

Using the standard DDEV configuration of Apache:

  • Values of $line_length up to roughly 8000 resulted in responses with status code 200. $line_length clearly above 8190 resulted in responses with status code 5xx
  • $line_count could get quite high, I think I got up to 10 (or 20, don't quite remember), without issue. So at least the total header size in Apache is set high
  • LimitRequestFieldSize has no effect on the response header line limit, because it is set at 16380 by default in DDEV. This is unsurprising, but some sources online suggested/guessed that the setting would apply to both requests and responses
  • When there are multiple header lines with the same header name, the default configuration in Apache is to merge those lines into one on output, with the lines glued together with ', '

Using the standard DDEV configuration of nginx:

  • With $line_count at 1, values of $line_length up to roughly 32000 resulted in responses with status code 200, and 5xx above that
  • With $line_count at 4, values of $line_length below around 8000 resulted in responses with status code 200, and 5xx above that
  • The two results above make sense that nginx is configured to have a max total header size of 32000 and there is no length limit per header line
  • Default configuration outputs multiple header lines with the same header name as separate header lines

The findings here are consistent with a lot of previous comments that the concern should be about splitting the header lines below the Apache 8K line limit, and not worrying about the total header size on Apache or nginx, since running into that would probably be a result of significant content architectural issues.

Anyway, I have rebased the MR and made the following changes:

  • I've dropped the -N suffixes on the header names, so there will be multiple lines with the same header name. As mentioned, in Apache, this most likely will result in only one X-Drupal-Cache-xxx line output anyway, just with a couple of ', ' separators sprinkled in
  • While spec suggests that the individual contexts/tags should be comma-separated, I've left the tags as space-separated here, because these are debug tags meant to be read by humans, and space separation is IMO much more readable than comma-separation. (And command + space separation just makes the headers longer). That being said, I made the separator a constant, so if anyone really needed to (not sure what the use case would be), they could alter the FinishResponseSubscriber class to use a subclass and change the constant value there.
  • The anonymous function that splits the lines is now a protected class method instead
  • Unit test has been updated to match the changes in FinishResponseSubscriber
  • Functional test added to show how the response on Apache is 500 without splitting the lines. This fails locally with splitting the lines, but apparently it passes on CI. I'm not sure how?
benstallings’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

@benstallings, what did you do to review this issue and the MR? And there are over 10 unresolved comments on the merge request. Have they been addressed?

There are several steps, or gates, that an issue must pass before it is marked RTBC. For most issues following step 10 in the Review a patch or merge request task of the Contributor guide is sufficient. The complete list of core gates has more topics. Also, check the tags on the issue and make sure they are complete.

Setting to Needs review. I updated the remaining tasks with what I see.

quietone’s picture

And I updated credit

godotislate’s picture

Issue summary: View changes

I thought I had resolved the MR comments, but either I forgot to hit submit back then or I just plain forgot. Updated the MR and addressed comments.

Issue summary update. Does this need an release note snippet

I removed the release note snippet, because I don't think it needs one, but I can add an updated one if needed.

What Documentation needs to be updated? Is it the list in #130?
I'm not sure documentation needs to be updated for what is ending up to be a subtle change, if it does, I'd say one or both of the following would suffice https://www.drupal.org/docs/drupal-apis/render-api/cacheability-of-rende...
and https://www.drupal.org/docs/8/api/responses/cacheableresponseinterface#d....

Something like "Note that on some servers, such as those running nginx, the headers could be split across multiple lines with the same name if there are a lot of tags or contexts. On those running Apache, the tags or contexts will be on a single line, but may be separated by commas at intervals of around 8000 characters in length."

godotislate’s picture

Status: Needs review » Needs work

Investigated the behavior of Apache appending (or merging?, not sure which since the header line values are different) headers with the same name: I'm not sure what I read previously that gave me the idea that this is default behavior. Looking again, I haven't found anything to substantiate or refute that it is the default setting, with or without mod_headers. Anyway, Apache in my local ddev is appending or merging the debug headers if there are more than one with the same name, but it is not happening on Drupal CI, and I'm not sure what the configuration difference is. I'll revisit whether some comments should be rewritten.

o&#039;briat’s picture

Is HTTP headers are really the place for these debug info?
Could these info just simply be printed as comments at the top of the body (as other cache and twig debug info)?
Do I miss a real use case where they have to be placed into HTTP headers?

I do understand that other headers containing cache-tags are used for proxy cache invalidation (with the same issue) but that's should be fixed in these contrib modules (ex: https://www.drupal.org/project/purge/issues/2952277)

rgpublic’s picture

@o'briat: Well, I think it's actually a pretty nice solution. Printing stuff right in the web page has frequently numerous problems. Depending on what styles exist, stuff is cut off, in a font size too small, in the wrong font altogether, in the wrong color and more. What's more it isnt really a solution for non-HTML requests like Ajax Requests etc. Printing stuff there will obviously destroy e.g. a proper JSON response.

This functionality, btw - you can't stress this enough - is opt-in and should only be used in development mode. I think just automatically splitting them across multiple headers if they exceed a certain length is still a solution that makes sense, although an alternative would be to just cut them off and add "overflow" at the end or to the error log, because such a long list is hardly usable anymore, anyway. The main point here is that a WSOD is prevented which is very confusing for developers to understand.

@godotislate: Your comment gave me the idea to checked whether this is actually covered by the HTTP standard:

https://greenbytes.de/tech/webdav/rfc2616.html#message.headers

It seems this is actually allowed (and as far as I could figure out also used e.g. with Set-Cookie or CSP headers), BUT: The list must be a) separated by commas and b) must have the same meaning regardless of whether it is one single entry or split over multiple entries. And c) the order of those entries is relevant. We fulfill b) and c) but I just tested this and our list is separated by spaces currently. Of course there is no standard for our custom header, so we can change this anyway we want. In order to fulfill the demands of the HTTP standard it seems we need to switch the list to comma separated it seems.

Some software along the response path (proxies etc) might merge or mangle this stuff anyway. But it's their responsibility then IMHO to make sure they don't merge stuff to a line longer than allowed etc. As this is an opt-in debugging feature, I don't think we need to take care of that. In the worst case, the multiple lines would still cause a WSOD, we can't do anything about it. But it will solve the WSOD for most developers who would otherweise get bitten by this, so I think it's still worthwhile solving this.

Okay, these are just my thought's on this FWIW.

godotislate’s picture

Could these info just simply be printed as comments at the top of the body

Among other things, it's difficult to do this because Drupal calculates and sets a content-length header by the time the subscriber runs, so adding markup at this point could cut off the output of the response sent by Apache or nginx.

In order to fulfill the demands of the HTTP standard it seems we need to switch the list to comma separated it seems.

I mentioned this in #165, but since these are debug headers meant to be read by humans on a non-production environtment, and space separation is more legible than comma separation, I think this is fine. (And comma + space separation just uses more bytes). Arguably we are still meeting HTTP standards because each line of multiple contexts or tags could be considered a singular value.

o&#039;briat’s picture

I meant to display then in HTML comments, but you go a point with json

damienmckenna’s picture

Just to mention it, but on a site I'm building I have a busy node edit form that was WSOD'ing even though $response->contents had HTML, it turned out to be this bug and the MR fixed it for me.

godotislate’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

OK, did some clean up with comments, and moved the Functional test to a Kernel test since we have #3390193: Add a drupalGet() method to KernelTestBase now. This made the header lines repeatedly separate in my local testing as well.

Updated the IS. I don't think there needs to be a release note or change record, and I'm not quite sure what it would say. Happy to provide one if there are suggestions. Removed IS update tag.

geek-merlin’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the code.
Code looks straightforward and good, tests are in place and look reasonably.
Bot is green.
+1 for no release not for such a simple bugfix.
and YAY! for leveraging the new kernel tests!

alexpott’s picture

Version: main » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Wow - it is lovely to see this get fixed - such a gotcha.

Committed and pushed d38a4f4ea13 to main and 47289f37d4d to 11.x and f0cd52cd632 to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed f0cd52cd on 11.3.x
    fix: #2844620 Automatically split cache debug headers into multiple...

  • alexpott committed 47289f37 on 11.x
    fix: #2844620 Automatically split cache debug headers into multiple...

  • alexpott committed d38a4f4e on main
    fix: #2844620 Automatically split cache debug headers into multiple...
alexpott’s picture

Version: 11.3.x-dev » 11.x-dev

Can't go in 11.3.x not all the kernel test drupalGet stuff has landed there.

alexpott’s picture

Also had to do a quick follow-up because

    $cacheData = new CacheableMetadata()
      ->setCacheTags($tags);

Only works on PHP 8.4 and up... breaks on PHP 8.3. It needs to be

    $cacheData = (new CacheableMetadata())
      ->setCacheTags($tags);

  • alexpott committed c4c50b2d on 11.3.x
    Revert "fix: #2844620 Automatically split cache debug headers into...

  • alexpott committed 5253354e on 11.x
    fix: #2844620 (follow-up) Automatically split cache debug headers into...

  • alexpott committed 024fc727 on main
    fix: #2844620 (follow-up) Automatically split cache debug headers into...

godotislate’s picture

Among other things, it's difficult to do this because Drupal calculates and sets a content-length header by the time the subscriber runs

I was wrong about this. The content length is calculated afterward, so debug information could be added to the response content somehow.
Opened #3589816: Look into alternate way to display debug cache information to look into it.