git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/


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