On Wed, Oct 3, 2018 at 5:14 PM Evgeny Kotkov <evgeny.kot...@visualsvn.com> 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);