On Wed, Oct 3, 2018 at 5:14 PM Evgeny Kotkov
<[email protected]> wrote:
>
> I am thinking about fixing this with the attached patch and proposing it for
> backport to 2.4.x.
>
> Would there be any objections to that?
+1 for the patch, I missed the separate 304 handling in
mod_brotli/deflate, thanks for catching this!
It seems that the 304 "shortcut" happens too late though, after
entity/content-* headers are added to the response, which does not
look right. Don't we need something like the attached patch too?
Regards,
Yann.
Index: modules/filters/mod_brotli.c
===================================================================
--- modules/filters/mod_brotli.c (revision 1840709)
+++ modules/filters/mod_brotli.c (working copy)
@@ -414,21 +414,6 @@ static apr_status_t compress_filter(ap_filter_t *f
return ap_pass_brigade(f->next, bb);
}
- /* If the entire Content-Encoding is "identity", we can replace it. */
- if (!encoding || ap_cstr_casecmp(encoding, "identity") == 0) {
- apr_table_setn(r->headers_out, "Content-Encoding", "br");
- } else {
- apr_table_mergen(r->headers_out, "Content-Encoding", "br");
- }
-
- if (r->content_encoding) {
- r->content_encoding = apr_table_get(r->headers_out,
- "Content-Encoding");
- }
-
- apr_table_unset(r->headers_out, "Content-Length");
- apr_table_unset(r->headers_out, "Content-MD5");
-
/* https://bz.apache.org/bugzilla/show_bug.cgi?id=39727
* https://bz.apache.org/bugzilla/show_bug.cgi?id=45023
*
@@ -461,6 +446,21 @@ static apr_status_t compress_filter(ap_filter_t *f
return ap_pass_brigade(f->next, bb);
}
+ /* If the entire Content-Encoding is "identity", we can replace it. */
+ if (!encoding || ap_cstr_casecmp(encoding, "identity") == 0) {
+ apr_table_setn(r->headers_out, "Content-Encoding", "br");
+ } else {
+ apr_table_mergen(r->headers_out, "Content-Encoding", "br");
+ }
+
+ if (r->content_encoding) {
+ r->content_encoding = apr_table_get(r->headers_out,
+ "Content-Encoding");
+ }
+
+ apr_table_unset(r->headers_out, "Content-Length");
+ apr_table_unset(r->headers_out, "Content-MD5");
+
ctx = create_ctx(conf->quality, conf->lgwin, conf->lgblock,
f->c->bucket_alloc, r->pool);
f->ctx = ctx;
Index: modules/filters/mod_deflate.c
===================================================================
--- modules/filters/mod_deflate.c (revision 1840709)
+++ modules/filters/mod_deflate.c (working copy)
@@ -772,12 +772,22 @@ static apr_status_t deflate_out_filter(ap_filter_t
"Forcing compression (force-gzip set)");
}
+ if (c->etag_opt != AP_DEFLATE_ETAG_NOCHANGE) {
+ deflate_check_etag(r, "gzip", c->etag_opt);
+ }
+
+ /* For a 304 response, only change the headers */
+ if (r->status == HTTP_NOT_MODIFIED) {
+ ap_remove_output_filter(f);
+ return ap_pass_brigade(f->next, bb);
+ }
+
/* At this point we have decided to filter the content. Let's try to
* to initialize zlib (except for 304 responses, where we will only
* send out the headers).
*/
- if (r->status != HTTP_NOT_MODIFIED) {
+ {
ctx->bb = apr_brigade_create(r->pool, f->c->bucket_alloc);
ctx->buffer = apr_palloc(r->pool, c->bufferSize);
ctx->libz_end_func = deflateEnd;
@@ -832,16 +842,7 @@ static apr_status_t deflate_out_filter(ap_filter_t
}
apr_table_unset(r->headers_out, "Content-Length");
apr_table_unset(r->headers_out, "Content-MD5");
- if (c->etag_opt != AP_DEFLATE_ETAG_NOCHANGE) {
- deflate_check_etag(r, "gzip", c->etag_opt);
- }
- /* For a 304 response, only change the headers */
- if (r->status == HTTP_NOT_MODIFIED) {
- ap_remove_output_filter(f);
- return ap_pass_brigade(f->next, bb);
- }
-
/* add immortal gzip header */
e = apr_bucket_immortal_create(gzip_header, sizeof gzip_header,
f->c->bucket_alloc);
