(followup) drupal_serve_page_from_cache can serve uncompressed data with Content-Encoding gzip header with page_cache_without_database = 1

D8 has a different way of loading settings, so I'm not sure if the bug exists too.

Regarding D7, this is a scratch of a patch. I don't know if this approach is right, as it will ignore page_compression if page_cache_without_database is set.

Are there any tests for this kind of funcionality? Shouldn't be hard, I guess.

Forgot the patch

Thinking of a better solution here, why don't we set $cache->data['page_compression'] and when delivering the page check that instead of variable page_compression?

Alternatively, we don't change any code, and just warn the user on performance page about the usage of non_database, in which case page_compression must be set in settings.php.

Is this fixed in drupal core at the moment?
I just had the same problem with APC installed.
I have drupal 7.16
I used the 2a configuration in the README file of the APC module and when adding this two lines to the settings.php:

$conf['page_cache_without_database'] = TRUE;
$conf['page_cache_invoke_hooks'] = FALSE;

after a while it gave me the following fatal error:
Fatal error: Call to undefined function db_query() includes/cache.inc on line 357
I deleted these two lines from the settings.php and everything went back to normal.

The patch in #2 is works for me.

But it works only when "Compress cached pages" setting enabled.

I use Memcache to store cached data(settings.php):

/**
 * Add Memcache backend.
 */
$conf['cache_backends'][] = 'sites/all/modules/memcache/memcache.inc';
$conf['cache_default_class'] = 'MemCacheDrupal';
$conf['cache_class_cache_form'] = 'DrupalDatabaseCache';

/**
 * Return cached pages for anonymous without database bootstrap.
 */
 $conf['page_cache_invoke_hooks'] = FALSE;
 $conf['page_cache_without_database'] = TRUE;

The patch in #2 works for me as well.

I set APC to serve cached pages from memory without a DB hit. Prior to the patch I was getting the invalid compression error message from the browser.

edit: I'm using D 7.21

I digged deeper into code and found a real source of this problem.
Look at drupal_serve_page_from_cache() function. Pay your attention at this code:

$page_compression = variable_get('page_compression', TRUE) && extension_loaded('zlib');

It looks pretty good: if page_compression is disabled in UI the page won't be gziped and then unziped. BUT if we got $conf['page_cache_without_database'] = TRUE; in our settings.php the variables is NOT initialized and variable_get('page_compression', TRUE) will ALWAYS return TRUE!

So, even if page_compression is disabled, Drupal will try to unzip it, and we will get encoding issues:

if ($page_compression) {
    header('Vary: Accept-Encoding', FALSE);
    // If page_compression is enabled, the cache contains gzipped data.
    if ($return_compressed) {
      // $cache->data['body'] is already gzip'ed, so make sure
      // zlib.output_compression does not compress it once more.
      ini_set('zlib.output_compression', '0');
      header('Content-Encoding: gzip');
    }
    else {
      // The client does not support compression, so unzip the data in the
      // cache. Strip the gzip header and run uncompress.
      $cache->data['body'] = gzinflate(substr(substr($cache->data['body'], 10), 0, -8));
    }

The easiest way to avoid this is to set this variable in settings.php:

$conf['page_compression'] = FALSE;

But we definately should log this case in the documentation. I'm not sure where is the right place to do that.

Thanks @Heine. I'll start working on solution proposed in #3.

As I promised, patch attached. Please, review it.

Nice idea, still needs test for rtbc

@andypost,

Manual or Unit tests?

Automated. Submit tests to demonstrate a test fail (thus, the bug), and a combined patch with test+code to demonstrate the fix.

I'm not sure how can I make an automated test for thia patch. It requires some external cache system (memcached, apc, or smth like that). Does Drupal test bot has required tools for that?

Bumping to Drupal 8, as it has to be fixed there first. Saving metadata with the page itself is a good idea.

I can provice patch for D8, but the question is still the same - how to use external cache in tests?

Just unit testing drupal_serve_page_from_cache() would be enough. Don't spend too much time on it, the days of the page cache in its current form are numbered.

Ok, will try to provide a patch asap.

Test for D8 without patch (should fail).

Patch + test.

Hm possibly problem in line endings. Fix and attach test again.

And fixed patch with test.

Oh, my bad again. Attached a new test with patch. The first one should fail while the second should pass.

I think adding a line of comment to the new 'page_compressed' => $page_compressed part would be good -- like "Every page cache item needs to store whether it's compressed or not because by the time it is read, the configuration might change". Then it's perfect, thanks for the test, the separate upload, the patch, it's really nice work.

@chx, agree. Attach updated patch.

Thanks again.

Looks good.

Committed/pushed to 8.x, moving to 7.x for backport.

Attach test and patch with test for D7.

With this patch we will solve two problems at one time:

• If page was cached with (or without) compression mode and then configuration was changed, page wasn't served correctly (we already fixed this in D8).
• When using $conf['page_cache_without_database'] = TRUE; Drupal tried to decompress cached data, no matter whether compression mode was enabled or not.

A few comments about the patch that went in for Drupal 8:

(1)

-  $page_compression = $config->get('response.gzip') && extension_loaded('zlib');
+  $page_compression = !empty($cache->data['page_compressed']) && extension_loaded('zlib');

This runtime check doesn't make sense. The fact that the zlib extension is loaded or not doesn't change the fact that the page was stored compressed or not. We need to drop the second part.

(2) The test is testing a side effect that I'm not even convinced is desirable (old pages are still served when the compression mode is changed). Could we look into unit testing the page cache instead?

(3) We need an update function that clears the old cached pages.

@Damien Tournoud,

(1) good catch, we can drop this part.

(2) It doesn't seems to be nescessary, this test shows how cached pages works for anonymous after changing compression mode.

(3) What if page compression was changed using $conf in settings.php? In this case cached won't flush.

(2) It doesn't seems to be nescessary, this test shows how cached pages works for anonymous after changing compression mode.

The problem is what's tested is just a side-effect of this patch. Even more, what's tested is arguably not a general property of "the page cache". We are essentially testing an implementation detail here, not a real feature.

I could totally see a page cache implementation that flushes all the entries when you change the compression mode. That would be totally legitimate (after all, the compression mode is a configuration option...), and will not affect the functionality that we name "page cache" in any way.

(3) What if page compression was changed using $conf in settings.php? In this case cached won't flush.

We just need an update function that unconditionally flushes the page cache, because the way we store pages changed, in a non-backward compatible way.

Still don't undestand why storing compression mode in cache is non-backward compatiple. I think that this feature helps to avoid problems with changing of the compression settings. Why should we flush all cached pages and cause a performance issues, if we can just use flag from cache?

@Spleshka: if you had compression enabled and update Drupal, we will serve compressed pages from the cache without the proper compression headers, because $cache->data['page_compressed'] is empty. And yes, we should just flush the page cache, it is cheap to rebuilt.

Oh, now I got your point. But we can just flush cache pages after update to next release. And no, flushing cached pages on high-load sites may cause some temporary problems, so why should we do this?

We just need an update function that flushes the page cache. We do that on very high traffic sites all the time. Don't be afraid of it.

The empty hook_update_N() implementation should be added only to 7.x branch. 8.x is not needed for it's current state

@Damien Tournoud,

You mean just to flush cached pages once after update to the next release? If so, it is OK.

@andypost,

Yep. D8 just needs a little patch that removes code with extension_loaded('zlib');

New patch for D7 attached.

After discussion with @Damien in irc, we concluded that comment for hook_update_N() should be better and more informative.

Great!
PS Let's get this in and then fix #33 in D8

I had a slight issue with applying the patch.

Patch in #43 doesn't apply cleanly on D7. I am running 7.23.

patch -p1 < drupal-drupal_serve_page_from_cache-can-serve-uncompressed-1476810-43.patch
patching file includes/bootstrap.inc
Hunk #1 succeeded at 1288 (offset 10 lines).
patching file includes/common.inc
Hunk #1 succeeded at 5167 (offset 8 lines).
Hunk #2 succeeded at 5178 (offset 8 lines).
Hunk #3 succeeded at 5198 (offset 8 lines).
patching file modules/simpletest/tests/bootstrap.test
Hunk #1 FAILED at 219.
1 out of 1 hunk FAILED -- saving rejects to file modules/simpletest/tests/bootstrap.test.rej
patching file modules/system/system.install
Hunk #1 succeeded at 3116 (offset 14 lines).

more modules/simpletest/tests/bootstrap.test.rej
--- modules/simpletest/tests/bootstrap.test
+++ modules/simpletest/tests/bootstrap.test
@@ -219,6 +219,18 @@
$this->assertFalse($this->drupalGetHeader('Content-Encoding'), 'A Content-Encoding header was not sent.');
$this->assertTitle(t('Welcome to @site-name | @site-name', array('@site-name' => variable_get('site_name', 'Drupal'))), 'Site title matches.');
$this->assertRaw('', 'Page was not compressed.');
+
+ // Disable compression mode.
+ variable_set('page_compression', FALSE);
+
+ // Verify if cached page is still available for a client with compression support.
+ $this->drupalGet('', array(), array('Accept-Encoding: gzip,deflate'));
+ $this->drupalSetContent(gzinflate(substr($this->drupalGetContent(), 10, -8)));
+ $this->assertRaw('', 'Page was delivered after compression mode is changed (compression support enabled).');
+
+ // Verify if cached page is still available for a client without compression support.
+ $this->drupalGet('');
+ $this->assertRaw('', 'Page was delivered after compression mode is changed (compression support disabled).');
}
}

@SocialNicheGuru,

Patch should be applied to 7.x-1.x, not for a stable release.

Any progress with getting this in core?

Erm... I think that this issue is quite important for high-load web sites, and it should be commited.

Guys, please tell me if here remained some work to do before we can commit this patch. But it seems already tested for me.

Let's get David opinion

The patch no longer works since a different version of system_update_7079() was recently added to the codebase. However, since it will go out in the same release, it will do the same job that the empty update function was trying to do (namely, force a cache clear).

So here is a reroll with the update function removed. Will commit before the upcoming release, as long as it still passes tests.

And then we have to bump this back to Drupal 8 to fix issues with that patch (#33.1)?... Normally would have done that first (or else in a separate issue), but it looks like the fix is pretty small.

Patch is green and yes, once next core version already adds update function there's no reason in new one.

After 7.x commit it makes sense to fix 8.x with a small follow-up patch to address #33.1

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/6cd1aae

Back to Drupal 8 for that small followup mentioned above.

What core should send to client when data compressed but no way to decompress?
I think we should throw exception and because fallback to serving page without cache will just hide the problem.

Patch for 33.1 follow-up

I'm closing this issue because it seems to be outdated. Feel free to reopen and update the issue summary if the problem still persists.