getgrav / grav

Modern, Crazy Fast, Ridiculously Easy and Amazingly Powerful Flat-File CMS powered by PHP, Markdown, Twig, and Symfony
https://getgrav.org
MIT License
14.53k stars 1.41k forks source link

"Content-Encoding: none" breaks mod_deflate #548

Closed philfry closed 8 years ago

philfry commented 8 years ago

Hi,

I'm using mod_deflate instead of grav's builtin gzip-compression because it's more flexible for my needs.

As grav sets the header Content-Encoding: none in system/src/Grav/Common/Grav.php when having system.cache.gzip set to false, grav is preventing httpd from compressing the output. Imho there's no need for setting this header when compression is not wanted.

Kind regards,

Phil

rhukster commented 8 years ago

I agree, will fix.

rhukster commented 8 years ago

@mahagr looks like your shutdown fix added a Content-Encoding: none, is this required for your fix to function on your end? https://github.com/getgrav/grav/commit/d2660e0755209523d772065ad0a2d9a2c6e4c987

mahagr commented 8 years ago

Actually that Content-Encoding: none is indeed needed to make the functionality to work. Otherwise connection will not be closed with mod_deflate enabled because of the Content-Length is set (but is too long after the compression).

Basically the whole fix was Content-Encoding: none; the another change to force ob_start() was just something I found to be buggy if the browser didn't support compressing.

Please do not remove the line; it just breaks the code.

mahagr commented 8 years ago

PHP does not have a way to close STDOUT. If you have something to process after the page has been sent, browser (and mod_deflate) will keep expecting more data and user needs to wait until PHP stops running before the page has been fully loaded. The only way to work around this is to send Content-Length and make client to close the connection when it has received all the data (except if you are using FastCGI, which has command to close the connection).

Assuming that you aren't using FastCGI, mod_deflate will never get the information that all the data has been sent and it will continue to expect more data regardless what you try to do. So the only way is not to use mod_deflate in this case.

All of this said:

In the second case request should probably not send Content-Length and connection close.

mahagr commented 8 years ago

New logic should allow mod_deflate to do the hard work, but only if you are using FastCGI. You need to disable Grav's builtin gzip handler to use mod_deflate.

Without FastCGI there is not much that can be done because of the limitations of PHP. In this case you are better off using Grav's implementation of gzip.

philfry commented 8 years ago

Hi, thanks for your feedback.

is Content-Encoding: none even valid (rfc2616)? I've never seen it in any other applications before. What about using flush(), ob_end_flush(), session_write_close() and, for fcgi, fastcgi_finish_request()? That should work fine even without the Content-Encoding header set to none.

fwiw, I'm using php-fpm. It might be possible workaround it using mod_headers, but that cannot be the final solution :)

mahagr commented 8 years ago

We replied at the same time. Please see my comment from above.

mahagr commented 8 years ago

To reply to your questions, I fixed the code to close the connection properly for you (php-fpm).

Content-Encoding: none is used in all the examples on how to prevent mod_deflate from compressing the PHP output. I don't think there is a better workaround for this -- I guess it is better to send slightly broken response than to let user to wait seconds or even minutes to get any response..

If you have ideas on how to do this in a better way, I'm all ears. At least the issue doesn't exist anymore with FastCGI. :)

philfry commented 8 years ago

great, thanks a lot :+1:

Monarobase commented 8 years ago

Hello,

This trick, seems to break on LiteSpeed when gzip is disabled in grav. Litespeed manages it's own gzip compression using a gzip cache which is much more efficient than what PHP can do) and add's gzip to the headers if it doesn't exist resulting in :

Content-Encoding:none,gzip

The result of this is that the browser show's the Gzip compressed content instead of compressing it.

If we comment out Content-Encoding:none the page loads without any delay on litespeed, I've also asked them if they have an equivilent to fastcgi_finish_request().

Some of your featured hosts also use litespeed, so I guess they also have to deal with this issue too.

mahagr commented 8 years ago

Unless they have similar function to fastcgi_finish_request(), gzip from the server cannot really used. If they do not have the function, ask what is the correct way to stop server from compressing the output.

The main reason for the code we have is that it allows us to close the connection and to continue processing some cron job taking a lot time. Without the ability to close the connection, users would see page loading a long time even if they already received the whole page.

litespeedtech commented 8 years ago

LiteSpeed can compress response with content-length header set without any problem. A properly written web server should be able handle it. When LiteSpeed compress the response body, it replaces "Content-Length" header with "Transfer-Encoding: chunked", the compressed response wont cause browser to spin forever.

Looks like we need to drop the "Content-Encoding:none" header internally.

ulab commented 7 years ago

I just found this issue because I was having a problem validating my site. The W3C Markup Validation Service will not work with a "Content-Encoding: none" header. It stops with an error "Don't know how to decode Content-Encoding 'none'".

Changing the value for system.cache.gzip to true in user/config/system.yaml fixed that.

mahagr commented 7 years ago

@rhukster I think the correct one would be using Content-Encoding: identity, which means "no compression, no modification"

miljan-aleksic commented 7 years ago

What is the state of this issue? I have run on it as @ulab and his solution works fine, but that means the default behaviour is buggy?

mahagr commented 7 years ago

Try latest CI build of Grav if it works in there.

rhukster commented 7 years ago

Ok, so i've made a change to this. Changing to 'identity' was causing some encoding problems for some people. I've rolled back the default to 'none' which actually works more reliably with our onShutDown() event. However, you can set the cache: allow_webserver_gzip: true in system.yaml if you want to change the behavior and don't care about the onShutDown() potential slowdown.

miljan-aleksic commented 7 years ago

@rhukster, you may want to comment this behaviour in the docs. The impression I got when tried to validate my site with W3C validator is that there is something broken. Gzip is recommended, but now should be considered a must if you want your site to be correctly validated and/or perhaps indexed (if W3C refuses to read the content, there is no warranty others will do).

rhukster commented 7 years ago

I have a note in my "items to document" mega issue: https://github.com/getgrav/grav-learn/issues/356

You can enable Gzip via the grav option directly. This will ensure gzip is working for the .php pages, and it will still work with onShutdown() event properly.